Skip to content

Commit 0bf9092

Browse files
committed
fix: address Copilot review comments on DRIVER-368
- ScassandraCluster: compute hostIdByNodeCount once as an instance field in the constructor so all primeMetadata() calls (init and restart) use stable, consistent host_id UUIDs across system.local and system.peers rows for the same logical node. - ControlConnection.intersectWithNeeded(): return null instead of an empty set when no server columns overlap the needed set, keeping the cache in the uninitialised sentinel state and preventing buildProjectedQuery() from generating an invalid empty-column SELECT. - ControlConnectionUnitTest: update empty-intersection tests to assert null (matching new contract); replace String.contains() assertions in testBuildProjectedQueryAllColumnsPresent() with exact column-list parsing via extractSelectedColumns() to avoid false positives when one column name is a substring of another (e.g. native_port vs native_transport_port).
1 parent 6395600 commit 0bf9092

3 files changed

Lines changed: 53 additions & 20 deletions

File tree

driver-core/src/main/java/com/datastax/driver/core/ControlConnection.java

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -932,8 +932,10 @@ public void onFailure(Throwable t) {
932932

933933
/**
934934
* Returns the intersection of the columns returned by the server (from {@code rs}) with the given
935-
* {@code needed} set. The result is used to cache projected column lists so subsequent queries
936-
* fetch only what the driver actually reads.
935+
* {@code needed} set, or {@code null} if the intersection is empty. The result is used to cache
936+
* projected column lists so subsequent queries fetch only what the driver actually reads. A
937+
* {@code null} return keeps the cache in the "uninitialized" sentinel state, ensuring the driver
938+
* continues issuing {@code SELECT *} rather than generating an invalid empty-column projection.
937939
*/
938940
@VisibleForTesting
939941
static Set<String> intersectWithNeeded(ResultSet rs, ImmutableSet<String> needed) {
@@ -943,7 +945,8 @@ static Set<String> intersectWithNeeded(ResultSet rs, ImmutableSet<String> needed
943945
result.add(def.getName());
944946
}
945947
}
946-
return result.build();
948+
ImmutableSet<String> built = result.build();
949+
return built.isEmpty() ? null : built;
947950
}
948951

949952
/**

driver-core/src/test/java/com/datastax/driver/core/ControlConnectionUnitTest.java

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -175,23 +175,26 @@ public void testIntersectWithNeededHandlesSubset() {
175175
}
176176

177177
@Test(groups = "unit")
178-
public void testIntersectWithNeededNoOverlapReturnsEmpty() {
178+
public void testIntersectWithNeededNoOverlapReturnsNull() {
179+
// When no server columns match the needed set, the result should be null so the cache remains
180+
// in the uninitialized sentinel state (avoids generating an empty-column SELECT projection).
179181
ImmutableSet<String> needed = ImmutableSet.of("cluster_name", "tokens");
180182
ResultSet rs = mockResultSetWithColumns("some_other_col", "another_col");
181183

182184
Set<String> result = ControlConnection.intersectWithNeeded(rs, needed);
183185

184-
assertThat(result).isEmpty();
186+
assertThat(result).isNull();
185187
}
186188

187189
@Test(groups = "unit")
188-
public void testIntersectWithNeededEmptyResultSet() {
190+
public void testIntersectWithNeededEmptyResultSetReturnsNull() {
191+
// An empty ResultSet has no column definitions, so the intersection is empty → null.
189192
ImmutableSet<String> needed = ControlConnection.LOCAL_COLUMNS_OF_INTEREST;
190193
ResultSet rs = mockResultSetWithColumns();
191194

192195
Set<String> result = ControlConnection.intersectWithNeeded(rs, needed);
193196

194-
assertThat(result).isEmpty();
197+
assertThat(result).isNull();
195198
}
196199

197200
// ---------------------------------------------------------------------------
@@ -235,17 +238,37 @@ public void testBuildProjectedQuerySingleColumn() {
235238

236239
@Test(groups = "unit")
237240
public void testBuildProjectedQueryAllColumnsPresent() {
238-
// Every column in the needed set appears somewhere in the resulting query
241+
// Every column in the needed set must appear as an exact identifier in the projected SELECT
242+
// list. Use exact parsing to avoid false positives where one column name is a substring of
243+
// another (e.g. "native_port" inside "native_transport_port").
239244
Set<String> columns = ControlConnection.PEERS_COLUMNS_OF_INTEREST;
240245
String query = ControlConnection.buildProjectedQuery("system.peers", columns, null);
246+
Set<String> selectedColumns = extractSelectedColumns(query);
241247

242248
for (String col : columns) {
243-
assertThat(query).as("query should contain column: " + col).contains(col);
249+
assertThat(selectedColumns).as("query should project column: " + col).contains(col);
244250
}
245251
assertThat(query).contains(" FROM system.peers");
246252
assertThat(query).doesNotContain("WHERE");
247253
}
248254

255+
/**
256+
* Parses the column identifiers from the {@code SELECT col1, col2, ... FROM ...} portion of a
257+
* projected query string and returns them as a set of trimmed names.
258+
*/
259+
private Set<String> extractSelectedColumns(String query) {
260+
int selectStart = query.indexOf("SELECT ");
261+
int fromStart = query.indexOf(" FROM ");
262+
assertThat(selectStart).as("query should start with SELECT").isEqualTo(0);
263+
assertThat(fromStart).as("query should contain FROM").isGreaterThan(selectStart);
264+
String columnList = query.substring("SELECT ".length(), fromStart);
265+
ImmutableSet.Builder<String> builder = ImmutableSet.builder();
266+
for (String col : columnList.split(",")) {
267+
builder.add(col.trim());
268+
}
269+
return builder.build();
270+
}
271+
249272
// ---------------------------------------------------------------------------
250273
// Cache fields: declared as volatile, private, instance-level Set
251274
// ---------------------------------------------------------------------------

