Depends on D5016
Adding c++ implementation for using the blob client in the backup reactor for creating a new backup.
Details
Details
backup builds
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| services/backup/src/Reactors/server/CreateNewBackupReactor.cpp | ||
|---|---|---|
| 92 ↗ | (On Diff #16177) | I'm not a big fan str -> int -> bool conversion. Also just not a big fan of assuming the shape of a string. Feel like this logic would better reside in a proto file and we can use the types from the protobuf files to avoid having to roll out error prone str parsing logic. |
| services/backup/src/Reactors/server/CreateNewBackupReactor.cpp | ||
|---|---|---|
| 69 ↗ | (On Diff #16245) | This isn't used anywhere |
| 75–79 ↗ | (On Diff #16245) | We should create some tasks that cover things mentioned in the comment |
| 75–85 ↗ | (On Diff #16245) | This is confusing. We call put_client_blocking_read_cxx twice, and we ignore the result once. Could you explain (preferably in code comment) what's going on here? |
| 108–112 ↗ | (On Diff #16245) | Why do we need to read after write? |
| services/backup/src/Reactors/server/CreateNewBackupReactor.cpp | ||
|---|---|---|
| 69 ↗ | (On Diff #16245) | right |
| 75–79 ↗ | (On Diff #16245) | |
| 75–85 ↗ | (On Diff #16245) | We read the response to unblock the reading queue. grpc in the bidi stream always yields a response even if it's empty. And it is in this case, that's why we ignore it, but we read so the queue's not blocked (this queue is a rust's channel with MPSC_CHANNEL_BUFFER_CAPACITY limit). |
| 108–112 ↗ | (On Diff #16245) | Explained in the other comment |
| 92 ↗ | (On Diff #16177) | int -> bool is pretty straightforward. The only thing that may be problematic is string -> int.
I checked this function and for empty string, NaN, etc. it just returns false, so I think we're good.
Sorry, but I don't understand, can you clarify what you mean? Btw, not sure why I used string here, I'm going to try to use bool |
| services/backup/src/Reactors/server/CreateNewBackupReactor.cpp | ||
|---|---|---|
| 92 ↗ | (On Diff #16177) |
You can create a BidiReactorStatus enum within the protbuf files, and then all of the c++, rust, and other code bases can agree that the enum exists and agree about it's contents. Currently this is shared across C++ and rust, and I would like to avoid "stringly typed" intefaces as they are usually a source of issues later on. For c++ and rust, this is nice because we would now get compile-time errors, instead of runtime errors. And for Rust specificly, this is nice because of exhaustive case matching, so we would be less prone to not handling a certain status. This also delegates the responsibility of correctness to protobuf, instead of us having to do error-prone parsing in several locations. I think a bool only makes sense on things like is_XXX. But for status, we may have many potential statuses and enum would be a better fit. |
| services/backup/src/Reactors/server/CreateNewBackupReactor.cpp | ||
|---|---|---|
| 92 ↗ | (On Diff #16177) | I think you may not fully understand how things work here, at the same time I'm a bit confused by what you wrote, so I just predict what exactly you have in mind. We do not have to handle passing the outcome status around, this is done automatically in the grpc.
That's exactly what we have here - dataExists is the same as isDataPresent. I'm not sure what's the problem. What status do you want to send? Status of what exactly? |
| services/backup/src/Reactors/server/CreateNewBackupReactor.cpp | ||
|---|---|---|
| 74 ↗ | (On Diff #16354) | Do we really need to call this function here? Can't we just call one write after another? |
Comment Actions
Created https://linear.app/comm/issue/ENG-1755/avoid-stringly-typed-values to address my string -> int -> bool concerns
| services/backup/src/Reactors/server/CreateNewBackupReactor.cpp | ||
|---|---|---|
| 74 ↗ | (On Diff #16354) | For now, we have to. if we want to omit this, we should change the way functions are invoked. |