Skip to content

[fix][client] Fix ArrayIndexOutOfBoundsException when using SameAuthParamsLookupAutoClusterFailover #23336

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Sep 23, 2024

Motivation

There is a mistake in SameAuthParamsLookupAutoClusterFailover.class, which may cause a ArrayIndexOutOfBoundsException, but it affects nothing because it only happens when no server is available.

Modifications

  • Fix the mistake
  • Add an log

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: x

@poorbarcode poorbarcode self-assigned this Sep 23, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 23, 2024
@lhotari
Copy link
Member

lhotari commented Sep 23, 2024

Any chance to add a test case?

@Technoboy- Technoboy- added this to the 4.1.0 milestone Oct 30, 2024
@Technoboy- Technoboy- closed this Oct 30, 2024
@Technoboy- Technoboy- reopened this Oct 30, 2024
@lhotari lhotari changed the title [fix] FIx ArrayIndexOutOfBoundsException when using SameAuthParamsLookupAutoClusterFailover [fix][client] Fix ArrayIndexOutOfBoundsException when using SameAuthParamsLookupAutoClusterFailover Oct 30, 2024
@lhotari
Copy link
Member

lhotari commented Oct 30, 2024

Any chance to add a test case?

@poorbarcode @Technoboy- any chance to handle this?

@@ -123,7 +130,7 @@ private int firstHealthyPulsarService() {
}

private int findFailoverTo() {
for (int i = currentPulsarServiceIndex + 1; i <= pulsarServiceUrlArray.length; i++) {
for (int i = currentPulsarServiceIndex + 1; i < pulsarServiceUrlArray.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering if this logic is correct in the first place. Shouldn't this wrap around the array?

the for loop in checkPulsarServices looks strange too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic of fallover is follows

  • if the current cluster is the highest priority cluster and it is healthy, do nothing.
  • else if there is a healthy and high priority cluster, recover to the higher priority one.
  • else if both current cluster and higher priority clusters are not healthy, find a healthy cluster to fallover to.

So the two method checkPulsarServices and findFailoverTo are for the different cases to same CPU resouces.

  • checkPulsarServices is for checking whether their is a healthy and high priority cluster.
  • findFailoverTo is for checking whether their has a cluster is healthy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@poorbarcode what happens in the case where currentPulsarServiceIndex is already the last one? How can it find a failover cluster in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens in the case where currentPulsarServiceIndex is already the last one? How can it find a failover cluster in that case?

It will transfer to the first healthy cluster. Once encounters the ArrayIndexOutOfBoundsException error, it means no cluster is healthy.

The issue the correct PR solved affects nothing

@lhotari
Copy link
Member

lhotari commented Nov 29, 2024

@poorbarcode Do you have a chance to follow up on the review comment?

@poorbarcode poorbarcode requested a review from lhotari December 2, 2024 01:47
@poorbarcode
Copy link
Contributor Author

@poorbarcode Do you have a chance to follow up on the review comment?

Answered here: #23336 (comment)

@poorbarcode poorbarcode force-pushed the fix/failover_out_of_array branch from 8057c39 to 471669d Compare December 2, 2024 02:19
@poorbarcode
Copy link
Contributor Author

Rebase master

@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 55.55556% with 8 lines in your changes missing coverage. Please review.

Project coverage is 74.37%. Comparing base (bbc6224) to head (471669d).
Report is 772 commits behind head on master.

Files with missing lines Patch % Lines
.../impl/SameAuthParamsLookupAutoClusterFailover.java 55.55% 6 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23336      +/-   ##
============================================
+ Coverage     73.57%   74.37%   +0.80%     
- Complexity    32624    35036    +2412     
============================================
  Files          1877     1944      +67     
  Lines        139502   147334    +7832     
  Branches      15299    16258     +959     
============================================
+ Hits         102638   109583    +6945     
- Misses        28908    29286     +378     
- Partials       7956     8465     +509     
Flag Coverage Δ
inttests 27.29% <0.00%> (+2.70%) ⬆️
systests 24.43% <0.00%> (+0.11%) ⬆️
unittests 73.76% <55.55%> (+0.91%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
.../impl/SameAuthParamsLookupAutoClusterFailover.java 68.29% <55.55%> (ø)

... and 664 files with indirect coverage changes

@poorbarcode poorbarcode requested review from lhotari and removed request for lhotari December 3, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants