Depends on D5004
Adding necessary imports and objects that we're going to use in the put client.
Details
Details
backup builds
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 28 | ||
| 31 | I don't think so. The purpose is to detect any errors in the async functions and break the flow yielding an error to the c++ if they occur. This was the simplest/fastest way of doing this but I'm open to suggestions. We can change it but I don't think we should just print them and move on. panic is not a way to go either. | |
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 31 |
If we enter an invalid state, then we should add some logic to get back to a valid state if possible.
If all we can do is just report the status, then we should print it and return that it failed. I don't see the value in indefinitely accumulating error messages.
Agreed, which is also why I don't like unwrap() | |
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 31 |
What do you mean exactly? I don't think this is a good idea. I think, if we enter an invalid state, we should break the connection and return a grpc status with an error.
If that's what's happening, then it is a mistake, I'm going to look at this code once again, if there's ever an error, we should exit with a proper Result(Err). I generally agree that the error handling here needs to be repaired big time. I just did it quickly and dirty. I created a task for this https://linear.app/comm/issue/ENG-1734/improve-error-handling-system-in-the-blob-client | |
Comment Actions
What do you mean exactly?
If the client gets to be in an unhealthy state, then there should be some logic to get try and get the client back into a healthy state and retry the get/put. Currently, an unhealthy client will both report an error and drop the message.
I don't think this is a good idea. I think, if we enter an invalid state, we should break the connection and return a grpc status with an error.
Yes, but the message also gets dropped. There should be some re-try logic (which can be done in another ticket).
Comment Actions
If the client gets to be in an unhealthy state, then there should be some logic to get try and get the client back into a healthy state and retry the get/put. Currently, an unhealthy client will both report an error and drop the message.
Sorry, but I disagree. I think when the client gets into an unhealthy state, it should drop the connection and return an error.
If we wanted to really attempt to retry, we should at least add some logic of when to do this and when not. If we get an error like provided holder already exists in the database, then what is the point of retrying? If we try again, it won't disappear magically I guess, will it? And adding logic for this will probably result in adding an additional layer of abstraction or something - we could have a type of error that has a flag shouldRetry, something like this. All in all, don't think we should do it here and I don't think this is necessary and important either.
The way I see it now is that we could have a button on the client side in the UI saying retry when there's an error returned.
Comment Actions
If we get an error like provided holder already exists in the database, then what is the point of retrying? If we try again, it won't disappear magically I guess, will it?
Wouldn't that mean that a previous put was already successful? Seems like a state that shouldn't ever occur, but I agree it should be handled by dropping.
I think the concept of the client and message are being fused here. There's a difference between a message being handled, and the client being healthy.
I'm refering to the lifecycle of a BidiClient, not the lifecycle of a message passed through a BidiClient.
Either way, seems like we are just pushing this through, and creating tasks for others to address concerns.