yifan-c commented on code in PR #45: URL: https://github.com/apache/cassandra-sidecar/pull/45#discussion_r1192846428
########## cassandra40/src/main/java/org/apache/cassandra/sidecar/cassandra40/TokenRangeReplicas.java: ########## @@ -0,0 +1,415 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.cassandra40; + +import java.math.BigInteger; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.Iterator; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import com.google.common.base.Objects; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.jetbrains.annotations.NotNull; + + +/** + * Representation of a token range and the corresponding mapping to replica-set hosts + */ +public class TokenRangeReplicas implements Comparable<TokenRangeReplicas> +{ + private final BigInteger start; + private final BigInteger end; + + private final Partitioner partitioner; + + private final Set<String> replicaSet; + + private static final Logger LOGGER = LoggerFactory.getLogger(TokenRangeReplicas.class); + + public TokenRangeReplicas(BigInteger start, BigInteger end, Partitioner partitioner, Set<String> replicaSet) + { + this.start = start; + this.end = end; + this.partitioner = partitioner; + this.replicaSet = replicaSet; + } + + + public BigInteger start() + { + return start; + } + + public BigInteger end() + { + return end; + } + + public Set<String> replicaSet() + { + return replicaSet; + } + + /** + * {@inheritDoc} + */ + @Override + public int compareTo(@NotNull TokenRangeReplicas other) + { + if (this.partitioner != other.partitioner) + throw new IllegalStateException("Token ranges being compared do not have the same partitioner"); + + BigInteger maxValue = this.partitioner.maxToken; + if (this.start.compareTo(other.start) == 0) + { + if (this.end.equals(maxValue)) return 1; + else if (other.end.equals(maxValue)) return -1; + else return this.end.compareTo(other.end); + } + else return this.start.compareTo(other.start); + } + + /** + * {@inheritDoc} + */ + @Override + public boolean equals(Object o) + { + if (!(o instanceof TokenRangeReplicas)) + { + return false; + } + TokenRangeReplicas that = (TokenRangeReplicas) o; + return (this.start.equals(that.start) && this.end.equals(that.end) && this.partitioner == that.partitioner); + } + + /** + * {@inheritDoc} + */ + @Override + public int hashCode() + { + return Objects.hashCode(start, end, partitioner); + } + + private boolean isWrapAround() + { + return start.compareTo(end) >= 0; + } + + private boolean isSubsetOf(TokenRangeReplicas other) + { + if (this.partitioner != other.partitioner) + throw new IllegalStateException("Token ranges being compared do not have the same partitioner"); + + BigInteger maxValue = this.partitioner.maxToken; + if (this.start.compareTo(other.start) >= 0) + { + if (other.end.equals(maxValue)) return true; + if (this.end.equals(maxValue)) return false; + if (this.end.compareTo(other.end) <= 0) return true; + } + return false; + } + + /** + * For subset ranges, this is used to determine if a range is larger than the other by comparing start-end lengths + * If both ranges end at the min, we compare starting points to determine the result. + * When the left range is the only one ending at min, it is always the larger one since all subsequent ranges + * in the sorted range list have to be smaller. + * + * @param other the next range in the range list to compare + * @return true if "this" range is larger than the other + */ + private boolean isLarger(TokenRangeReplicas other) Review Comment: 1. Please write unit tests for this method. 2. What qualifies a range to be larger? I am still confused after reading the java doc. The first sentence says it compares the lengths, which makes sense. But the later sentences seem to indicate that it also depends on the position of the ranges. ########## client/src/main/java/org/apache/cassandra/sidecar/client/SidecarInstanceImpl.java: ########## @@ -0,0 +1,105 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.client; + +import java.util.Objects; + +/** + * A simple implementation of the {@link SidecarInstance} interface + */ +public class SidecarInstanceImpl implements SidecarInstance +{ + protected int port; + protected String hostname; + + /** + * Constructs a new Sidecar instance with the given {@code port} and {@code hostname} + * + * @param hostname the host name where Sidecar is running + * @param port the port where Sidecar is running + */ + public SidecarInstanceImpl(String hostname, int port) + { + if (port < 1 || port > 65535) + { + throw new IllegalArgumentException(String.format("Invalid port number for the Sidecar service: %d", + port)); + } + this.port = port; + this.hostname = Objects.requireNonNull(hostname, "The Sidecar hostname must be non-null"); + } + + /** + * {@inheritDoc} + */ + @Override + public int port() + { + return port; + } + + /** + * {@inheritDoc} + */ + @Override + public String hostname() + { + return hostname; + } + + /** + * {@inheritDoc} + */ + @Override + Review Comment: remove the empty line. ########## cassandra40/src/main/java/org/apache/cassandra/sidecar/cassandra40/TokenRangeReplicas.java: ########## @@ -0,0 +1,415 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.cassandra40; + +import java.math.BigInteger; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.Iterator; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import com.google.common.base.Objects; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.jetbrains.annotations.NotNull; + + +/** + * Representation of a token range and the corresponding mapping to replica-set hosts Review Comment: add a sentence to explain which end of the range is exclusive. I would assume `(start, end]` ########## client/src/main/java/org/apache/cassandra/sidecar/client/RequestContext.java: ########## @@ -0,0 +1,455 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.client; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import org.apache.cassandra.sidecar.client.request.CleanSSTableUploadSessionRequest; +import org.apache.cassandra.sidecar.client.request.ClearSnapshotRequest; +import org.apache.cassandra.sidecar.client.request.CreateSnapshotRequest; +import org.apache.cassandra.sidecar.client.request.GossipInfoRequest; +import org.apache.cassandra.sidecar.client.request.ImportSSTableRequest; +import org.apache.cassandra.sidecar.client.request.ListSnapshotFilesRequest; +import org.apache.cassandra.sidecar.client.request.NodeSettingsRequest; +import org.apache.cassandra.sidecar.client.request.Request; +import org.apache.cassandra.sidecar.client.request.RingRequest; +import org.apache.cassandra.sidecar.client.request.SSTableComponentRequest; +import org.apache.cassandra.sidecar.client.request.SchemaRequest; +import org.apache.cassandra.sidecar.client.request.TimeSkewRequest; +import org.apache.cassandra.sidecar.client.request.TokenRangeReplicasRequest; +import org.apache.cassandra.sidecar.client.request.UploadSSTableRequest; +import org.apache.cassandra.sidecar.client.retry.ExponentialBackoffRetryPolicy; +import org.apache.cassandra.sidecar.client.retry.NoRetryPolicy; +import org.apache.cassandra.sidecar.client.retry.RetryPolicy; +import org.apache.cassandra.sidecar.client.selection.InstanceSelectionPolicy; +import org.apache.cassandra.sidecar.client.selection.SingleInstanceSelectionPolicy; +import org.apache.cassandra.sidecar.common.utils.HttpRange; + +/** + * The context for a given request that include the {@link InstanceSelectionPolicy}, the {@link RetryPolicy}, and + * the {@link Request} + */ +public class RequestContext +{ + protected static final SchemaRequest FULL_SCHEMA_REQUEST = new SchemaRequest(); + protected static final TimeSkewRequest TIME_SKEW_REQUEST = new TimeSkewRequest(); + protected static final NodeSettingsRequest NODE_SETTINGS_REQUEST = new NodeSettingsRequest(); + protected static final RingRequest RING_REQUEST = new RingRequest(); Review Comment: nit: the request objects, as well as the request builder methods below, do not feel should part of `RequestContext`, which supplies the context for a request. But I think it is fine to leave them as is for now. ########## cassandra40/src/main/java/org/apache/cassandra/sidecar/cassandra40/TokenRangeReplicas.java: ########## @@ -0,0 +1,415 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.cassandra40; + +import java.math.BigInteger; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.Iterator; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import com.google.common.base.Objects; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.jetbrains.annotations.NotNull; + + +/** + * Representation of a token range and the corresponding mapping to replica-set hosts + */ +public class TokenRangeReplicas implements Comparable<TokenRangeReplicas> +{ + private final BigInteger start; + private final BigInteger end; + + private final Partitioner partitioner; + + private final Set<String> replicaSet; + + private static final Logger LOGGER = LoggerFactory.getLogger(TokenRangeReplicas.class); + + public TokenRangeReplicas(BigInteger start, BigInteger end, Partitioner partitioner, Set<String> replicaSet) + { + this.start = start; + this.end = end; + this.partitioner = partitioner; + this.replicaSet = replicaSet; + } + + + public BigInteger start() + { + return start; + } + + public BigInteger end() + { + return end; + } + + public Set<String> replicaSet() + { + return replicaSet; + } + + /** + * {@inheritDoc} + */ + @Override + public int compareTo(@NotNull TokenRangeReplicas other) + { + if (this.partitioner != other.partitioner) + throw new IllegalStateException("Token ranges being compared do not have the same partitioner"); + + BigInteger maxValue = this.partitioner.maxToken; + if (this.start.compareTo(other.start) == 0) + { + if (this.end.equals(maxValue)) return 1; + else if (other.end.equals(maxValue)) return -1; + else return this.end.compareTo(other.end); + } + else return this.start.compareTo(other.start); + } + + /** + * {@inheritDoc} + */ + @Override + public boolean equals(Object o) + { + if (!(o instanceof TokenRangeReplicas)) + { + return false; + } + TokenRangeReplicas that = (TokenRangeReplicas) o; + return (this.start.equals(that.start) && this.end.equals(that.end) && this.partitioner == that.partitioner); + } + + /** + * {@inheritDoc} + */ + @Override + public int hashCode() + { + return Objects.hashCode(start, end, partitioner); + } + + private boolean isWrapAround() + { + return start.compareTo(end) >= 0; + } + + private boolean isSubsetOf(TokenRangeReplicas other) Review Comment: Does it check if this range is entirely covered by the other range? In other word, the `other` range contains `this` range. If so, I think the implementation is wrong. See test below ```java @Test void testSubsetRelationship() { // wraps around TokenRangeReplicas range1 = new TokenRangeReplicas(BigInteger.valueOf(100), BigInteger.valueOf(-100), Partitioner.Murmur3, new HashSet<>()); TokenRangeReplicas range2 = new TokenRangeReplicas(BigInteger.valueOf(100), Partitioner.Murmur3.maxToken, Partitioner.Murmur3, new HashSet<>()); assertThat(range1.isSubsetOf(range2)).isFalse(); assertThat(range2.isSubsetOf(range1)).isTrue(); } ``` nit: the name could be more succinct if it is called "contains". The implementation needs to be adjusted if being renamed. ########## client/src/main/java/org/apache/cassandra/sidecar/client/SidecarConfig.java: ########## @@ -0,0 +1,125 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.cassandra.sidecar.client; + +/** + * Encapsulates configurations for the {@link SidecarClient} + */ +public class SidecarConfig +{ + private static final int DEFAULT_MAX_RETRIES = 3; + private static final long DEFAULT_RETRY_DELAY_MILLIS = 500L; + private static final long DEFAULT_MAX_RETRY_DELAY_MILLIS = 60_000L; + + private final int maxRetries; + private final long retryDelayMillis; + private final long maxRetryDelayMillis; + + private SidecarConfig(Builder builder) + { + maxRetries = builder.maxRetries; + retryDelayMillis = builder.retryDelayMillis; + maxRetryDelayMillis = builder.maxRetryDelayMillis; + } + + /** + * @return the maximum number of times to retry a request + */ + public int maxRetries() + { + return maxRetries; + } + + /** + * @return the initial amount of time to wait before retrying a failed request + */ + public long retryDelayMillis() + { + return retryDelayMillis; + } + + /** + * @return the maximum amount of time to wait before retrying a failed request + */ + public long maxRetryDelayMillis() + { + return maxRetryDelayMillis; + } + + /** + * {@code SidecarConfig} builder static inner class. + */ + public static final class Builder + { + private int maxRetries = DEFAULT_MAX_RETRIES; + private long retryDelayMillis = DEFAULT_RETRY_DELAY_MILLIS; + private long maxRetryDelayMillis = DEFAULT_MAX_RETRY_DELAY_MILLIS; + + public Builder() + { + } Review Comment: It can be removed. ########## checkstyle.xml: ########## @@ -139,6 +139,7 @@ page at http://checkstyle.sourceforge.net/config.html --> <module name="JavadocType"> <property name="scope" value="protected" /> + <property name="allowUnknownTags" value="true" /> Review Comment: 👍 -- 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]
