Differential D3719
[services] Backup - client reactors - distinguish terminated from done Authored by • karol on Apr 13 2022, 7:04 AM. Tags None Referenced Files
Details Depends on D3664 We need to separate two pieces of information: when the reactor is terminated and when it is "done".
Thing is, gRPC does some finalizing operations after we terminate a reactor. cd services yarn run-backup-service
Diff Detail
Event Timeline
Comment Actions What do you think about adding a state which can be RUNNING, TERMINATED and DONE? Maybe it will make this code more maintainable. Comment Actions Good idea
Comment Actions I implemented this in D3785 D3786 D3787 D3788 D3789 I know we talked about this that rebasing is better if it's not yet landed but in this case I think having it implemented separately is better because it fits better in that task and also this applies to all reactors not only the ones here. I hope you understnad. Comment Actions Thanks for implementing this! So the code that is introduced here is replaced by what is in that stack? Can we then abandon this diff? Comment Actions
This is a really bad pattern and a great example of behavior that is costly for your reviewer. You should not be submitting useless diffs to people that get overwritten by later diffs... and a reviewer should never be in a position of asking about something that gets addressed in a later diff. Instead, the stack should have been rebased, with this diff updated with the corrected approach. This is my number one piece of feedback to you, @karol-bisztyga – you need to move away from making commits as if you're on a solo project, and treating diffs as "here are some commits I made today"... and instead think of the stack you're trying to create, and to build your commits around a clean stack that is easy to review. Simpler framing: we need a lot fewer diffs from you, and a lot more rebasing. | ||||||||||||||||||||||||||||||||||||||||