Depends on D3517
Add implementation for the create new backup reactor
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
There are a very large number of changes, so older changes are hidden. Show Older Changes
| services/backup/docker-server/contents/server/src/Reactors/server/CreateNewBackupReactor.h | ||
|---|---|---|
| 71 ↗ | (On Diff #10553) | Here we have to decide how exactly the backup id will be generated. |
Comment Actions
Please update the logs
| services/backup/docker-server/contents/server/src/Reactors/server/CreateNewBackupReactor.h | ||
|---|---|---|
| 45–47 ↗ | (On Diff #10654) | Why do we need to mock this function? Usually it's better to first introduce a function and then use it - by taking that approach you make sure that your code is always correct and it's a lot harder to forget about fixing the logic in the future |
| 81–83 ↗ | (On Diff #10654) | Can we somehow recover from this? Can we initialize it if it's not initialized? |
Comment Actions
I have very little context on the reactor stuff, and haven't reviewed the other diffs in this stack... so I don't think I can do a very thorough review here. @palys-swm, can you clarify what you wanted me to look at?
Removing from my queue until then, happy to review once it's clarified!
Comment Actions
The logs are going to be removed once this will no longer be a draft.
Once this is accepted as a draft, I'm going to remove the draft label and request a review as for a normal diff.
Back to your queue.
| services/backup/docker-server/contents/server/src/Reactors/server/CreateNewBackupReactor.h | ||
|---|---|---|
| 45–47 ↗ | (On Diff #10654) | I created a task for this https://linear.app/comm/issue/ENG-918/implement-generating-backup-id. |
| 81–83 ↗ | (On Diff #10654) | It is removed in https://phabricator.ashoat.com/D3488. |
Comment Actions
Sorry for the confusion - just wanted for you to take a look on Karol's inline comment
| services/backup/docker-server/contents/server/src/Reactors/server/CreateNewBackupReactor.h | ||
|---|---|---|
| 81–83 ↗ | (On Diff #10654) | It's usually a lot better to not introduce an error at all than to introduce and fix it later. Is there a reason why we should break this rule here? I guess it might be challenging to inverse the order here... |
| 71 ↗ | (On Diff #10553) | We have a couple of options: we can try using some UUID, or use a prefix as you suggested. With userID as a prefix we might have some privacy issues - not sure if they are real, but they need to be considered. This is a comment I wanted @ashoat opinion on |
| services/backup/docker-server/contents/server/src/Reactors/server/CreateNewBackupReactor.h | ||
|---|---|---|
| 71 ↗ | (On Diff #10553) | Here are some properties I think we should look for in a holder ID scheme:
|
Comment Actions
back to you
| services/backup/docker-server/contents/server/src/Reactors/server/CreateNewBackupReactor.h | ||
|---|---|---|
| 81–83 ↗ | (On Diff #10654) | Honestly, I'm not sure where did you come up with this rule from... For me it's pretty natural to throw and then fix it in an upcoming diff, a trivial example would be: void foo() {
throw("unimplemented");
}I'm still a bit confused with feedback like this which seems to be standing against the philosophy of diffs ("put up small diffs even when you're doing a big feature and if something turns out to need a different solution, just overwrite the code from the prior diffs"). |
| 71 ↗ | (On Diff #10553) | I think we can follow up in https://linear.app/comm/issue/ENG-918/implement-generating-backup-id |
| services/backup/docker-server/contents/server/src/Reactors/server/CreateNewBackupReactor.h | ||
|---|---|---|
| 81–83 ↗ | (On Diff #10654) | Actually, the philosophy is totally different. In diff philosophy, you start with things you're sure that are needed and know how to do them. Later you use the code you've introduced. So in the case you mentioned it would be e.g.: void aFunction() {
// implementation
}2: void function2() {
// implementation
}3: void foo() {
aFunction();
function2();
}So again, you start with introducing things that are not used, but at the same time after each diff all the codebase is correct. There are no regressions and exceptions. In real life it is sometimes useful to have a shortcut and start with usage and then add the implementation. The rule you mentioned was a solution to even worse pattern: where we had a stack of diffs opened for weeks and were constantly updating diffs from the begging. But if we started with things the we were sure about then this improvement wouldn't be needed. So to summarize, instead of
I would say:
|
| services/backup/docker-server/contents/server/src/Reactors/server/CreateNewBackupReactor.h | ||
|---|---|---|
| 81–83 ↗ | (On Diff #10654) | I think so - it doesn't matter too much in this case |
| services/backup/docker-server/contents/server/src/Reactors/server/CreateNewBackupReactor.h | ||
|---|---|---|
| 67 ↗ | (On Diff #10751) | In D3466 in ServiceBlobClient.put we have if (this->putReactor != nullptr && !this->putReactor->isDone()) {
throw std::runtime_error(
"trying to run reactor while the previous one is not finished yet");
}And here we're using ServiceBlobClient::getInstance(), which means that we're going to throw if we tried to call put before other put has finished. Does that mean that we can handle only a single request at a time? We should be able to have as many concurrent connections as we want, and not limit ourselves to just a single connection per service instance. (but I might be wrong) |
| services/backup/docker-server/contents/server/src/Reactors/server/CreateNewBackupReactor.h | ||
|---|---|---|
| 67 ↗ | (On Diff #10751) | Issues like this highlight that we should improve diffs / reviews process. It doesn't sound like an approach where we first introduce code with bugs and then fix the bugs later is maintainable long-term. In this case a better ordering would be to have D3488 before this diff. (we shouldn't change it now, but in the future we need to be more mindful about this to avoid forgetting about issues and to reduce the possible confusion when reviewing).
It's even harder to review them. We need to find ways of making the stacks more manageable. One easy solution might be to reduce their size by e.g. splitting by feature. In this case, this huge stack could be a number of stacks each introducing only one (or two: client and server) reactors. Do you think it would be more manageable? |
| services/backup/docker-server/contents/server/src/Reactors/server/CreateNewBackupReactor.h | ||
|---|---|---|
| 67 ↗ | (On Diff #10751) | I do agree there's room for improvement in terms of the process. I think we can discuss better approaches outside of this diff. For now, I'm going to land this.
I'm not sure about this. In such a case, we'll end up in a situation when the diffs will be decomposed into smaller stacks but effectively the stacks will have to be considered in certain order as well. |
| services/backup/docker-server/contents/server/src/Reactors/server/CreateNewBackupReactor.h | ||
|---|---|---|
| 67 ↗ | (On Diff #10751) |
Yeah, that definitely makes sense.
That's true, but there's no issue with the first diff of a stack to depend on one of the diffs from the other. Also, a diff can depend on multiple diffs, if that's helpful. |