Differential D4245
[services] Backup - Add current id and attachment holders Authored by • karol on Jun 13 2022, 6:49 AM. Tags None Referenced Files
Details Depends on D4233 We need to be able to separate logs from each other when we pull the backup from the cloud. With the current id, we can keep track of what is the current log of the chunk that is being pushed so that when it changes, we know that the current chunk goes for a different log. none - it is going to be used later in the stack. Services still can be built.
Diff Detail
Event TimelineComment Actions Also would be good to have more context on attachmentHolders – is this for a compaction, or for a log? How do we know which compaction / log the attachment corresponds to? Also – won't the compaction / log have a pointer into the attachment holder anyways, inside the encrypted text? I would assume that it does...
Comment Actions One last question – isn't it the case that attachmentHolders should be provided in tandem with a given compactionChunk or logChunk? It seems to me like it would reduce round trip times if we did something like this: message PullBackupResponse { oneof id { string backupID = 1; string logID = 2; } oneof data { bytes compactionChunk = 3; bytes logChunk = 4; } optional string attachmentHolders = 5; } Not sure what the impact might be on the size of the message, though... I know this can sometimes be tricky to make sure it doesn't exceed some limit.
Comment Actions That does not look good to me, sorry. Message size is one thing but it just looks like a waste of space. If we do it as you suggest, we'll send attachment holders in every single message. So, if we send 7 chunks of backup and this backup has attachments, we're going to send the same holders 7 times. I'd rather send the attachments in a separate cycle. I think it makes more sense and brings less waste. The thing is, we're going to read these holders just once, and then we're probably just going to ignore them.
Comment Actions
I don't understand how my suggestion would result in holders being repeated. I think there are trivial ways to avoid this, no? Comment Actions Confused why don't just make these changes right now. I understand it may be easier for @karol-bisztyga to keep your existing code, but this pattern leads to more work for reviewers. I think we should update all of the existing diffs in this existing stack to reflect the new proposed proto Comment Actions We've been in this situation multiple times. Look at the size of this stack. Do you realize how expensive is to keep amending and rebasing? It also makes it harder for the reviewers because if I amend an existing commit I should re-request a review even on the accepted diffs. So I don't do this because I'm selfish-lazy and just want to make everything comfortable for myself. That results is you reviewing the same code again and again only because I had to change one line or something. I don't know how this even works for you but for me, it is just a mess. Please look at this stack. It's almost all green and you keep blocking the very first diff of this stack. Why cannot we just land this and address the changes later instead of amending these diffs forever?
Strongly disagree. I think the pattern you're suggesting leads to more work for both reviewers and committers for the reason I mentioned above. Comment Actions Also - look at your comment here: https://phab.comm.dev/D4439#126271 I came up with a solution that can be included in 4 separate diffs. If we decide that the design is bad, we can just abandon them and come up with something else. If I updated existing diffs with that code, it would be a pure nightmare to really deduce the system design changes, decide if they're good or not, and if not also reverting them would be extremely error-prone and uncomfortable, don't you think? Comment Actions Hey @karol-bisztyga, last time we talked about this it seemed like we were all on the same page that this is the practice on the team. I can see your diff stack and how huge it is, but the feedback I've shared previously with you is that your diff stack simply should not be that large. It's my perception that you are putting yourself in this position – every time you want to make a change, you make a new diff. And then when changes are requested, you complain that the diff stack is too large now and the only way to solve it is to make yet another diff. I would guess that your diff stacks are ~3-5x larger than anybody else's on the team. If you still want to re-request review on this one, can you please INSTEAD schedule a 1:1 to discuss? Here's a link. I would really appreciate it if you do not re-request review here before we have time to discuss 1:1. Separately, I'm curious for @palys-swm's perspective on this one as well. Comment Actions
This is simply not true - in this very stack I multiple times did rebase/amend. You never responded to my arguments above for this particular case. I get that this is the best practice but I don't think that the best practice is something we should do in 100% cases. Can you maybe refer to what I said here https://phab.comm.dev/D4245#126496?
This stack is so big because you've been blocking the very first diff for some cycles now. If you accept now, I'll land 19/36 diffs (because 18 diffs straight ahead of this one are already accepted), leaving the stack with 17 diffs among which another 9 are also accepted. I feel like you've been responding in some diffs recently not addressing what I said (another example: https://phab.comm.dev/D4375#126504), which probably would be a big problem if it happened the other way (me not addressing what you said). Is this a good practice? Is it also a good practice to block a stack like this instead of landing a huge amount of diffs and solve the problem separately? Especially if the problem is an enhancement (here it is) and the current solution is not broken?
I looked at your calendar and honestly, those slots do not look good. The earliest hours are around 1900 CET which is pretty late. We also have 1:1 on Thu, which makes it another 5 days of keeping this blocked. I just don't understand why cannot you just let it be landed. What is a gain here? This blocks a lot of work, when you look at the stack it even looks ridiculous. It's ok to have some rules, but is it so good to follow them blindly no matter what? If your office sets on flames, will you finish the review because we agreed on that or rather jump off the window? Ok, I'm not going to request a review, let's make it blocked for another week until we have our 1:1. Comment Actions This diff is now blocking 31 following accepted diffs. So if this gets accepted, we're able to land 32 diffs. Otherwise, we're going to stay with this huge stack dangling and every day there may be more and more commits on master making it harder to rebase and land later.  It hardly fits on my screen. @ashoat what's your perspective on continuing the discussion here and doing amend/rebase? Just a reminder that if we do amend/rebase we'll have to mark all changed diffs with "needs review" status, which will require all reviewers to revisit them and check again if the new changes play well with the code that is already accepted. This effectively results in reviewers reviewing the same code over and over again. What are your thoughts about the effectiveness of these actions? Comment Actions My high-level perspective is that you are the only person on the team who ever has diff stacks this large. I would like us to spend some more time thinking about why that is – I have many ideas here, most of which I have previously shared. I don't like the practice of adding on more diffs later because it avoids solving the real problem, which is the size of your diff stacks. We've been over this problem so many times and it doesn't feel like it's getting better. At this point, my fear is that until you are forced to deal with the consequences of your huge diff stack (having to rebase it), you won't consider alternatives. In the past we have given you what I framed as one-time exceptions, with the hope that you would take the feedback and improve so the exceptions would not be necessary again. I don't want to keep doing that, because I'm afraid things won't get better and you will continue to demand exceptions instead of dealing with the consequences of your huge diff stack.
It's up to you if you want to re-request review on other diffs in the stack, but if your changes are not substantial then I do not see why you would do that. To clear up any confusion, there is no expectation from me that you re-request review when rebasing – only when you have substantial changes.
By a large measure, you are the diff author on the team who takes the most of my review time. I need to be effective with my time, and unfortunately that means I won't always be able to respond to every comment, especially when you leave very long, combative comments. My hope is that as you get more experience you'll be able to be more independent and require less direct engagement from me.
It's really not about this stack. It's about a repeated pattern of behavior that we've discussed over months of feedback, that does not seem like it's getting better. Comment Actions
Yes, alright, maybe this is because I land the biggest amount of code on the team? What then? Sources: Also this script: #!/bin/bash set -e declare -a CONTRIBUTORS=("karol-bisztyga" "Jacek Nitychoruk" "atul" "Ashoat Tevosyan" "Tomasz Palys" "Max Kalashnikoff" "Abosh Upadhyaya" "Varun Dhananjaya" "marcinwasowicz" "Jonathan Ringer") COMM_DIR=~/Desktop/workspace/comm pushd $COMM_DIR echo "----------" for CON in "${CONTRIBUTORS[@]}"; do RES=$(git log --first-parent --shortstat --author "$CON" --since "2 months ago" | grep "files changed" | awk '{files+=$1; inserted+=$4; deleted+=$6} END {print "files changed", files, "lines inserted:", inserted, "lines deleted:", deleted}') echo "$CON: $RES" done; echo "----------" popd echo "done" Results: ---------- karol-bisztyga: files changed 272 lines inserted: 3277 lines deleted: 1498 Jacek Nitychoruk: files changed 18 lines inserted: 1235 lines deleted: 206 atul: files changed 261 lines inserted: 2246 lines deleted: 1504 Ashoat Tevosyan: files changed 49 lines inserted: 1030 lines deleted: 783 Tomasz Palys: files changed 59 lines inserted: 2070 lines deleted: 761 Max Kalashnikoff: files changed 35 lines inserted: 285 lines deleted: 98 Abosh Upadhyaya: files changed 61 lines inserted: 323 lines deleted: 878 Varun Dhananjaya: files changed 51 lines inserted: 1197 lines deleted: 134 marcinwasowicz: files changed 62 lines inserted: 853 lines deleted: 146 Jonathan Ringer: files changed 13 lines inserted: 280 lines deleted: 9 ---------- Of course, I may have this wrong, if you have a better tool for checking committed lines of code for the last n days for specific contributors - you can let me know and/or share the results. The only person who is close is Atul. But it's just one person, so arguments like "you are the only one who has big diff stacks in the team" simply do not work if you look at these numbers and how far away they are. The problem exists because I often put up code faster than the code review can go for one feature, so those stacks just grow like this.
See, this is a problem. If you don't have time, you can always resign as a reviewer and that's ok. If you first block something and then say you don't have time, it doesn't make sense.
I am independent, I constantly throw ideas and solve problems on my own, and being blocked by you for no good reason is not a sign of not being independent. You still have not responded to https://phab.comm.dev/D4245#126496 even though I explicitly asked for that. Can you respond to that? Thanks. | ||||||||||||||||||||||||||||||||||||||||||||||