absurdfarce commented on PR #2027:
URL:
https://github.com/apache/cassandra-java-driver/pull/2027#issuecomment-3026542119
So I can demonstrate pretty conclusively that without this patch we are
getting a non-empty list for preferred remote DCs in the default case:
```java
@Test
public void default_preferred_dcs_should_be_empty() {
CqlSession session = new CqlSessionBuilder().build();
DriverExecutionProfile profile =
session.getContext().getConfig().getDefaultProfile();
assertThat(profile.getStringList(DefaultDriverOption.LOAD_BALANCING_DC_FAILOVER_PREFERRED_REMOTE_DCS)).isEmpty();
}
```
```
java.lang.AssertionError:
Expecting empty but was: [""]
```
The fix provided by @jahstreet in this PR does indeed make this problem go
away (as expected).
That said, I agree with @jahstreet that it's not immediately clear to me
that there's a problem here. The fact that the preferred DC list is always
non-empty puts us into buildRemoteQueryPlanPreferred()
[here](https://github.com/apache/cassandra-java-driver/blob/4.19.0/core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/BasicLoadBalancingPolicy.java#L333-L336)
which means that we'll prefer nodes from a DC whose name is the empty string
first in this method. But we won't find any nodes in that DC so... we
basically wind up looping through the existing nodes anyway, which leaves us
with something very much like what we'd get out of buildRemoteQueryPlanAll().
All of _that_ said, the analysis above is entirely dependent on the current
impl of these functions; that could easily change and a change in
buildRemoteQueryPlanPreferred() might cause unexpected side effects if it's
still being used when we don't expect it to be. I agree with @jahstreet that
this is hardly the most significant problem we're facing but it seems to me
like (a) we do have a pretty clear error in the code in this PR and (b) we have
a very simple fix (in the form of this PR) for it. So my inclination is to say
let's merge it.
@netudima is right to ask about a test for this case but honestly I'm not
sure how we'd do that. You could try a variation on
[BasicLoadBalancingPolicyPreferredRemoteDcsTest](https://github.com/apache/cassandra-java-driver/blob/4.19.0/core/src/test/java/com/datastax/oss/driver/internal/core/loadbalancing/BasicLoadBalancingPolicyPreferredRemoteDcsTest.java)
with a different impl of
[createAndInitPolicy()](https://github.com/apache/cassandra-java-driver/blob/4.19.0/core/src/test/java/com/datastax/oss/driver/internal/core/loadbalancing/BasicLoadBalancingPolicyPreferredRemoteDcsTest.java#L125)
which sets up the preferred remote DCs list to be either empty or (probably
better) equal to the default. But here you hit a problem based on the
behaviour mentioned above; because buildRemoteQueryPlanPreferred() and
buildRemoteQueryPlanAll() should produce similar results here there's no way to
tell which function was actually used. I suppose there's an argument that it
doesn't matter; our o
nly interest is in making sure that the expected value is returned in this
case and we don't care which of those two functions actually produces it... as
long as we can confirm we get the right output. I guess I'd be okay with
that... but it seems like we'd need a pretty good comment (at least) saying
what the pitfall is and why we're doing it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
