-
Notifications
You must be signed in to change notification settings - Fork 845
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
feat: targetServerType=preferPrimary connection parameter #2483
Conversation
return any.iterator(); | ||
} | ||
|
||
if (any.isEmpty()) { | ||
return secondaries.iterator(); | ||
return preferred.iterator(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the logic here ?
if preferred is empty fails and any is empty is true, then it's going to return an empty iterator ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If preferred
is not empty, then neither can any
be.
I'm guessing a static analysis would flag the any.get(0)
without checking as a bug.
An assert !any.isEmpty()
should suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the logic here ?
if preferred is empty fails and any is empty is true, then it's going to return an empty iterator ?
if preferred-is-empty fails (i.e. "preferred" iterator is not empty) and any-is-empty is true (i.e. "any" iterator is empty), then it's going to return "preferred" iterator, which is not empty. It follows exactly same logic for the "preferred" iterator that was here before for the "secondaries" iterator.
I agree with @OrangeDog though that if "preferred" is not empty, then "any" cannot be empty either, which means that "any-is-empty is true" branch will never be entered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way this code seems difficult to follow. Can someone suggest a simplification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the checks are actually necessary:
List<CandidateHost> preferred = getCandidateHosts(preferredServerType);
List<CandidateHost> any = getCandidateHosts(HostRequirement.any);
any.removeAll(preferred);
return append(preferred, any).iterator();
May need a different function call if the lists aren't mutable. Or skip removeAll
and don't worry about repeated attempts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way this code seems difficult to follow. Can someone suggest a simplification?
Pushed 404efd1 commit that simplifies MultiHostChooser.candidateIterator()
code and fixes condition for the optimization inside this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way this code seems difficult to follow. Can someone suggest a simplification?
Pushed 404efd1 commit that simplifies
MultiHostChooser.candidateIterator()
code and fixes condition for the optimization inside this method
@davecramer - can I do anything else to address your concern that the code in question is difficult to follow and your request for its simplification?
// Note: this is only an optimization | ||
secondaries = rtrim(1, secondaries); | ||
preferred = rtrim(1, preferred); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any.removeAll(preferred)
would avoid duplicate attempts even more effectively, and not require explanation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I just tried to keep the changes minimal and keep the logic as close as possible to the original logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any.removeAll(preferred)
would avoid duplicate attempts even more effectively, and not require explanation
After further consideration I think that any.removeAll(preferred)
won't work with CandidateHost
class instances because it won't find a match in any
for any instance from preferred
and, even if implemented properly (i.e. against HostSpec
instances instead of CandidateHost
), then it will break the logic implemented here originally for the host selection.
None of the checks are actually necessary:
List<CandidateHost> preferred = getCandidateHosts(preferredServerType); List<CandidateHost> any = getCandidateHosts(HostRequirement.any); any.removeAll(preferred); return append(preferred, any).iterator();
May need a different function call if the lists aren't mutable. Or skip
removeAll
and don't worry about repeated attempts.
Those repeated attempts are actually important here since they are made for different HostRequirement
s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't work with
CandidateHost
class instances
Ah yes. I missed the .hostSpec
in the original. Removing all of them would need a bit more code.
Those repeated attempts are actually important
Then why is it removing one of them as an optimisation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then why is it removing one of them as an optimisation?
It is removing just this one for optimization because it would check this host two times in a row, first time for a narrower requirement/condition (e.g. secondary
) and, if the first check fails, it checks it right away for the broader any
requirement/condition. These two checks in a row don't make too much sense so it removes the check for the narrower condition and makes just one check for the broader any
condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's important that the HostSpec
s' order in each part of the returned List<CandidateHost>
result (i.e. the first preferred
part and the last any
part) is the same as the order of host specs in the jdbc url. What's also important is that those preferred
and any
parts are shuffled independently, when loadBalanceHosts=true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. The previous logic with trimming secondaries
was invalid.
… optimization in there
I fully support
@davecramer , @mitya555 , WDYT? |
@vlsi no, that doesn't make much sense. It already always gets all the matching hosts, and already always puts them first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Not sure how the changelog is managed, but this ended up in "Fixed" instead of "Added". |
@davecramer - this PR is a cleaner copy of the PR#1430 as we discussed here: https://www.postgresql.org/message-id/CADK3HHJBQJje9Etd29HY6_1S1kRdR4KEviPev-vjtrQp-dZ%2BFQ%40mail.gmail.com
Below is the original PR#1430 Description:
All Submissions:
New Feature Submissions:
./gradlew autostyleCheck checkstyleAll
pass ?Changes to Existing Features: