Depends on D3720
We need to be able to throw in the terminationCallback and handle it gracefully.
Details
Details
cd services yarn run-backup-service yarn run-blob-service
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/server/base-reactors/ServerWriteReactorBase.h | ||
|---|---|---|
| 42–48 ↗ | (On Diff #11419) | This should be a separate diff. In this diff we should only
|
| services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerWriteReactorBase.h | ||
|---|---|---|
| 42–48 ↗ | (On Diff #11419) | I don't understand this feedback, sorry. I'm indeed wrapping terminateCallback invocation in the try ... catch block. Can you specify what exactly is wrong here? |
Comment Actions
This diff isn't too complicated, but we should introduce a habit of having diffs that focus on one thing.
This diff does three things:
- It wraps terminateCallback with try-catch - which should be the only code here
- It introduces and calls validate method
- It fixes a bug where we were using state instead of this->state
Please split this diff.
As a side note, it would be a lot more efficient if the diff was split at the beginning. Each of these diffs would be an instant accept.
| services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerWriteReactorBase.h | ||
|---|---|---|
| 42–48 ↗ | (On Diff #11419) | Yes, you're doing that. But also you're introducing validate method, which isn't mentioned in diff title or summary. Please try to create diffs that do one thing. If that is not convenient, we should at least explain why is that. |
| services/blob/src/Reactors/server/base-reactors/ServerReadReactorBase.h | ||
| 52 ↗ | (On Diff #11419) | What is the reason of this change? Is it a part of wrapping with try-catch? It looks like this change fixes a bug. |