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: [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]
