services/backup/src/Reactors/ReactorUtility.h
19 ↗	(On Diff #12038)	`ReactorUtility` this name doesn't sound too useful. At this point this class is e.g. `ReactorStateManager` or `ReactorStateHolder`. We could even create a generic version of it and store arbitrary value, something like `template<typename T> SynchronizedValueHolder<T>` where we store value `T` and synchronize on a mutex every time it is set or queried. The question is, what is the plan for this class. Are we going to introduce more logic to it? Because if that's not the case, we should at least change the name and maybe even consider making it more generic.

ashoat accepted this revision.Apr 30 2022, 4:08 PM

ashoat added inline comments.

services/backup/src/Reactors/ReactorUtility.h
19 ↗	(On Diff #12038)	Agree with @palys-swm's comments on the class name. Also wondering if something like `SynchronizedValueHolder` already exists in `std`

This revision is now accepted and ready to land.Apr 30 2022, 4:08 PM

Requesting a review as I think we should get on the same page about the problem mentioned in inlines.

services/backup/src/Reactors/ReactorUtility.h
19 ↗	(On Diff #12038)	Not sure if you noticed but this class has two fields: status - this is a grpc status and access to it is synchronized manually with a mutex. It is here as every reactor is going to hold status as a result of the grpc operation. state - this is a reactor state, synchronized using `atomic` It would be generic if we had only the `status` field. But as we have both, it's more reactor-specific to me, to be honest. We can extract something like `template<typename T> SynchronizedValueHolder<T>` as a separate class as you suggested. `ReactorStateManager` and `ReactorStateHolder` don't work for me because we not only store state but also status here. I'm not really convinced by what you said, sorry. Maybe you missed that there are two fields and they're specific to reactors? I realize I'm outvoted but please consider what I said. I'd leave this solution as is, we can change the name, I don't mind.
19 ↗	(On Diff #12038)	wondering if something like `SynchronizedValueHolder` already exists in `std` I'm not aware of anything like this.

ashoat accepted this revision.May 4 2022, 8:58 AM

ashoat added inline comments.

services/backup/src/Reactors/ReactorUtility.h
19 ↗	(On Diff #12038)	Personally, I didn't notice there were two values. I see your point @karol-bisztyga, but also think `ReactorUtility` is too generic of a name. Can we come up with a better name that reflects what this class is doing? Maybe `ReactorStatusHolder` or something?

This revision is now accepted and ready to land.May 4 2022, 8:58 AM

• karol added inline comments.May 5 2022, 1:41 AM

services/backup/src/Reactors/ReactorUtility.h
19 ↗	(On Diff #12038)	The rest of this stack is accepted, I think it's best to land the whole thing and discuss the naming separately to unblock the work. Changing the name's going to be an easy change. Created a task for this https://linear.app/comm/issue/ENG-1086/discuss-the-structure-of-reactor-utility

Closed by commit rCOMM483c82d173bd: [services] Backup - Pull more mutual code from base reactors - Add base reactor (authored by • karol). · Explain WhyMay 5 2022, 1:47 AM

This revision was automatically updated to reflect the committed changes.

• karol added a commit: rCOMM483c82d173bd: [services] Backup - Pull more mutual code from base reactors - Add base reactor.

• karol mentioned this in rCOMM875fbc6e1c9d: [services] Blob - Pull more mutual code from base reactors - Add base reactor.

I don't agree with landing this with the current name. You do this too often – like you said, it is trivial to rename the utility, and I gave you a suggestion.

@karol-bisztyga I really think you need to get more comfortable with interactive rebase and editing a stack. Until you do, you will continue to make life hard for you and for your reviewers. The more you avoid editing a stack, the longer you make the stack, the harder you make it to edit your stack.

In D3785#109902, @ashoat wrote:

I don't agree with landing this with the current name. You do this too often – like you said, it is trivial to rename the utility, and I gave you a suggestion.

I don't understand what it is that I do too often, sorry. We've not agreed on anything, you said:

Maybe ReactorStatusHolder or something?

So I didn't perceive it as a strong suggestion. Therefore I created a follow-up task.

@karol-bisztyga I really think you need to get more comfortable with interactive rebase and editing a stack. Until you do, you will continue to make life hard for you and for your reviewers. The more you avoid editing a stack, the longer you make the stack, the harder you make it to edit your stack.

I am comfortable with interactive rebase and editing a stack. What you're referring to here is in the past and we've discussed it - there were situations where I did too much follow-ups. This is not one of these cases. Why would I avoid editing a stack when editing means changing one name? It's not expensive, hard, etc. I did it because I wanted to unblock. Changing the name in the future is equally cheap. I think it is more expensive to hold the entire stack because of one single name without having really strong preferences than to change one name in the codebase in the future.

I don't understand what it is that I do too often, sorry.

The thing you do too often is that you choose to defer responding to comments from your reviewers, and instead create new tasks and new diffs to address them.

This results in a very long stack and a lot of moving pieces. It makes it harder for me to track your work. It makes it harder for me to review your diffs.

More details on this feedback here.

So I didn't perceive it as a strong suggestion. Therefore I created a follow-up task.

This is an anti-pattern... regardless of whether it is a strong or weak suggestion, if it is easy to address immediately you should address it immediately.

there were situations where I did too much follow-ups. This is not one of these cases.

Strong disagree, this is absolutely a case where you are adding unnecessary overhead. There is a fixed cost to each diff, to each task. You could have easily addressed the feedback immediately, and it would've saved us overhead.