Depends on D3635
The logic for writeResponse handling writing compaction chunks
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Is it possible for someone to append a log while we're reading it? What is the plan for handling such scenario?
| services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h | ||
|---|---|---|
| 109–112 ↗ | (On Diff #11110) | Could you describe when this may happen and why? |
| 115–117 ↗ | (On Diff #11110) | I think in this case it's better to use if. We could have something like this: if (this->state == COMPACTION) {
...
if (...) {
this->state = LOGS;
}
}
if (this->state == LOGS) {
...
}What do you think? |
| 125–127 ↗ | (On Diff #11110) | When this->currentLogIndex > this->logs.size() it could mean that something went wrong |
| services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h | ||
|---|---|---|
| 109–112 ↗ | (On Diff #11110) | This is a sanity check - I'm not sure if it is possible to occur but wanted to make sure we never ever allow this. |
| 115–117 ↗ | (On Diff #11110) | I'm not sure if using if instead of switch is good here. I use switch for controlling the flow in reactor implementations so being consistent about this is good I think. Looking at this as a separated example, I'd say what you're suggesting is equivalent to the existing code - these are just two ways of handling this, I don't really see any big pros/cons of one of them over the other. |
| 125–127 ↗ | (On Diff #11110) | Yes, you're right. We should probably throw. |
| services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h | ||
|---|---|---|
| 115–117 ↗ | (On Diff #11110) | It is true that the execution is the same but switch approach is worse. The biggest issue is that you had to write a comment to explain what's going on - it means that this isn't intuitive. With if you wouldn't need to do that.
Consistency is really important, but we shouldn't sacrifice maintainability and readability just for it. |
Comment Actions
I agree switch doesn't seem great here given we are entering two of the blocks. Yes this is possible with switch, but personally I believe it should be avoided and considered an anti-pattern. I think @palys-swm's framing is good: if you can ever avoid writing a comment by changing the code around, then you should change the code around.
Comment Actions
I still lean towards switch but since I'm outvoted, I'm just going to push the change with if.
Comment Actions
The indentation is really deep in this code. Could you spend some time verifying if there are ways to reduce it?
| services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h | ||
|---|---|---|
| 107 ↗ | (On Diff #11603) | We're returning in if so maybe we should delete this else |
| services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h | ||
|---|---|---|
| 107 ↗ | (On Diff #11603) | right |
| services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h | ||
|---|---|---|
| 91–92 ↗ | (On Diff #11692) | I remember asking about it somewhere and you've probably answered, but what is the plan for a scenario where someone appends a log after we started reading but before we finish? |
Comment Actions
@karol-bisztyga you should NEVER, EVER land a diff without responding to every single comment on the diff. We talked about this in the retro yesterday... it continues to be a serious issue.
Honest question: what do you think of the idea of having @palys-swm only accept your diffs if he has zero comments?
| services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h | ||
|---|---|---|
| 91–92 ↗ | (On Diff #11692) | The diff was landed without replying to this question! |
Comment Actions
Ahh, I see what's wrong, I wrote a comment but forgot to hit Submit :( Sorry
| services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h | ||
|---|---|---|
| 91–92 ↗ | (On Diff #11692) | I honestly don't remember a discussion like this. What plan could be there? We're going to read the current state of the logs. It feels a bit weird to see a scenario containing two people. Backups are individual, right? |
| services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h | ||
|---|---|---|
| 91–92 ↗ | (On Diff #11692) | Ok, so let's be more precise. By someone, I meant two different clients, and in that case the same user could be logged in on both. There are three ways of handling this:
I think that option 2 is easier, but at the same time we provide something which we know is outdated. Option 3 can be implemented e.g. by checking if there are new entries when we finished sending logs. Option 2 has a big issue when we restore the state from a backup - we might miss some updates that were made after we started the process. We can work-around it by e.g. having currentAsOf field and fetch new logs from the client after the backup is restored. So overall, both options are ok, but we should choose one and have it in our minds while implementing the rest of the system. @karol-bisztyga @ashoat |
Comment Actions
I think ideally, the process of downloading logs is "checkpointed", which effectively means option 3 – basically, we download N logs at a time, starting with the oldest. After each download, the client passes back the timestamp on the newest log downloaded so far as a checkpoint, saying "what are the next logs to download after this one?" That way, if a new log is created, it should still be downloaded at the end.
Comment Actions
Ok, now I realize we might've talked about that, thanks for clarifying. Option no. 3 is in fact the best, I agree. I think this is too big of a topic to just create a diff right away with a fix. I think we should create a task for this with an explicit summary and a test plan. It may not be so easy to test it. Do you agree we should create a task for this or should we just proceed with a diff?
Comment Actions
It looks like the updated implementation will require a couple of diffs. At the same time, this code works ok. So it seems that handling this as a separate task is a good idea.