Depends on D3729
While testing I spotted a few problems in the logic for pulling the backup. I'm going to describe each one of them in the inline comments.
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| services/backup/docker-server/contents/server/src/Reactors/client/blob/BlobGetClientReactor.h | ||
|---|---|---|
| 43 ↗ | (On Diff #11495) | We use it not only for reading compaction but also for reading logs so this message should be more generic. |
| services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h | ||
| 66 ↗ | (On Diff #11495) | I deferred checking whether it is a nullptr to the outer scope - we do not always need it. I find reset more readable as it indicates we're aware that we may deallocate the current object. |
| 101 ↗ | (On Diff #11495) | Previously, we checked for the nullptr inside of the initializeGetReactor function. Now, we check it here. |
| 114–117 ↗ | (On Diff #11495) | We want to terminate the connection if there were any errors during reading compaction instead of just proceeding to read the logs. |
| 130–133 ↗ | (On Diff #11495) | We don't want any dangling data when we're done reading logs. |
| 151–153 ↗ | (On Diff #11495) | We should check the state of the client reactor after reading every chunk of data. |
| 155 ↗ | (On Diff #11495) | should be deleted, sorry |
| 159 ↗ | (On Diff #11495) | should be deleted, sorry |
| services/blob/src/Reactors/server/GetReactor.h | ||
| 41 ↗ | (On Diff #11495) | Funny it wasn't spotted before. Since we're starting at index 0 like in arrays, we should subtract 1 here. It works, I tested it. |
| services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h | ||
|---|---|---|
| 108 | This else is completely unnecessary, please remove it. And please spend more time thinking about control flow, indentation, etc. It should look clean and minimal on the first submission | |
| 118–119 | This indentation scheme is nonsensical and it looks like someting is wrong with clang-format. Can you please look into it? | |
Comment Actions
It seems like there are some build errors
| services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h | ||
|---|---|---|
| 118–163 ↗ | (On Diff #11697) | This logic is really complicated, with deeply nested structure and duplication. Could you spend some time searching for a solution where the code is more readable? |
| services/blob/src/Reactors/server/GetReactor.h | ||
| 41 ↗ | (On Diff #11495) | It's not that funny - it means that the code wasn't tested enough. We need to focus on making test plans that catch issues like this. |
| services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h | ||
|---|---|---|
| 118–163 ↗ | (On Diff #11697) | Yes, ok. |
| services/blob/src/Reactors/server/GetReactor.h | ||
| 41 ↗ | (On Diff #11495) | I think we may want to wait for this task: https://linear.app/comm/issue/ENG-1020/backupblob-do-performance-tests I know it's about performance tests but once we have automated tests that use reactors, we can manipulate the inputs and see how different scenarios work. I try to test my solutions the best way I can but I think we're just not at such a stage yet - of testing the reactors in real-world scenarios carefully taking into consideration all possible inputs. This is because, for now, I test this code manually (not sure about the rest of the team). |
Comment Actions
I analyzed the code a bit and I'm not sure how to make it more readable, to be honest... I don't see a room for decomposition or something like this, but I may be wrong obviously. I added some comments inline to maybe make it easier for you. We can as well set up a call.
I understand that the code should be readable for someone else after half of a year as well. I can add some comments to the code as well. Or maybe do you know how else I can make code more readable in general (maybe you know some general, high-level rules)?
| services/backup/docker-server/contents/server/src/Reactors/server/PullBackupReactor.h | ||
|---|---|---|
| 120–122 ↗ | (On Diff #11702) | this means that there are no logs at all so we just terminate with the compaction |
| 123 ↗ | (On Diff #11702) | we reached the end of the logs collection so we just want to terminate |
| 124–126 ↗ | (On Diff #11702) | either we terminate with an error if we have some dangling data |
| 127 ↗ | (On Diff #11702) | or with success if we don't |
| 129–131 ↗ | (On Diff #11702) | we went out of the scope of the logs collection, this should never happen and should be perceived as an error |
| 132 ↗ | (On Diff #11702) | this means that we're not reading anything between invocations of writeResponse it is only not null when we read data in chunks |
| 133 ↗ | (On Diff #11702) | we pull next item from the logs collection |
| 135 ↗ | (On Diff #11702) | if the item is stored in the blob, we initialize the get reactor and proceed |
| 137–140 ↗ | (On Diff #11702) | if the item is persisted in the database, we just take it, send the data to the client and reset currentLog so the next invocation of writeResponse will take another one from the collection |
| 144–148 ↗ | (On Diff #11702) | we want to read the chunks from the blob through the get client until we get an empty chunk - a sign of "end of chunks" |
| 149–153 ↗ | (On Diff #11702) | if we get an empty chunk, we reset the currentLog so we can read the next one from the logs collection. |
Comment Actions
Thanks a lot for the comments - they are really helpful. It would be really great if you could add comments like these when putting up diffs with complicated logic - that would make reviewing them a lot faster.
When it comes to adding comments in code, I'd say that it would be hard to maintain them. We can have one, bigger comment, that explains the overall idea. What do you think?
| services/blob/src/Reactors/server/GetReactor.h | ||
|---|---|---|
| 41 ↗ | (On Diff #11495) | Ok, that makes sense. When it's really early it's indeed hard to test things - I sometimes write some code just to verify the logic. |
Comment Actions
Agree code comments would be great here. I'm okay with either one big comment or inline comments to be honest – understand @palys-swm's point but it might be easier to write and to read if they are individual comments
Comment Actions
Thanks a lot for the comments - they are really helpful. It would be really great if you could add comments like these when putting up diffs with complicated logic - that would make reviewing them a lot faster.
I understand, sometimes I may miss it as I'm not always sure what's complicated, but I'm going to try to think about this more.
When it comes to adding comments in code, I'd say that it would be hard to maintain them. We can have one, bigger comment, that explains the overall idea. What do you think?
Agree code comments would be great here. I'm okay with either one big comment or inline comments to be honest – understand @palys-swm's point but it might be easier to write and to read if they are individual comments
I also think smaller comments are better. It's hard to wrap the entire algorithm in one big comment. I mean I'm not sure if that would be helpful.