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 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.

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

Either way this code seems difficult to follow. Can someone suggest a simplification?

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.

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

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?

any.removeAll(preferred) would avoid duplicate attempts even more effectively, and not require explanation

I agree. I just tried to keep the changes minimal and keep the logic as close as possible to the original logic

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 HostRequirements

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?

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

It's important that the HostSpecs' 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

+1. The previous logic with trimming secondaries was invalid.