Return an error in the leader is not ready yet #98
Conversation
leader is not ready yet, add also a small fix in the aggregation
leader is not ready yet, add also a small fix in the aggregation
|
@victorges ready for another round. now you can do something like: if err != nil {
if errors.Is(err, stream.LeaderNotReady) {..}
if errors.Is(err, stream.ReplicaNotReady) {..}
if errors.Is(err, stream.StreamNotAvailable) {..}
....
}
|
Codecov Report
@@ Coverage Diff @@
## main #98 +/- ##
==========================================
- Coverage 79.49% 79.05% -0.44%
==========================================
Files 16 16
Lines 2390 2402 +12
==========================================
- Hits 1900 1899 -1
- Misses 357 368 +11
- Partials 133 135 +2
Continue to review full report at Codecov.
|
| @@ -135,6 +141,12 @@ func lookErrorCode(errorCode uint16) error { | |||
| return FrameTooLarge | |||
| case responseCodeAccessRefused: | |||
| return CodeAccessRefused | |||
| case responseCodeStreamNotAvailable: | |||
There was a problem hiding this comment.
Nice! I think I did see some "unknown response code" errors or something like that, will be helpful to have these.
pkg/stream/client.go
Outdated
|
|
||
| for _, replica := range streamMetadata.Replicas { | ||
| if replica == nil { | ||
| return nil, ReplicaNotReady |
There was a problem hiding this comment.
Maybe we should only filter the nil elements instead of a fatal error if any of them is not present?
For example, if only the leader and 1 replica from a 3 member cluster are available, should we not connect to the cluster at all or should we just connect to one of the online members? I'd generally assume the latter, but I'm not sure if it works differently for streams.
So as a concrete idea, I'd suggest moving this loop to the point where we append to the final brokers slice (line 633 rn), and instead of returning an error we just skip (continue) the nil brokers, only appending the non-nil replicas one-by-one to the slice. WDYT?
There was a problem hiding this comment.
Not totally sure to understand. Can you please fork the repo e propose a PR to my branch ? With your suggestions ?
Thank you
There was a problem hiding this comment.
Sure! Will do tomorrow when I get back to the computer ok?
Sorry for the confusion
There was a problem hiding this comment.
No problem at all :)
Thank you for your contributions!
There was a problem hiding this comment.
Just created a PR, to merge on the branch of this current PR, with the suggested fix!
|
Ok, @victorges we finally did it :) thank you! We have to close #99 and #43 then we can release the first RC. The first RC introduces a couple of breaking changes. |
leader is not ready yet, add also a small fix in the aggregation
fixes #96