Page MenuHomePhabricator
Differential D3719

[services] Backup - client reactors - distinguish terminated from done
ClosedPublic
Actions

Authored by • karol on Apr 13 2022, 7:04 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 19, 8:59 PM
Unknown Object (File)
Tue, Mar 4, 6:57 PM
Unknown Object (File)
Feb 19 2025, 4:20 PM
Unknown Object (File)
Feb 6 2025, 10:55 PM
Unknown Object (File)
Jan 19 2025, 9:54 PM
Unknown Object (File)
Jan 17 2025, 4:07 AM
Unknown Object (File)
Jan 8 2025, 10:49 PM
Unknown Object (File)
Jan 8 2025, 6:50 PM
View All Files
Subscribers
• abosh
• adrian
ashoat
atul
• benschac
tomek

Details

Reviewers
tomek
Commits
rCOMM040e1b7f2f9c: [services] Backup - client reactors - distinguish terminated from done
Summary

Depends on D3664

We need to separate two pieces of information: when the reactor is terminated and when it is "done".

  • terminated - this means that there are no more operations defined by the user
  • done - this means that there will be no more gRPC operations performed for this reactor (all of 'em are done)

Thing is, gRPC does some finalizing operations after we terminate a reactor.

Test Plan
cd services
yarn run-backup-service

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• karol created this revision.Apr 13 2022, 7:04 AM
Herald added subscribers: • abosh, • benschac, atul and 3 others. · View Herald TranscriptApr 13 2022, 7:04 AM
Harbormaster completed remote builds in B8155: Diff 11417.Apr 13 2022, 7:08 AM
• karol requested review of this revision.Apr 13 2022, 7:08 AM
• karol edited the summary of this revision. (Show Details)Apr 13 2022, 7:12 AM
• karol edited the test plan for this revision. (Show Details)
• karol added a reviewer: tomek.
• karol added a parent revision: D3664: [services] Backup/Blob - Make sure Finish is only called once.
• karol added a child revision: D3720: [services] Backup - Blob client reactors - add getStatus.
tomek requested changes to this revision.Apr 15 2022, 9:45 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientBidiReactorBase.h
114–115 ↗(On Diff #11417)

What is the relationship between terminated and done? Based on the summary it looks like a reactor should never be done and not terminated. Is that the case? Should we change this code

This revision now requires changes to proceed.Apr 15 2022, 9:45 AM
tomek added a comment.Apr 15 2022, 9:48 AM

What do you think about adding a state which can be RUNNING, TERMINATED and DONE? Maybe it will make this code more maintainable.

• karol added a comment.Apr 19 2022, 6:40 AM
In D3719#103333, @palys-swm wrote:

What do you think about adding a state which can be RUNNING, TERMINATED and DONE? Maybe it will make this code more maintainable.

Good idea

services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientBidiReactorBase.h
114–115 ↗(On Diff #11417)

Yes

• karol updated this revision to Diff 11661.Apr 20 2022, 4:10 AM
• karol edited the summary of this revision. (Show Details)

modify done after calling terminate

Harbormaster completed remote builds in B8351: Diff 11661.Apr 20 2022, 4:16 AM
• karol added a comment.Apr 20 2022, 4:22 AM
In D3719#103333, @palys-swm wrote:

What do you think about adding a state which can be RUNNING, TERMINATED and DONE? Maybe it will make this code more maintainable.

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.

tomek accepted this revision.Apr 20 2022, 7:24 AM
In D3719#104588, @karol-bisztyga wrote:
In D3719#103333, @palys-swm wrote:

What do you think about adding a state which can be RUNNING, TERMINATED and DONE? Maybe it will make this code more maintainable.

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.

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?
But overall, it makes sense to do such a big refactoring in a separate place.

This revision is now accepted and ready to land.Apr 20 2022, 7:24 AM
• karol added a comment.Apr 21 2022, 1:47 AM

I think we can safely land this with no harm done.

Closed by commit rCOMM040e1b7f2f9c: [services] Backup - client reactors - distinguish terminated from done (authored by • karol). · Explain WhyApr 21 2022, 1:49 AM
This revision was automatically updated to reflect the committed changes.
• karol added a commit: rCOMM040e1b7f2f9c: [services] Backup - client reactors - distinguish terminated from done.
ashoat added a comment.EditedApr 21 2022, 1:25 PM

I think we can safely land this with no harm done.

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.

Revision Contents
Changeset List

PathSize
services/
backup/
docker-server/
contents/
server/
src/
Reactors/
client/
base-reactors/
12 lines
16 lines
12 lines

Diff 11661

View Options

services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientBidiReactorBase.h

::-webkit-scrollbar { ::-webkit-scrollbar {
width: 15px; width: 15px;
} }
::-webkit-scrollbar-track-piece { ::-webkit-scrollbar-track-piece {
background-color: var(--shades-black-80); background-color: var(--shades-black-75);
ginsuAuthorUnsubmitted
Done
Inline Actions

This will be fixed with https://linear.app/comm/issue/ENG-5687/desktop-app-scrollbar

ginsu: This will be fixed with https://linear.app/comm/issue/ENG-5687/desktop-app-scrollbar
} }
::-webkit-scrollbar-thumb { ::-webkit-scrollbar-thumb {
background-color: var(--shades-black-60); background-color: var(--shades-black-50);
border: 3px var(--shades-black-80) solid; border: 3px var(--shades-black-75) solid;
border-radius: 10px; border-radius: 10px;
} }
View Options

services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientReadReactorBase.h

::-webkit-scrollbar { ::-webkit-scrollbar {
width: 15px; width: 15px;
} }
::-webkit-scrollbar-track-piece { ::-webkit-scrollbar-track-piece {
background-color: var(--shades-black-80); background-color: var(--shades-black-75);
ginsuAuthorUnsubmitted
Done
Inline Actions

This will be fixed with https://linear.app/comm/issue/ENG-5687/desktop-app-scrollbar

ginsu: This will be fixed with https://linear.app/comm/issue/ENG-5687/desktop-app-scrollbar
} }
::-webkit-scrollbar-thumb { ::-webkit-scrollbar-thumb {
background-color: var(--shades-black-60); background-color: var(--shades-black-50);
border: 3px var(--shades-black-80) solid; border: 3px var(--shades-black-75) solid;
border-radius: 10px; border-radius: 10px;
} }
View Options

services/backup/docker-server/contents/server/src/Reactors/client/base-reactors/ClientWriteReactorBase.h

::-webkit-scrollbar { ::-webkit-scrollbar {
width: 15px; width: 15px;
} }
::-webkit-scrollbar-track-piece { ::-webkit-scrollbar-track-piece {
background-color: var(--shades-black-80); background-color: var(--shades-black-75);
ginsuAuthorUnsubmitted
Done
Inline Actions

This will be fixed with https://linear.app/comm/issue/ENG-5687/desktop-app-scrollbar

ginsu: This will be fixed with https://linear.app/comm/issue/ENG-5687/desktop-app-scrollbar
} }
::-webkit-scrollbar-thumb { ::-webkit-scrollbar-thumb {
background-color: var(--shades-black-60); background-color: var(--shades-black-50);
border: 3px var(--shades-black-80) solid; border: 3px var(--shades-black-75) solid;
border-radius: 10px; border-radius: 10px;
} }