Fix potential IllegalCapacityException in LLRC when selecting nodes #37821
Conversation
|
Hi @SivagurunathanV, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile? |
There was a problem hiding this comment.
The change itself looks like the solution proposed in #37739, so that part looks fine. It would be good to have some sort of test though that checks this behaviour. It looks like it should be easy to unit-test RestClient#selectNodes even though it is package-private. There is already a RestClientTests that should be a good place for this.
cbuescher
added
>bug
:Clients/Java Low Level REST Client
|
Pinging @elastic/es-core-features |
cbuescher
changed the title
There was a problem hiding this comment.
As @cbuescher mentioned, do you think you could add a unit test for this @SivagurunathanV?
| @@ -398,7 +398,7 @@ private void setHeaders(HttpRequest httpRequest, Collection<Header> requestHeade | |||
| /* | |||
| * Sort the nodes into living and dead lists. | |||
| */ | |||
| List<Node> livingNodes = new ArrayList<>(nodeTuple.nodes.size() - blacklist.size()); | |||
| List<Node> livingNodes = new ArrayList<>(Math.max(0,nodeTuple.nodes.size() - blacklist.size())); | |||
There was a problem hiding this comment.
Super minor nit, could you add a space after the comma?
| List<Node> livingNodes = new ArrayList<>(Math.max(0,nodeTuple.nodes.size() - blacklist.size())); | |
| List<Node> livingNodes = new ArrayList<>(Math.max(0, nodeTuple.nodes.size() - blacklist.size())); |
SivagurunathanV
force-pushed
the
master
branch
from
3bb5e22
to
b5625fd
Compare
|
Added test case. |
|
@elasticmachine ok to test |
|
Hi @dakrone "org.elasticsearch.transport.ConnectTransportException: [node_sm1][127.0.0.1:37453] connect_exception I would appreciate if can you help to debug on this or any point of reference? Thanks, |
|
@dakrone @cbuescher |
|
Merged this, thanks for working through this @SivagurunathanV! |
|
Thanks @dakrone & @cbuescher for guiding me in my first PR 👍. Cheers. |
* elastic/master: (68 commits) Fix potential IllegalCapacityException in LLRC when selecting nodes (elastic#37821) Mute CcrRepositoryIT#testFollowerMappingIsUpdated Fix S3 Repository ITs When Docker is not Available (elastic#37878) Pass distribution type through to docs tests (elastic#37885) Mute SharedClusterSnapshotRestoreIT#testSnapshotCanceledOnRemovedShard SQL: Fix casting from date to numeric type to use millis (elastic#37869) Migrate o.e.i.r.RecoveryState to Writeable (elastic#37380) ML: removing unnecessary upgrade code (elastic#37879) Relax cluster metadata version check (elastic#37834) Mute TransformIntegrationTests#testSearchTransform Refactored GeoHashGrid unit tests (elastic#37832) Fixes for a few randomized agg tests that fail hasValue() checks Geo: replace intermediate geo objects with libs/geo (elastic#37721) Remove NOREPLACE for /etc/elasticsearch in rpm and deb (elastic#37839) Remove "reinstall" packaging tests (elastic#37851) Add unit tests for ShardStateAction's ShardStartedClusterStateTaskExecutor (elastic#37756) Exit batch files explictly using ERRORLEVEL (elastic#29583) TransportUnfollowAction should increase settings version (elastic#37859) AsyncTwoPhaseIndexerTests race condition fixed (elastic#37830) Do not allow negative variances (elastic#37384) ...
Handling LLRC when live node size < 0