Depends on D4320
fixed the bug in if else in terminateCallback
The problem was that we were checking for the put reactor state only in the else if block, we should check for it always.
Details
Details
Services build, for now this went out of sync with the integration tests
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| services/backup/src/Reactors/server/SendLogReactor.cpp | ||
|---|---|---|
| 128 ↗ | (On Diff #13662) | this should be before we call store in the db |
Comment Actions
For the future, probably the best way would be to introduce a small change and in the same diff, do necessary changes in tests so all tests pass. But I spent some time debugging and wanted to put this up ASAP now as this also brings some changes to the overall logic.
Comment Actions
It's hard to tell which code change corresponds to which action
A couple of changes in the send log logic:
- we separate the value and the blobHash. Until now, we only used the member value that could really be either of these things, but now it is split into two separate fields so it's easier to understand.
- the logic of the readRequest is simplified
- fixed the bug in if else in terminateCallback
- checking the real item size against the dynamo DB limit
Could you split this diff so that each change is a separate one?
Comment Actions
Defer to @palys-swm on most of this, don't have time to do a full review
| services/backup/src/Reactors/server/SendLogReactor.cpp | ||
|---|---|---|
| 19–20 ↗ | (On Diff #13663) | Can we avoid this ternary here? It (just barely) violates rule 3 of "Use ternary conditionals sparingly":
|
Comment Actions
As I said in the description:
I realize this should be split into the separate diffs, I'm going to do this if we accept the high-level idea.
I'm going to assume that the high-level idea is accepted and just split this.
| services/backup/src/Reactors/server/SendLogReactor.cpp | ||
|---|---|---|
| 19–20 ↗ | (On Diff #13663) | For me this looks good, but ok |
| services/backup/src/Reactors/server/SendLogReactor.cpp | ||
|---|---|---|
| 19–20 ↗ | (On Diff #13663) | Honestly, I thought about this and do not see a decent replacement. What is an alternative here? string str = this->value; if (this->persistenceMethod == PersistenceMethod::BLOB) this->blobHolder; This causes re-assignment, which is more expensive and less readable for me. The ternary is pretty straight and clear. If you have any good suggestions, please let me know. As for violating "Use ternary conditionals sparingly" - I'm not sure if I overuse ternary and don't see how one ternary is an argument for saying that the ternary is not used "sparingly". Also, the condition's simple and it is not nested, etc. What makes you think this is a violation? |
Comment Actions
What makes you think this is a violation?
The language says:
Either the whole expression should fit on one line, or the ? and : characters should appear at the start of subsequent lines.
The readability of this ternary expression isn't great... it seems easy to miss that there is a conditional occuring here.
As for the best alternative... I am not sure about that. I don't feel that strongly about this case honestly, but I'm sure there is some alternative if you look into it. I don't have the time right now to do that research... in general, the expectation is that the diff author does research in response to requests from diff reviewers.
Comment Actions
Please describe in the summary what this bug was about. The test plan should describe how it was verified that the bug was fixed.
| services/backup/src/Reactors/server/SendLogReactor.cpp | ||
|---|---|---|
| 19–20 ↗ | (On Diff #13663) | You can consider assigning this->persistenceMethod == PersistenceMethod::BLOB to a variable - it is used twice and having a variable with shorter name would allow keeping the ternary |
This comment was removed by • karol.
Comment Actions
Please describe in the summary what this bug was about. The test plan should describe how it was verified that the bug was fixed.
| services/backup/src/Reactors/server/SendLogReactor.cpp | ||
|---|---|---|
| 135–138 ↗ | (On Diff #14024) | What is the scenario in which terminateCallback is called while persistenceMethod isn't BLOB or DB? |
| services/backup/src/Reactors/server/SendLogReactor.cpp | ||
|---|---|---|
| 135–138 ↗ | (On Diff #14024) | It may happen when an error is thrown in readRequest before we ever assign anything to the persistenceMethod. |
| services/backup/src/Reactors/server/SendLogReactor.cpp | ||
|---|---|---|
| 135–138 ↗ | (On Diff #14024) | we probably shouldn't store anything in the db if the status is not OK |
| services/backup/src/Reactors/server/SendLogReactor.cpp | ||
|---|---|---|
| 135–138 ↗ | (On Diff #14024) | I looked into it, it worked for some cases when the log item was incomplete during the operation of adding it to the database but it is not a good approach. Thanks for catching this, I'm going to add a check here to properly propagate the error. |
| services/backup/src/Reactors/server/SendLogReactor.cpp | ||
|---|---|---|
| 141 ↗ | (On Diff #14148) | This should not be here. If the status is ok, it means we finished readRequest successfully and the persistence method has to be either DB or BLOB. We may also add a sanity check for this - if the state here is neither DB nor BLOB, we may throw. |
| services/backup/src/Reactors/server/SendLogReactor.cpp | ||
|---|---|---|
| 145 ↗ | (On Diff #14245) | In this case maybe it is a good idea to check if the method is DB? |