Depends on D3873
https://linear.app/comm/issue/ENG-916/consider-pulling-more-mutual-code-from-the-async-reactors
Adding a master base reactor for all reactors
Details
Details
cd services yarn run-backup-service
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
| 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. |
| 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 |
Comment Actions
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:
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) |
I'm not aware of anything like this. |
| 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? |
| 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 |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Changing the name in the future is equally cheap.
Absolutely disagree. It is not equally cheap. Tasks and diffs are not free. They each have overhead.
Instead of continuing to churn on this diff, let's discuss in our 1:1.