Depends on D3520
Add implementations for the server reactors - get 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
| services/blob/src/Reactors/server/GetReactor.h | ||
|---|---|---|
| 55 ↗ | (On Diff #10689) | Why do we add 1 here? |
| services/blob/src/Reactors/server/GetReactor.h | ||
|---|---|---|
| 55 ↗ | (On Diff #10689) | For now, I don't have an answer right away. This was introduced in D2672. I removed the + 1 and ran the blob test and it failed, saying that the retrieved string differed from the original one. If you feel a strong need to know why exactly this is needed, I can check, but I'm willing to trust our previous reviews as well as tests results. |
Comment Actions
It is accepted as a draft so I removed the draft label and I'm requesting changes once again to keep the appropriate order of events.
| services/blob/src/Reactors/server/GetReactor.h | ||
|---|---|---|
| 55 ↗ | (On Diff #10689) | It looks like Aws::IOStream is just a wrapper on std::basic_iostream where get method:
So having +1 here makes sense. Do you know if we can have \n in this file? If yes, then should we modify the logic so that reading doesn't stop on \n? |
| services/blob/src/Reactors/server/GetReactor.h | ||
|---|---|---|
| 55 ↗ | (On Diff #10689) | Nice catch with the \n! |
| 55 ↗ | (On Diff #10689) | Ok So I decided to fix this in a separate diff since this issue applies not only here but also in |