You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "frankgh (via GitHub)" <gi...@apache.org> on 2024/01/02 17:16:35 UTC
Re: [PR] CASSANDRASC-90 Token-ranges endpoint fix to unwrap token-ranges by th… [cassandra-sidecar]
frankgh commented on code in PR #87:
URL: https://github.com/apache/cassandra-sidecar/pull/87#discussion_r1439645641
##########
src/test/integration/org/apache/cassandra/sidecar/routes/tokenrange/BaseTokenRangeIntegrationTest.java:
##########
@@ -291,4 +294,17 @@ void assertMappingResponseOK(TokenRangeReplicasResponse mappingResponse,
.isGreaterThanOrEqualTo(replicationFactor);
}
}
+
+ private void validateRanges(List<TokenRangeReplicasResponse.ReplicaInfo> replicaRanges)
+ {
+ // Ranges should not be empty
+ replicaRanges.stream().forEach(r -> assertThat(r.start()).isNotEqualTo(r.end()));
Review Comment:
nit:
```suggestion
replicaRanges.forEach(r -> assertThat(r.start()).isNotEqualTo(r.end()));
```
##########
adapters/base/src/main/java/org/apache/cassandra/sidecar/adapters/base/TokenRangeReplicas.java:
##########
@@ -219,7 +219,7 @@ public static List<TokenRangeReplicas> normalize(List<TokenRangeReplicas> ranges
if (ranges.stream().noneMatch(r -> r.partitioner.minToken.compareTo(r.start()) == 0))
{
- LOGGER.warn("{} based minToken does not exist in the token ranges", Partitioner.class.getName());
+ LOGGER.warn("Partitioner based minToken does not exist in the token ranges");
Review Comment:
ah, it looks like this method is called from other places as well..
##########
src/test/integration/org/apache/cassandra/sidecar/routes/tokenrange/BaseTokenRangeIntegrationTest.java:
##########
@@ -291,4 +294,17 @@ void assertMappingResponseOK(TokenRangeReplicasResponse mappingResponse,
.isGreaterThanOrEqualTo(replicationFactor);
}
}
+
+ private void validateRanges(List<TokenRangeReplicasResponse.ReplicaInfo> replicaRanges)
+ {
+ // Ranges should not be empty
+ replicaRanges.stream().forEach(r -> assertThat(r.start()).isNotEqualTo(r.end()));
+ // Ranges should include partitioner start and end
+ replicaRanges.stream()
+ .map(TokenRangeReplicasResponse.ReplicaInfo::start)
+ .anyMatch(s -> s.equals(Murmur3Partitioner.MINIMUM.toString()));
+ replicaRanges.stream()
+ .map(TokenRangeReplicasResponse.ReplicaInfo::end)
+ .anyMatch(s -> s.equals(Long.toString(Murmur3Partitioner.MAXIMUM)));
Review Comment:
should we assert something here?
```suggestion
assertThat(replicaRanges.stream()
.map(TokenRangeReplicasResponse.ReplicaInfo::start)
.anyMatch(s -> s.equals(Murmur3Partitioner.MINIMUM.toString()))).isTrue();
assertThat(replicaRanges.stream()
.map(TokenRangeReplicasResponse.ReplicaInfo::end)
.anyMatch(s -> s.equals(Long.toString(Murmur3Partitioner.MAXIMUM)))).isTrue();
```
##########
adapters/base/src/main/java/org/apache/cassandra/sidecar/adapters/base/TokenRangeReplicas.java:
##########
@@ -219,7 +219,7 @@ public static List<TokenRangeReplicas> normalize(List<TokenRangeReplicas> ranges
if (ranges.stream().noneMatch(r -> r.partitioner.minToken.compareTo(r.start()) == 0))
{
- LOGGER.warn("{} based minToken does not exist in the token ranges", Partitioner.class.getName());
+ LOGGER.warn("Partitioner based minToken does not exist in the token ranges");
Review Comment:
it seems that the intent of the logger was to log the partitioner name ? should we make the method non-static so we have access to the configured `partitioner`?
```
LOGGER.warn("{} based minToken does not exist in the token ranges", partitioner.name());
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org