driver-core/src/test/java/com/datastax/driver/core/ScassandraCluster.java

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,13 @@ public class ScassandraCluster {
8282

8383
private final boolean peersV2;
8484

85+
/**
86+
* One stable UUID per node (keyed by 1-based nodeCount), computed once at construction so that
87+
* system.local and system.peers rows for the same node always carry the same host_id regardless
88+
* of which Scassandra process is being primed or how many times primeMetadata() is called.
89+
*/
90+
private final Map<Integer, java.util.UUID> hostIdByNodeCount;
91+
8592
ScassandraCluster(
8693
Integer[] nodes,
8794
String ipPrefix,
@@ -148,6 +155,17 @@ public class ScassandraCluster {
148155
instances = instanceListBuilder.build();
149156
dcNodeMap = dcNodeMapBuilder.build();
150157

158+
// Compute stable host_id UUIDs once so every primeMetadata() call uses the same values.
159+
Map<Integer, java.util.UUID> hostIds = new HashMap<>();
160+
int tempCount = 1;
161+
for (Integer dc : new TreeSet<Integer>(dcNodeMap.keySet())) {
162+
for (int n = 0; n < dcNodeMap.get(dc).size(); n++) {
163+
hostIds.put(tempCount, UUIDs.random());
164+
tempCount++;
165+
}
166+
}
167+
this.hostIdByNodeCount = hostIds;
168+
151169
// Prime correct keyspace table based on C* version.
152170
String[] versionArray = this.cassandraVersion.split("\\.|-");
153171
double major = Double.parseDouble(versionArray[0] + "." + versionArray[1]);
@@ -392,17 +410,6 @@ public List<Long> getTokensForDC(int dc) {
392410
private void primeMetadata(Scassandra node) {
393411
PrimingClient client = node.primingClient();
394412

395-
// Pre-generate one UUID per node so that system.local and system.peers rows for the same node
396-
// use the same host_id. Keys are nodeCount values (1-based).
397-
Map<Integer, java.util.UUID> hostIdByNodeCount = new HashMap<>();
398-
int tempCount = 1;
399-
for (Integer dc : new TreeSet<Integer>(dcNodeMap.keySet())) {
400-
for (int n = 0; n < dcNodeMap.get(dc).size(); n++) {
401-
hostIdByNodeCount.put(tempCount, UUIDs.random());
402-
tempCount++;
403-
}
404-
}
405-
406413
int nodeCount = 1;
407414

408415
ImmutableList.Builder<Map<String, ?>> rows = ImmutableList.builder();

0 commit comments

Comments
 (0)