Repository navigation
RATIS-2804. Fix gRPC log-appender.connections conf and code cleanup - #1685
Conversation
Assisted by claude-opus-5-5
amaliujia
left a comment
amaliujia
left a comment
There was a problem hiding this comment.
Overall LGTM. Just a minor comment.
| } | ||
|
|
||
| /** | ||
| * This method must be called only when there are other channels. |
There was a problem hiding this comment.
This is an odd contract that the caller has to be aware of otherStubs is not null otherwise this call will hit an NPE.
The best way to avoid it is to make this nextChannelIndex as private thus only this class will use it and we know what we are doing. However I see that we need it in testing as well so have to keep this as package private. But then package private means other Ratis developer might still call it and cause unseen issue when testing is not enough or reviewers not having enough context.
I am wondering if we can return -1 when otherStubs is null? Though still not perfect, it avoids surprising NPE. Meanwhile negative values are often used to indicate something wrong in other languages like C++.
There was a problem hiding this comment.
Since the method is not expected to be called when otherStubs is null, throwing an exception is better. Here is some AI suggestion:
There was a problem hiding this comment.
Yep throw an exception other than NPE also works.
|
AI also suggests to throw IllegalStateException instead of NPE. Pushed a change for it. |
|
@amaliujia , thanks for reviewing and merging this! |
See RATIS-2804.
Assisted by claude-opus-5-5