diff --git a/CHANGES.txt b/CHANGES.txt index 1198a67ea..8f75562a1 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,5 +1,6 @@ 0.5.0 ----- + * Support per-instance Sidecar routing for bulk writes behind load balancers using discovered instance IDs or explicit contact-point IDs (CASSANALYTICS-177) * Fix UDT column deserialization in CDC (CASSANALYTICS-203) * Skip CDC-irrelevant mutations on deserialization failure (CASSANALYTICS-196) * Opening one keyspace instance rebuilds every keyspace instance in the Cassandra 6.0 bridge (CASSANALYTICS-202) diff --git a/analytics-sidecar-client-common/src/main/java/org/apache/cassandra/sidecar/client/SidecarInstance.java b/analytics-sidecar-client-common/src/main/java/org/apache/cassandra/sidecar/client/SidecarInstance.java index 1d2d14829..26b977b02 100644 --- a/analytics-sidecar-client-common/src/main/java/org/apache/cassandra/sidecar/client/SidecarInstance.java +++ b/analytics-sidecar-client-common/src/main/java/org/apache/cassandra/sidecar/client/SidecarInstance.java @@ -32,4 +32,20 @@ public interface SidecarInstance * @return the hostname where the Cassandra Sidecar instance is running */ String hostname(); + + /** + * Returns the identifier of the specific Cassandra instance that requests sent to this Sidecar + * endpoint should be routed to, or {@code null} when no per-instance identifier is configured. + * + *

When non-null, this value is used to populate the {@code instanceId} query parameter on outbound + * requests so the Sidecar can resolve the correct local Cassandra instance even when a shared address + * (for example a load balancer) hides the real target from the {@code Host} header. When {@code null}, + * the client falls back to the job-level {@code instanceId} configured on the HTTP client, if any. + * + * @return the per-instance identifier, or {@code null} when not set + */ + default Integer instanceId() + { + return null; + } } diff --git a/analytics-sidecar-client-common/src/main/java/org/apache/cassandra/sidecar/client/SidecarInstanceImpl.java b/analytics-sidecar-client-common/src/main/java/org/apache/cassandra/sidecar/client/SidecarInstanceImpl.java index 751218b16..f3bbf3b1b 100644 --- a/analytics-sidecar-client-common/src/main/java/org/apache/cassandra/sidecar/client/SidecarInstanceImpl.java +++ b/analytics-sidecar-client-common/src/main/java/org/apache/cassandra/sidecar/client/SidecarInstanceImpl.java @@ -27,22 +27,45 @@ public class SidecarInstanceImpl implements SidecarInstance { protected int port; protected String hostname; + protected Integer instanceId; /** - * Constructs a new Sidecar instance with the given {@code port} and {@code hostname} + * Constructs a new Sidecar instance with the given {@code port} and {@code hostname} and no + * per-instance identifier (requests fall back to the job-level {@code instanceId}, if any). * * @param hostname the host name where Sidecar is running * @param port the port where Sidecar is running */ public SidecarInstanceImpl(String hostname, int port) + { + this(hostname, port, null); + } + + /** + * Constructs a new Sidecar instance with the given {@code hostname}, {@code port} and per-instance + * {@code instanceId}. + * + * @param hostname the host name where Sidecar is running + * @param port the port where Sidecar is running + * @param instanceId the identifier of the Cassandra instance that requests sent to this Sidecar + * endpoint should be routed to, or {@code null} to fall back to the job-level + * {@code instanceId} + */ + public SidecarInstanceImpl(String hostname, int port, Integer instanceId) { if (port < 1 || port > 65535) { throw new IllegalArgumentException(String.format("Invalid port number for the Sidecar service: %d", port)); } + if (instanceId != null && instanceId < 0) + { + throw new IllegalArgumentException(String.format("Invalid instanceId for the Sidecar service: %d", + instanceId)); + } this.port = port; this.hostname = Objects.requireNonNull(hostname, "The Sidecar hostname must be non-null"); + this.instanceId = instanceId; } /** @@ -63,6 +86,15 @@ public String hostname() return hostname; } + /** + * {@inheritDoc} + */ + @Override + public Integer instanceId() + { + return instanceId; + } + /** * {@inheritDoc} */ @@ -78,7 +110,7 @@ public boolean equals(Object o) return false; } SidecarInstanceImpl that = (SidecarInstanceImpl) o; - return port == that.port && Objects.equals(hostname, that.hostname); + return port == that.port && Objects.equals(hostname, that.hostname) && Objects.equals(instanceId, that.instanceId); } /** @@ -87,7 +119,7 @@ public boolean equals(Object o) @Override public int hashCode() { - return Objects.hash(port, hostname); + return Objects.hash(port, hostname, instanceId); } /** @@ -99,6 +131,7 @@ public String toString() return "SidecarInstanceImpl{" + "port=" + port + ", hostname='" + hostname + '\'' + + ", instanceId=" + instanceId + '}'; } } diff --git a/analytics-sidecar-client-common/src/main/java/org/apache/cassandra/sidecar/common/http/SidecarQueryParamNames.java b/analytics-sidecar-client-common/src/main/java/org/apache/cassandra/sidecar/common/http/SidecarQueryParamNames.java new file mode 100644 index 000000000..6b615d261 --- /dev/null +++ b/analytics-sidecar-client-common/src/main/java/org/apache/cassandra/sidecar/common/http/SidecarQueryParamNames.java @@ -0,0 +1,40 @@ +/* + * 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.common.http; + +/** + * Custom query parameter names for sidecar HTTP requests. + */ +public final class SidecarQueryParamNames +{ + /** + * {@code "instanceId"} query parameter. When present on an outbound sidecar request it carries + * the job-level instance identifier supplied by the client (see the Spark conf key + * {@code spark.cassandra_analytics.sidecar.instance.id}). + * + *

Requires a Sidecar server >= 0.2.0 (see {@code AbstractHandler#host}, introduced in + * CASSSIDECAR-208); older servers do not resolve this parameter and requests will fall back to + * Host-header-based instance resolution. + */ + public static final String INSTANCE_ID = "instanceId"; + + private SidecarQueryParamNames() + { + } +} diff --git a/analytics-sidecar-client-common/src/main/java/org/apache/cassandra/sidecar/common/response/TokenRangeReplicasResponse.java b/analytics-sidecar-client-common/src/main/java/org/apache/cassandra/sidecar/common/response/TokenRangeReplicasResponse.java index 3c79c4ce8..eee0878d7 100644 --- a/analytics-sidecar-client-common/src/main/java/org/apache/cassandra/sidecar/common/response/TokenRangeReplicasResponse.java +++ b/analytics-sidecar-client-common/src/main/java/org/apache/cassandra/sidecar/common/response/TokenRangeReplicasResponse.java @@ -24,6 +24,7 @@ import com.fasterxml.jackson.annotation.JsonProperty; import org.apache.cassandra.sidecar.common.request.TokenRangeReplicasRequest; +import org.jetbrains.annotations.Nullable; /** * Class response for the {@link TokenRangeReplicasRequest} @@ -179,13 +180,16 @@ public static class ReplicaMetadata private final String address; private final int port; private final String datacenter; + @Nullable + private final Integer sidecarInstanceId; public ReplicaMetadata(@JsonProperty("state") String state, @JsonProperty("status") String status, @JsonProperty("fqdn") String fqdn, @JsonProperty("address") String address, @JsonProperty("port") int port, - @JsonProperty("datacenter") String datacenter) + @JsonProperty("datacenter") String datacenter, + @JsonProperty("sidecarInstanceId") @Nullable Integer sidecarInstanceId) { this.state = state; this.status = status; @@ -193,6 +197,7 @@ public ReplicaMetadata(@JsonProperty("state") String state, this.address = address; this.port = port; this.datacenter = datacenter; + this.sidecarInstanceId = sidecarInstanceId; } /** @@ -249,6 +254,18 @@ public String datacenter() return datacenter; } + /** + * @return the id of the Sidecar instance that manages this replica, or {@code null} when the Sidecar + * that served the request does not manage this replica. Used to route per-replica requests through a + * load balancer via the {@code instanceId} query parameter. + */ + @JsonProperty("sidecarInstanceId") + @Nullable + public Integer sidecarInstanceId() + { + return sidecarInstanceId; + } + /** * {@inheritDoc} */ @@ -261,6 +278,7 @@ public String toString() ", address='" + address + '\'' + ", port='" + port + '\'' + ", datacenter='" + datacenter + '\'' + + ", sidecarInstanceId=" + sidecarInstanceId + '}'; } } diff --git a/analytics-sidecar-client/src/test/java/org/apache/cassandra/sidecar/client/HttpClientConfigTest.java b/analytics-sidecar-client/src/test/java/org/apache/cassandra/sidecar/client/HttpClientConfigTest.java index a2c1db659..0f0ae41b1 100644 --- a/analytics-sidecar-client/src/test/java/org/apache/cassandra/sidecar/client/HttpClientConfigTest.java +++ b/analytics-sidecar-client/src/test/java/org/apache/cassandra/sidecar/client/HttpClientConfigTest.java @@ -159,4 +159,5 @@ void testCassandraRole() HttpClientConfig config = new HttpClientConfig.Builder<>().cassandraRole("custom_role").build(); assertThat(config.cassandraRole()).isEqualTo("custom_role"); } + } diff --git a/analytics-sidecar-client/src/test/java/org/apache/cassandra/sidecar/client/SidecarInstanceImplTest.java b/analytics-sidecar-client/src/test/java/org/apache/cassandra/sidecar/client/SidecarInstanceImplTest.java index 8307eba75..eac97872e 100644 --- a/analytics-sidecar-client/src/test/java/org/apache/cassandra/sidecar/client/SidecarInstanceImplTest.java +++ b/analytics-sidecar-client/src/test/java/org/apache/cassandra/sidecar/client/SidecarInstanceImplTest.java @@ -18,6 +18,11 @@ package org.apache.cassandra.sidecar.client; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; + /** * Unit tests for the {@link SidecarInstanceImpl} class */ @@ -28,4 +33,39 @@ protected SidecarInstance newInstance(String hostname, int port) { return new SidecarInstanceImpl(hostname, port); } + + @Test + void testInstanceIdDefaultsToNull() + { + assertThat(new SidecarInstanceImpl("localhost", 8080).instanceId()).isNull(); + } + + @Test + void testInstanceIdIsRetained() + { + assertThat(new SidecarInstanceImpl("localhost", 8080, 2).instanceId()).isEqualTo(2); + assertThat(new SidecarInstanceImpl("localhost", 8080, 0).instanceId()).isEqualTo(0); + } + + @Test + void testNegativeInstanceIdRejected() + { + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> new SidecarInstanceImpl("localhost", 8080, -1)) + .withMessageContaining("Invalid instanceId for the Sidecar service: -1"); + } + + @Test + void testEqualityDistinguishesInstanceId() + { + SidecarInstance a = new SidecarInstanceImpl("localhost", 8080, 1); + SidecarInstance b = new SidecarInstanceImpl("localhost", 8080, 2); + SidecarInstance c = new SidecarInstanceImpl("localhost", 8080, 1); + SidecarInstance noId = new SidecarInstanceImpl("localhost", 8080); + + assertThat(a).isEqualTo(c); + assertThat(a).hasSameHashCodeAs(c); + assertThat(a).isNotEqualTo(b); + assertThat(a).isNotEqualTo(noId); + } } diff --git a/analytics-sidecar-vertx-client/src/main/java/org/apache/cassandra/sidecar/client/VertxHttpClient.java b/analytics-sidecar-vertx-client/src/main/java/org/apache/cassandra/sidecar/client/VertxHttpClient.java index f4c3890df..c756dfbdc 100644 --- a/analytics-sidecar-vertx-client/src/main/java/org/apache/cassandra/sidecar/client/VertxHttpClient.java +++ b/analytics-sidecar-vertx-client/src/main/java/org/apache/cassandra/sidecar/client/VertxHttpClient.java @@ -58,6 +58,7 @@ import org.apache.cassandra.sidecar.common.request.UploadableRequest; import static org.apache.cassandra.sidecar.common.http.SidecarHttpHeaderNames.AUTH_ROLE; +import static org.apache.cassandra.sidecar.common.http.SidecarQueryParamNames.INSTANCE_ID; import static org.apache.cassandra.sidecar.common.utils.StringUtils.isNullOrEmpty; /** @@ -252,6 +253,17 @@ protected HttpRequest vertxRequest(SidecarInstance sidecarInstance, Requ sidecarInstance.hostname(), request.requestURI()); + // Use the id resolved for the specific instance this request is being sent to, so requests + // fanned out across multiple instances each get the correct id. + Integer instanceId = sidecarInstance.instanceId(); + if (instanceId != null) + { + vertxRequest = vertxRequest.addQueryParam(INSTANCE_ID, String.valueOf(instanceId)); + LOGGER.debug("Appended {}={} to request uri. instance={}:{}, originalUri={}, finalUri={}", + INSTANCE_ID, instanceId, sidecarInstance.hostname(), sidecarInstance.port(), + request.requestURI(), vertxRequest.uri()); + } + vertxRequest = applyHeaders(vertxRequest, request.headers()); Map customHeaders = context.customHeaders(); diff --git a/analytics-sidecar-vertx-client/src/test/java/org/apache/cassandra/sidecar/client/VertxHttpClientTest.java b/analytics-sidecar-vertx-client/src/test/java/org/apache/cassandra/sidecar/client/VertxHttpClientTest.java index 8d8d0952d..1698541f5 100644 --- a/analytics-sidecar-vertx-client/src/test/java/org/apache/cassandra/sidecar/client/VertxHttpClientTest.java +++ b/analytics-sidecar-vertx-client/src/test/java/org/apache/cassandra/sidecar/client/VertxHttpClientTest.java @@ -22,11 +22,15 @@ import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import io.netty.handler.codec.http.HttpMethod; import io.vertx.core.Vertx; import io.vertx.core.buffer.Buffer; import io.vertx.ext.web.client.HttpRequest; +import org.apache.cassandra.sidecar.common.request.Request; + import static org.apache.cassandra.sidecar.common.http.SidecarHttpHeaderNames.AUTH_ROLE; +import static org.apache.cassandra.sidecar.common.http.SidecarQueryParamNames.INSTANCE_ID; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -56,16 +60,67 @@ public void testAuthHeaderSet() HttpClientConfig config = httpClientConfigBuilder().cassandraRole("custom_role").build(); try (VertxHttpClient client = new VertxHttpClient(vertx, config)) { - SidecarInstance instance = mock(SidecarInstance.class); - when(instance.port()).thenReturn(9043); - when(instance.hostname()).thenReturn("localhost"); RequestContext context = new RequestContext.Builder().ringRequest().build(); - HttpRequest request = client.vertxRequest(instance, context); + HttpRequest request = client.vertxRequest(mockInstance(), context); assertThat(request.headers()).isNotEmpty(); assertThat(request.headers().get(AUTH_ROLE)).isEqualTo("custom_role"); } } + @Test + public void testInstanceIdQueryParamAppended() + { + HttpClientConfig config = httpClientConfigBuilder().build(); + try (VertxHttpClient client = new VertxHttpClient(vertx, config)) + { + RequestContext context = new RequestContext.Builder().ringRequest().build(); + HttpRequest request = client.vertxRequest(mockInstance(42), context); + assertThat(request.queryParams().get(INSTANCE_ID)).isEqualTo("42"); + } + } + + @Test + public void testInstanceIdQueryParamNotAppendedWhenNull() + { + HttpClientConfig config = httpClientConfigBuilder().build(); + try (VertxHttpClient client = new VertxHttpClient(vertx, config)) + { + RequestContext context = new RequestContext.Builder().ringRequest().build(); + HttpRequest request = client.vertxRequest(mockInstance(), context); + assertThat(request.queryParams().contains(INSTANCE_ID)).isFalse(); + } + } + + @Test + public void testInstanceIdQueryParamAppendedWithExistingQueryParams() + { + HttpClientConfig config = httpClientConfigBuilder().build(); + try (VertxHttpClient client = new VertxHttpClient(vertx, config)) + { + Request mockRequest = mock(Request.class); + when(mockRequest.method()).thenReturn(HttpMethod.GET); + when(mockRequest.requestURI()).thenReturn("/api/v1/ring?existingParam=value"); + RequestContext context = new RequestContext.Builder().request(mockRequest).build(); + HttpRequest request = client.vertxRequest(mockInstance(7), context); + assertThat(request.queryParams().get("existingParam")).isEqualTo("value"); + assertThat(request.queryParams().get(INSTANCE_ID)).isEqualTo("7"); + } + } + + private SidecarInstance mockInstance() + { + return mockInstance(null); + } + + private SidecarInstance mockInstance(Integer instanceId) + { + SidecarInstance instance = mock(SidecarInstance.class); + when(instance.port()).thenReturn(9043); + when(instance.hostname()).thenReturn("localhost"); + when(instance.instanceId()).thenReturn(instanceId); + return instance; + } + private HttpClientConfig.Builder httpClientConfigBuilder() { return new HttpClientConfig.Builder<>() diff --git a/cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/common/model/CassandraInstance.java b/cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/common/model/CassandraInstance.java index fed2f2d59..f546c9d34 100644 --- a/cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/common/model/CassandraInstance.java +++ b/cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/common/model/CassandraInstance.java @@ -55,4 +55,20 @@ public interface CassandraInstance extends TokenOwner * @return status of the node */ NodeStatus nodeStatus(); + + /** + * Returns the identifier of the specific Cassandra instance that a shared Sidecar endpoint should route + * requests to, or {@code null} when not configured. + * + *

This is only meaningful when a single Sidecar endpoint (for example, one fronted by a load balancer) + * fronts more than one Cassandra instance: the id disambiguates which local instance a request targets, + * since the endpoint alone (hostname/Host header) cannot. When {@code null}, callers fall back to a + * job-level default, if any. + * + * @return the per-instance Sidecar routing id, or {@code null} when not set + */ + default @Nullable Integer sidecarInstanceId() + { + return null; + } } diff --git a/cassandra-analytics-core/src/main/java/org/apache/cassandra/clients/AnalyticsSidecarClient.java b/cassandra-analytics-core/src/main/java/org/apache/cassandra/clients/AnalyticsSidecarClient.java index 2096a60a9..6f15602bf 100644 --- a/cassandra-analytics-core/src/main/java/org/apache/cassandra/clients/AnalyticsSidecarClient.java +++ b/cassandra-analytics-core/src/main/java/org/apache/cassandra/clients/AnalyticsSidecarClient.java @@ -45,20 +45,7 @@ public static SidecarClient from(SidecarInstancesProvider sidecarInstancesProvid Vertx vertx = Vertx.vertx(new VertxOptions().setUseDaemonThread(true) .setWorkerPoolSize(conf.getMaxHttpConnections())); - String userAgent = transportModeBasedWriterUserAgent(conf.getTransportInfo().getTransport()); - HttpClientConfig httpClientConfig = new HttpClientConfig.Builder<>() - .timeoutMillis(conf.getHttpResponseTimeoutMs()) - .idleTimeoutMillis(conf.getHttpConnectionTimeoutMs()) - .userAgent(userAgent) - .keyStoreInputStream(conf.getKeyStore()) - .keyStorePassword(conf.getKeyStorePassword()) - .keyStoreType(conf.getKeyStoreTypeOrDefault()) - .trustStoreInputStream(conf.getTrustStore()) - .trustStorePassword(conf.getTrustStorePasswordOrDefault()) - .trustStoreType(conf.getTrustStoreTypeOrDefault()) - .ssl(conf.hasKeystoreAndKeystorePassword()) - .cassandraRole(conf.getCassandraRole()) - .build(); + HttpClientConfig httpClientConfig = buildHttpClientConfig(conf); StartupValidator.instance().register(new SslValidation(conf)); StartupValidator.instance().register(new BulkWriterKeyStoreValidation(conf)); @@ -74,6 +61,24 @@ public static SidecarClient from(SidecarInstancesProvider sidecarInstancesProvid return Sidecar.buildClient(sidecarConfig, vertx, httpClientConfig, sidecarInstancesProvider); } + static HttpClientConfig buildHttpClientConfig(BulkSparkConf conf) + { + String userAgent = transportModeBasedWriterUserAgent(conf.getTransportInfo().getTransport()); + return new HttpClientConfig.Builder<>() + .timeoutMillis(conf.getHttpResponseTimeoutMs()) + .idleTimeoutMillis(conf.getHttpConnectionTimeoutMs()) + .userAgent(userAgent) + .keyStoreInputStream(conf.getKeyStore()) + .keyStorePassword(conf.getKeyStorePassword()) + .keyStoreType(conf.getKeyStoreTypeOrDefault()) + .trustStoreInputStream(conf.getTrustStore()) + .trustStorePassword(conf.getTrustStorePasswordOrDefault()) + .trustStoreType(conf.getTrustStoreTypeOrDefault()) + .ssl(conf.hasKeystoreAndKeystorePassword()) + .cassandraRole(conf.getCassandraRole()) + .build(); + } + static String transportModeBasedWriterUserAgent(DataTransport transport) { switch (transport) diff --git a/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/CassandraClusterInfo.java b/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/CassandraClusterInfo.java index 3f7523bc0..2cbce5da3 100644 --- a/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/CassandraClusterInfo.java +++ b/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/CassandraClusterInfo.java @@ -45,6 +45,7 @@ import o.a.c.sidecar.client.shaded.common.response.SchemaResponse; import o.a.c.sidecar.client.shaded.common.response.TimeSkewResponse; import o.a.c.sidecar.client.shaded.common.response.TokenRangeReplicasResponse; +import o.a.c.sidecar.client.shaded.common.response.TokenRangeReplicasResponse.ReplicaMetadata; import org.apache.cassandra.bridge.CassandraBridge; import org.apache.cassandra.bridge.CassandraBridgeFactory; import org.apache.cassandra.bridge.CassandraVersion; @@ -253,7 +254,8 @@ void validateTimeSkewWithLocalNow(Range range, Instant localNow) thr .stream() .flatMap(Collection::stream) .distinct() // remove duplications - .map(replica -> new SidecarInstanceImpl(replica.nodeName(), getCassandraContext().sidecarPort())) + .map(replica -> new SidecarInstanceImpl(replica.nodeName(), getCassandraContext().sidecarPort(), + replica.sidecarInstanceId())) .collect(Collectors.toList()); timeSkew = getCassandraContext().getSidecarClient().timeSkew(instances).get(); } @@ -484,9 +486,61 @@ protected WriteAvailability determineWriteAvailability(RingInstance instance) private TokenRangeMapping getTokenRangeReplicasFromSidecar() { - return TokenRangeMapping.create(this::getTokenRangesAndReplicaSets, - this::getPartitioner, - metadata -> new RingInstance(metadata, clusterId())); + // Fallback for Sidecar versions that don't yet report a per-replica id in the ring/token-range-replicas + // response (see resolveSidecarInstanceId). Resolved from the contact points actually in effect for this + // cluster (getCluster() already picks the right source: conf.sidecarContactPoints() for a plain job, or + // conf.coordinatedWriteConf().cluster(clusterId).sidecarContactPoints() for a coordinated-write job). + // Deriving this from conf.sidecarContactPoints() directly would silently miss (or NPE on) coordinated + // writes, since their contact points don't live there. + Map instanceIdsByHostname = sidecarInstanceIdsByHostname(getCassandraContext().getCluster()); + TokenRangeMapping topology = + TokenRangeMapping.create(this::getTokenRangesAndReplicaSets, + this::getPartitioner, + metadata -> new RingInstance(metadata, clusterId(), + resolveSidecarInstanceId(metadata, instanceIdsByHostname))); + return topology; + } + + /** + * Resolves the Sidecar instance id to associate with a ring replica, preferring the id that Sidecar itself + * reports for that replica ({@link ReplicaMetadata#sidecarInstanceId()}), since Sidecar resolves it from its + * own local instance configuration keyed by the replica's address. Falls back to the statically-configured + * {@code host[:port]=} contact-point lookup only when talking to an older Sidecar that does not yet + * populate the field, in which case the {@link #sidecarInstanceIdsByHostname} limitations apply (see below). + * + * @param metadata the replica metadata returned by Sidecar for a ring entry + * @param instanceIdsByHostname the static fallback lookup built by {@link #sidecarInstanceIdsByHostname} + * @return the resolved Sidecar instance id, or {@code null} when neither source resolves one + */ + @VisibleForTesting + static Integer resolveSidecarInstanceId(ReplicaMetadata metadata, Map instanceIdsByHostname) + { + Integer dynamicId = metadata.sidecarInstanceId(); + return dynamicId != null ? dynamicId : instanceIdsByHostname.get(metadata.fqdn()); + } + + /** + * Builds a hostname (nodeName/fqdn) to per-instance Sidecar routing id lookup from the given contact points, + * e.g. ones declared as {@code "host:port="} (see {@link SidecarInstanceFactory#createFromString}). + * + *

This is only used as a fallback (see {@link #resolveSidecarInstanceId}) when Sidecar does not report a + * per-replica id itself. As a static, address-keyed lookup it has real limitations: it can only represent a + * 1:1 Sidecar-to-instance topology, where every instance has its own distinct address. If two or more + * instances share the same address with different ids configured, building this map throws + * {@code IllegalStateException: Duplicate key} — it cannot express that topology at all. The address must + * also match exactly (IP vs hostname, case) what the ring reports for that instance ({@code + * ReplicaMetadata#fqdn()}); a mismatch silently drops the configured id rather than resolving it. None of + * this applies when Sidecar reports the id dynamically, since that is resolved server-side per instance. + * + * @param contactPoints the Sidecar contact points in effect for this cluster + * @return a map of hostname to configured Sidecar instance id; entries with no configured id are omitted + */ + @VisibleForTesting + static Map sidecarInstanceIdsByHostname(Set contactPoints) + { + return contactPoints.stream() + .filter(instance -> instance.instanceId() != null) + .collect(Collectors.toMap(SidecarInstance::hostname, SidecarInstance::instanceId)); } public String getVersionFromFeature() diff --git a/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/RingInstance.java b/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/RingInstance.java index 5e5ed60e9..4a8fc63dd 100644 --- a/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/RingInstance.java +++ b/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/RingInstance.java @@ -36,13 +36,25 @@ public class RingInstance implements CassandraInstance, Serializable { - private static final long serialVersionUID = 4399143234683369652L; + private static final long serialVersionUID = 4399143234683369653L; private RingEntry ringEntry; private @Nullable String clusterId; + private @Nullable Integer sidecarInstanceId; public RingInstance(ReplicaMetadata replica, @Nullable String clusterId) + { + this(replica, clusterId, null); + } + + /** + * @param sidecarInstanceId the id of the Cassandra instance that a shared Sidecar endpoint fronting this + * instance should route requests to, or {@code null} when not configured for this + * instance (see {@link CassandraInstance#sidecarInstanceId()}) + */ + public RingInstance(ReplicaMetadata replica, @Nullable String clusterId, @Nullable Integer sidecarInstanceId) { this.clusterId = clusterId; + this.sidecarInstanceId = sidecarInstanceId; this.ringEntry = new RingEntry.Builder() .fqdn(replica.fqdn()) .address(replica.address()) @@ -61,8 +73,15 @@ public RingInstance(RingEntry ringEntry) @VisibleForTesting public RingInstance(RingEntry ringEntry, @Nullable String clusterId) + { + this(ringEntry, clusterId, null); + } + + @VisibleForTesting + public RingInstance(RingEntry ringEntry, @Nullable String clusterId, @Nullable Integer sidecarInstanceId) { this.clusterId = clusterId; + this.sidecarInstanceId = sidecarInstanceId; this.ringEntry = ringEntry; } @@ -122,13 +141,21 @@ public NodeStatus nodeStatus() return NodeStatus.fromNameIgnoreCase(ringEntry.status()); } + @Override + @Nullable + public Integer sidecarInstanceId() + { + return sidecarInstanceId; + } + /** * Custom equality that compares the token, fully qualified domain name, the rack, the port, the datacenter * and the clusterId * - * Note that node state, status and IP address are not part of the calculation. The IP address is excluded - * because a node can come back with a different IP address (e.g. a pod replacement in Kubernetes) while - * remaining the same logical instance. + * Note that node state, status, IP address and sidecarInstanceId are not part of the calculation. The IP + * address is excluded because a node can come back with a different IP address (e.g. a pod replacement in + * Kubernetes) while remaining the same logical instance. sidecarInstanceId is excluded because it is routing + * metadata derived from configuration, not part of the instance's identity. * * @param other the other instance * @return true if both instances are equal, false otherwise @@ -171,7 +198,7 @@ public int hashCode() @Override public String toString() { - return "RingInstance{cluster='" + clusterId + "', " + ringEntry.toString() + '}'; + return "RingInstance{cluster='" + clusterId + "', sidecarInstanceId=" + sidecarInstanceId + ", " + ringEntry.toString() + '}'; } public RingEntry ringEntry() @@ -194,6 +221,7 @@ private void writeObject(ObjectOutputStream out) throws IOException out.writeObject(ringEntry.load()); out.writeObject(ringEntry.owns()); out.writeObject(clusterId); + out.writeObject(sidecarInstanceId); } private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException @@ -211,6 +239,7 @@ private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundE String load = (String) in.readObject(); String owns = (String) in.readObject(); String clusterId = (String) in.readObject(); + Integer sidecarInstanceId = (Integer) in.readObject(); ringEntry = new RingEntry.Builder().datacenter(datacenter) .address(address) .port(port) @@ -224,5 +253,6 @@ private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundE .owns(owns) .build(); this.clusterId = clusterId; + this.sidecarInstanceId = sidecarInstanceId; } } diff --git a/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/SidecarDataTransferApi.java b/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/SidecarDataTransferApi.java index 9199b01ca..2d98c5d27 100644 --- a/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/SidecarDataTransferApi.java +++ b/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/SidecarDataTransferApi.java @@ -161,6 +161,6 @@ protected String getUploadId(String sessionID, String jobId) protected SidecarInstanceImpl toSidecarInstance(CassandraInstance instance) { - return new SidecarInstanceImpl(instance.nodeName(), sidecarPort); + return new SidecarInstanceImpl(instance.nodeName(), sidecarPort, instance.sidecarInstanceId()); } } diff --git a/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/WriterOptions.java b/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/WriterOptions.java index 5440016ac..134df9e89 100644 --- a/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/WriterOptions.java +++ b/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/WriterOptions.java @@ -30,6 +30,9 @@ public enum WriterOptions implements WriterOption SIDECAR_INSTANCES, // The option specifies the initial contact points of sidecar servers to discover the cluster topology // Note that the addresses can include port; when port is present, it takes precedence over SIDECAR_PORT + // Each address may also carry a trailing "=" suffix (e.g. "host:9043=2") to declare that instance's + // own per-instance Sidecar routing id - see SidecarInstanceFactory#createFromString. When an address has no + // such suffix, no instanceId is sent for that instance unless Sidecar reports one for it in the ring response. SIDECAR_CONTACT_POINTS, /** * The option specifies the configuration (in JSON) for coordinated write. diff --git a/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/common/SidecarInstanceFactory.java b/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/common/SidecarInstanceFactory.java index 129826b50..4dde5fa03 100644 --- a/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/common/SidecarInstanceFactory.java +++ b/cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/common/SidecarInstanceFactory.java @@ -38,6 +38,11 @@ private SidecarInstanceFactory() /** * Create SidecarInstance object by parsing the input string, which is IP address or hostname and optionally includes port + *

The input may also carry an optional per-instance id as a trailing {@code "="} suffix, e.g. + * {@code "host:9043=2"}. The id identifies which local Cassandra instance the receiving Sidecar should route + * requests to; it is used to populate the {@code instanceId} query parameter per instance. {@code '='} cannot + * appear in a hostname, IPv4/IPv6 address or port, so the suffix is unambiguous. When absent, no per-instance + * {@code instanceId} is sent for that instance (unless Sidecar reports one for it in the ring/token-range response). * @param input hostname string that can optionally includes the port. If port is present, the defaultPort param is ignored. * @param defaultPort port value used when the input string contains no port * @return SidecarInstanceImpl @@ -46,22 +51,42 @@ public static SidecarInstanceImpl createFromString(String input, int defaultPort { Preconditions.checkArgument(StringUtils.isNotEmpty(input), "Unable to create sidecar instance from empty input"); - String hostname = input; + String address = input; + Integer instanceId = null; + // Optional per-instance id, expressed as a trailing "=" suffix (e.g. "host:9043=2"). + int equalsIndex = input.lastIndexOf('='); + if (equalsIndex >= 0) + { + String instanceIdStr = input.substring(equalsIndex + 1).trim(); + try + { + instanceId = Integer.parseInt(instanceIdStr); + } + catch (NumberFormatException e) + { + throw new IllegalArgumentException( + String.format("Invalid sidecar instanceId '%s' in '%s'; expected a non-negative integer", instanceIdStr, input), e); + } + Preconditions.checkArgument(instanceId >= 0, "Sidecar instanceId must be non-negative; got %s in '%s'", instanceId, input); + address = input.substring(0, equalsIndex); + } + + String hostname = address; int port = defaultPort; // has port in the string. The former matches ipv6 and the latter matches ipv4 and hostnames // ipv6 with port example: [2024:a::1]:8080 - if (input.contains("]:") || (!input.startsWith("[") && input.contains(":"))) + if (address.contains("]:") || (!address.startsWith("[") && address.contains(":"))) { - int index = input.lastIndexOf(':'); - hostname = input.substring(0, index); // includes ']' if it is ipv6 - String portStr = input.substring(index + 1); + int index = address.lastIndexOf(':'); + hostname = address.substring(0, index); // includes ']' if it is ipv6 + String portStr = address.substring(index + 1); port = Integer.parseInt(portStr); } Preconditions.checkState(port != -1, "Unable to resolve port from %s", input); - LOGGER.info("Create sidecar instance. hostname={} port={}", hostname, port); - return new SidecarInstanceImpl(hostname, port); + LOGGER.info("Create sidecar instance. hostname={} port={} instanceId={}", hostname, port, instanceId); + return new SidecarInstanceImpl(hostname, port, instanceId); } /** diff --git a/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/CassandraClusterInfoTest.java b/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/CassandraClusterInfoTest.java index 99b06712f..597c7a41c 100644 --- a/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/CassandraClusterInfoTest.java +++ b/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/CassandraClusterInfoTest.java @@ -21,8 +21,12 @@ import java.time.Duration; import java.time.Instant; +import java.util.Map; +import java.util.Set; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; @@ -36,12 +40,16 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import o.a.c.sidecar.client.shaded.client.SidecarInstance; import o.a.c.sidecar.client.shaded.common.response.NodeSettings; import o.a.c.sidecar.client.shaded.common.response.TimeSkewResponse; +import o.a.c.sidecar.client.shaded.common.response.TokenRangeReplicasResponse.ReplicaMetadata; import org.apache.cassandra.spark.bulkwriter.token.TokenRangeMapping; +import org.apache.cassandra.spark.common.SidecarInstanceFactory; import org.apache.cassandra.spark.exception.TimeSkewTooLargeException; import static org.apache.cassandra.spark.TestUtils.range; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatNoException; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; @@ -120,6 +128,115 @@ public static CassandraClusterInfo mockClusterInfoForTimeSkewTest(int allowanceM return new MockClusterInfoForTimeSkew(allowanceMinutes, remoteNow); } + @Test + void testSidecarInstanceIdsByHostnameFromPlainContactPoints() + { + Set contactPoints = new HashSet<>(Arrays.asList( + SidecarInstanceFactory.createFromString("cassandra1:9043=1", 9043), + SidecarInstanceFactory.createFromString("cassandra2:9043=2", 9043), + SidecarInstanceFactory.createFromString("cassandra3:9043", 9043))); + + Map byHostname = CassandraClusterInfo.sidecarInstanceIdsByHostname(contactPoints); + + assertThat(byHostname).containsEntry("cassandra1", 1).containsEntry("cassandra2", 2); + assertThat(byHostname) + .describedAs("contact point with no '=' suffix has no entry") + .doesNotContainKey("cassandra3"); + } + + @Test + void testSidecarInstanceIdsByHostnameEmptyWhenNoneConfigured() + { + Set contactPoints = new HashSet<>(Arrays.asList( + SidecarInstanceFactory.createFromString("cassandra1:9043", 9043), + SidecarInstanceFactory.createFromString("cassandra2:9043", 9043))); + + assertThat(CassandraClusterInfo.sidecarInstanceIdsByHostname(contactPoints)).isEmpty(); + } + + @Test + void testSidecarInstanceIdsByHostnameThrowsWhenSharedHostnameHasDifferentIds() + { + // Reproduces a Sidecar deployment shared/load-balanced across multiple Cassandra instances: they are + // only reachable through the same address (e.g. one LB VIP), each meant to carry its own id. The + // current hostname-keyed lookup cannot represent this - it can only associate a single id per hostname - + // so instead of silently picking one id, it fails fast while building the lookup. + Set contactPoints = new HashSet<>(Arrays.asList( + SidecarInstanceFactory.createFromString("sidecar-lb:9043=1"), + SidecarInstanceFactory.createFromString("sidecar-lb:9043=2"), + SidecarInstanceFactory.createFromString("sidecar-lb:9043=3"))); + + assertThatThrownBy(() -> CassandraClusterInfo.sidecarInstanceIdsByHostname(contactPoints)) + .describedAs("a single shared Sidecar endpoint fronting multiple instances (e.g. behind a load balancer) " + + "cannot be expressed by a hostname->id map keyed on hostname alone") + .isExactlyInstanceOf(IllegalStateException.class) + .hasMessageContaining("Duplicate key"); + } + + @Test + void testSidecarInstanceIdsByHostnameMissesWhenAddressFormatDiffersFromRingFqdn() + { + // The lookup is keyed on the literal contact-point address string. If the ring later reports this same + // physical instance under a different string (e.g. its fqdn, when the contact point was configured by + // IP), the two never match: getTokenRangeReplicasFromSidecar's instanceIdsByHostname.get(metadata.fqdn()) + // misses, and the configured id is silently dropped for that instance - not an exception, just a null. + Set contactPoints = Collections.singleton( + SidecarInstanceFactory.createFromString("10.0.0.5:9043=1")); + + Map byHostname = CassandraClusterInfo.sidecarInstanceIdsByHostname(contactPoints); + + assertThat(byHostname).containsEntry("10.0.0.5", 1); + assertThat(byHostname) + .describedAs("the ring would report this same instance by its fqdn, not its IP - the lookup key must " + + "match exactly, so the configured id is invisible under the fqdn key") + .doesNotContainKey("node1.example.com"); + } + + @Test + void testResolveSidecarInstanceIdPrefersDynamicIdFromSidecar() + { + ReplicaMetadata metadata = new ReplicaMetadata("NORMAL", "UP", "dc1-i0", "10.0.0.5", 9042, "dc1", 7); + Map instanceIdsByHostname = Collections.singletonMap("dc1-i0", 99); + + assertThat(CassandraClusterInfo.resolveSidecarInstanceId(metadata, instanceIdsByHostname)) + .describedAs("the id Sidecar resolves for the replica itself must win over the static contact-point fallback") + .isEqualTo(7); + } + + @Test + void testResolveSidecarInstanceIdFallsBackToStaticMapWhenSidecarDoesNotReportOne() + { + ReplicaMetadata metadata = new ReplicaMetadata("NORMAL", "UP", "dc1-i0", "10.0.0.5", 9042, "dc1", null); + Map instanceIdsByHostname = Collections.singletonMap("dc1-i0", 99); + + assertThat(CassandraClusterInfo.resolveSidecarInstanceId(metadata, instanceIdsByHostname)) + .describedAs("an older Sidecar that doesn't populate sidecarInstanceId falls back to the static, " + + "operator-configured contact-point lookup") + .isEqualTo(99); + } + + @Test + void testResolveSidecarInstanceIdNullWhenNeitherSourceResolves() + { + ReplicaMetadata metadata = new ReplicaMetadata("NORMAL", "UP", "dc1-i0", "10.0.0.5", 9042, "dc1", null); + + assertThat(CassandraClusterInfo.resolveSidecarInstanceId(metadata, Collections.emptyMap())).isNull(); + } + + @Test + void testResolveSidecarInstanceIdDoesNotDependOnAddressKeyMatching() + { + // Two replicas reported under the same address string - the static, hostname-keyed fallback cannot + // express two different ids for one key (see testSidecarInstanceIdsByHostnameThrowsWhenSharedHostnameHasDifferentIds). + // Resolution here doesn't go through that lookup at all when Sidecar reports the id itself, so it isn't + // subject to that limitation. + ReplicaMetadata replicaManagedByInstance1 = new ReplicaMetadata("NORMAL", "UP", "shared-lb", "10.0.0.5", 9042, "dc1", 1); + ReplicaMetadata replicaManagedByInstance2 = new ReplicaMetadata("NORMAL", "UP", "shared-lb", "10.0.0.5", 9042, "dc1", 2); + + assertThat(CassandraClusterInfo.resolveSidecarInstanceId(replicaManagedByInstance1, Collections.emptyMap())).isEqualTo(1); + assertThat(CassandraClusterInfo.resolveSidecarInstanceId(replicaManagedByInstance2, Collections.emptyMap())).isEqualTo(2); + } + private BulkSparkConf mockBulkSparkWithSidecarConf(int requestTimeoutSeconds, long maxRetryDelayMillis, int retryCount) { BulkSparkConf conf = mock(BulkSparkConf.class); diff --git a/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/RingInstanceSerializationTest.java b/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/RingInstanceSerializationTest.java index c2ff45901..fd8c831cb 100644 --- a/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/RingInstanceSerializationTest.java +++ b/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/RingInstanceSerializationTest.java @@ -67,7 +67,8 @@ public void testRingSerializesFromReplicaMetadata() dataCenter + "-i" + index, "127.0." + dcOffset + "." + index, 7000, - dataCenter); + dataCenter, + null); RingInstance ring = new RingInstance(metadata, "test-cluster"); @@ -75,4 +76,29 @@ public void testRingSerializesFromReplicaMetadata() RingInstance deserialized = deserialize(bytes, RingInstance.class); assertThat(deserialized).isEqualTo(ring); } + + @Test + public void testSidecarInstanceIdSurvivesSerialization() + { + int dcOffset = 0; + String dataCenter = "DC1"; + int index = 0; + ReplicaMetadata metadata = new ReplicaMetadata("NORMAL", + "UP", + dataCenter + "-i" + index, + "127.0." + dcOffset + "." + index, + 7000, + dataCenter, + null); + + RingInstance ring = new RingInstance(metadata, "test-cluster", 2); + + byte[] bytes = serialize(ring); + RingInstance deserialized = deserialize(bytes, RingInstance.class); + // sidecarInstanceId is excluded from equals/hashCode, so it must be checked explicitly: + // this is exactly the field that would silently revert to null if serialization dropped it, + // which would reintroduce the misrouting bug once the instance travels to an executor. + assertThat(deserialized).isEqualTo(ring); + assertThat(deserialized.sidecarInstanceId()).isEqualTo(2); + } } diff --git a/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/RingInstanceTest.java b/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/RingInstanceTest.java index b73cc272a..0aecd7157 100644 --- a/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/RingInstanceTest.java +++ b/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/RingInstanceTest.java @@ -178,16 +178,42 @@ public void testToString() { RingEntry ringEntry = mockRingEntry(); RingInstance instanceWithoutClusterId = new RingInstance(ringEntry); - assertThat(instanceWithoutClusterId.toString()).isEqualTo("RingInstance{cluster='null', " + + assertThat(instanceWithoutClusterId.toString()).isEqualTo("RingInstance{cluster='null', sidecarInstanceId=null, " + "RingEntry{datacenter='DATACENTER1', address='127.0.0.1', port=0, rack='Rack', " + "status='UP', state='NORMAL', load='0', owns='', token='0', fqdn='DATACENTER1-i1', hostId=''}}"); RingInstance instanceWithClusterId = new RingInstance(ringEntry, "clusterId"); - assertThat(instanceWithClusterId.toString()).isEqualTo("RingInstance{cluster='clusterId', " + + assertThat(instanceWithClusterId.toString()).isEqualTo("RingInstance{cluster='clusterId', sidecarInstanceId=null, " + "RingEntry{datacenter='DATACENTER1', address='127.0.0.1', port=0, rack='Rack', " + "status='UP', state='NORMAL', load='0', owns='', token='0', fqdn='DATACENTER1-i1', hostId=''}}"); } + @Test + public void testSidecarInstanceIdDefaultsToNull() + { + RingInstance instance = new RingInstance(mockRingEntry()); + assertThat(instance.sidecarInstanceId()).isNull(); + } + + @Test + public void testSidecarInstanceIdIsRetained() + { + RingInstance instance = new RingInstance(mockRingEntry(), null, 2); + assertThat(instance.sidecarInstanceId()).isEqualTo(2); + } + + @Test + public void testEqualsAndHashcodeIgnoreSidecarInstanceId() + { + RingEntry ringEntry = mockRingEntry(); + RingInstance instanceWithId = new RingInstance(ringEntry, null, 1); + RingInstance instanceWithDifferentId = new RingInstance(ringEntry, null, 2); + RingInstance instanceWithoutId = new RingInstance(ringEntry); + + assertThat(instanceWithId).isEqualTo(instanceWithDifferentId).isEqualTo(instanceWithoutId); + assertThat(instanceWithId.hashCode()).isEqualTo(instanceWithDifferentId.hashCode()).isEqualTo(instanceWithoutId.hashCode()); + } + @NotNull private static RingEntry mockRingEntry() diff --git a/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/SidecarDataTransferApiTest.java b/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/SidecarDataTransferApiTest.java new file mode 100644 index 000000000..f2d4baeea --- /dev/null +++ b/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/SidecarDataTransferApiTest.java @@ -0,0 +1,88 @@ +/* + * 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.spark.bulkwriter; + +import org.junit.jupiter.api.Test; + +import o.a.c.sidecar.client.shaded.client.SidecarInstanceImpl; +import o.a.c.sidecar.client.shaded.common.response.data.RingEntry; +import org.apache.cassandra.bridge.CassandraBridge; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.RETURNS_DEEP_STUBS; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Unit tests for {@link SidecarDataTransferApi} + */ +class SidecarDataTransferApiTest +{ + @Test + void testToSidecarInstanceCarriesPerInstanceId() + { + SidecarDataTransferApi api = api(); + RingInstance instance = ringInstance("dc1-i0", 2); + + SidecarInstanceImpl sidecarInstance = api.toSidecarInstance(instance); + + assertThat(sidecarInstance.hostname()).isEqualTo("dc1-i0"); + assertThat(sidecarInstance.port()).isEqualTo(9043); + assertThat(sidecarInstance.instanceId()) + .describedAs("upload/commit/cleanup requests must carry the target instance's own id, " + + "not a single job-wide value, or a multi-node write silently misroutes") + .isEqualTo(2); + } + + @Test + void testToSidecarInstanceFallsBackToNullWhenNoPerInstanceIdConfigured() + { + SidecarDataTransferApi api = api(); + RingInstance instance = ringInstance("dc1-i1", null); + + SidecarInstanceImpl sidecarInstance = api.toSidecarInstance(instance); + + assertThat(sidecarInstance.instanceId()).isNull(); + } + + private static SidecarDataTransferApi api() + { + CassandraContext context = mock(CassandraContext.class, RETURNS_DEEP_STUBS); + when(context.sidecarPort()).thenReturn(9043); + return new SidecarDataTransferApi(context, mock(CassandraBridge.class), mock(JobInfo.class)); + } + + private static RingInstance ringInstance(String fqdn, Integer sidecarInstanceId) + { + return new RingInstance(new RingEntry.Builder() + .datacenter("dc1") + .address(fqdn) + .port(7000) + .status("UP") + .state("NORMAL") + .token("0") + .fqdn(fqdn) + .rack("rack") + .owns("") + .load("") + .hostId("") + .build(), null, sidecarInstanceId); + } +} diff --git a/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/TokenRangeMappingUtils.java b/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/TokenRangeMappingUtils.java index 4864144e3..f1252f809 100644 --- a/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/TokenRangeMappingUtils.java +++ b/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/TokenRangeMappingUtils.java @@ -261,7 +261,7 @@ public static TokenRangeReplicasResponse mockSimpleTokenRangeReplicasResponse(lo String address = "localhost" + i; int port = 9042; String addressWithPort = address + ":" + port; - ReplicaMetadata rm = new ReplicaMetadata("NORMAL", "UP", address, address, 9042, "dc1"); + ReplicaMetadata rm = new ReplicaMetadata("NORMAL", "UP", address, address, 9042, "dc1", null); replicaMetadata.put(addressWithPort, rm); startToken = endToken; } diff --git a/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/token/TokenRangeMappingTest.java b/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/token/TokenRangeMappingTest.java index 92f9abad0..0b7babe58 100644 --- a/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/token/TokenRangeMappingTest.java +++ b/cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/bulkwriter/token/TokenRangeMappingTest.java @@ -93,7 +93,8 @@ void testCreateTokenRangeMappingWithPending() metadata.fqdn(), metadata.address(), metadata.port(), - metadata.datacenter()); + metadata.datacenter(), + metadata.sidecarInstanceId()); response.replicaMetadata().put(key, updatedMetadata); if (state.isPending) { diff --git a/cassandra-analytics-core/src/test/spark3/org/apache/cassandra/spark/common/SidecarInstanceFactoryTest.java b/cassandra-analytics-core/src/test/spark3/org/apache/cassandra/spark/common/SidecarInstanceFactoryTest.java index 277175a7f..6c8a6663c 100644 --- a/cassandra-analytics-core/src/test/spark3/org/apache/cassandra/spark/common/SidecarInstanceFactoryTest.java +++ b/cassandra-analytics-core/src/test/spark3/org/apache/cassandra/spark/common/SidecarInstanceFactoryTest.java @@ -47,9 +47,46 @@ void testCreateSidecarInstance() "[2024:a::1]", 8888); } + @Test + void testCreateSidecarInstanceWithInstanceId() + { + assertSidecarInstance(SidecarInstanceFactory.createFromString("localhost:8888=2", 9999), + "localhost", 8888, 2); + assertSidecarInstance(SidecarInstanceFactory.createFromString("127.0.0.1:8888=0", 9999), + "127.0.0.1", 8888, 0); + // no explicit port: default port applies, id still parsed + assertSidecarInstance(SidecarInstanceFactory.createFromString("localhost=3", 9999), + "localhost", 9999, 3); + // ipv6 with port and id + assertSidecarInstance(SidecarInstanceFactory.createFromString("[2024:a::1]:8888=7", 9999), + "[2024:a::1]", 8888, 7); + // no id: instanceId is null (falls back to the job-level value) + assertSidecarInstance(SidecarInstanceFactory.createFromString("localhost:8888", 9999), + "localhost", 8888, null); + } + + @Test + void testCreateSidecarInstanceWithInvalidInstanceId() + { + assertThatThrownBy(() -> SidecarInstanceFactory.createFromString("localhost:8888=abc", 9999)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Invalid sidecar instanceId"); + + assertThatThrownBy(() -> SidecarInstanceFactory.createFromString("localhost:8888=-1", 9999)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("non-negative"); + } + private void assertSidecarInstance(SidecarInstance sidecarInstance, String expectedHostname, int expectedPort) { assertThat(sidecarInstance.hostname()).isEqualTo(expectedHostname); assertThat(sidecarInstance.port()).isEqualTo(expectedPort); } + + private void assertSidecarInstance(SidecarInstance sidecarInstance, String expectedHostname, int expectedPort, + Integer expectedInstanceId) + { + assertSidecarInstance(sidecarInstance, expectedHostname, expectedPort); + assertThat(sidecarInstance.instanceId()).isEqualTo(expectedInstanceId); + } } diff --git a/cassandra-analytics-core/src/test/spark4/org/apache/cassandra/spark/common/SidecarInstanceFactoryTest.java b/cassandra-analytics-core/src/test/spark4/org/apache/cassandra/spark/common/SidecarInstanceFactoryTest.java index 277175a7f..6c8a6663c 100644 --- a/cassandra-analytics-core/src/test/spark4/org/apache/cassandra/spark/common/SidecarInstanceFactoryTest.java +++ b/cassandra-analytics-core/src/test/spark4/org/apache/cassandra/spark/common/SidecarInstanceFactoryTest.java @@ -47,9 +47,46 @@ void testCreateSidecarInstance() "[2024:a::1]", 8888); } + @Test + void testCreateSidecarInstanceWithInstanceId() + { + assertSidecarInstance(SidecarInstanceFactory.createFromString("localhost:8888=2", 9999), + "localhost", 8888, 2); + assertSidecarInstance(SidecarInstanceFactory.createFromString("127.0.0.1:8888=0", 9999), + "127.0.0.1", 8888, 0); + // no explicit port: default port applies, id still parsed + assertSidecarInstance(SidecarInstanceFactory.createFromString("localhost=3", 9999), + "localhost", 9999, 3); + // ipv6 with port and id + assertSidecarInstance(SidecarInstanceFactory.createFromString("[2024:a::1]:8888=7", 9999), + "[2024:a::1]", 8888, 7); + // no id: instanceId is null (falls back to the job-level value) + assertSidecarInstance(SidecarInstanceFactory.createFromString("localhost:8888", 9999), + "localhost", 8888, null); + } + + @Test + void testCreateSidecarInstanceWithInvalidInstanceId() + { + assertThatThrownBy(() -> SidecarInstanceFactory.createFromString("localhost:8888=abc", 9999)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Invalid sidecar instanceId"); + + assertThatThrownBy(() -> SidecarInstanceFactory.createFromString("localhost:8888=-1", 9999)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("non-negative"); + } + private void assertSidecarInstance(SidecarInstance sidecarInstance, String expectedHostname, int expectedPort) { assertThat(sidecarInstance.hostname()).isEqualTo(expectedHostname); assertThat(sidecarInstance.port()).isEqualTo(expectedPort); } + + private void assertSidecarInstance(SidecarInstance sidecarInstance, String expectedHostname, int expectedPort, + Integer expectedInstanceId) + { + assertSidecarInstance(sidecarInstance, expectedHostname, expectedPort); + assertThat(sidecarInstance.instanceId()).isEqualTo(expectedInstanceId); + } } diff --git a/cassandra-analytics-sidecar-client/src/main/java/org/apache/cassandra/clients/Sidecar.java b/cassandra-analytics-sidecar-client/src/main/java/org/apache/cassandra/clients/Sidecar.java index 78798416a..1a9269c27 100644 --- a/cassandra-analytics-sidecar-client/src/main/java/org/apache/cassandra/clients/Sidecar.java +++ b/cassandra-analytics-sidecar-client/src/main/java/org/apache/cassandra/clients/Sidecar.java @@ -235,7 +235,7 @@ else if (gossipInfoResponses.size() < gossipInfoFutures.size()) public static SidecarInstance toSidecarInstance(CassandraInstance instance, int sidecarPort) { - return new SidecarInstanceImpl(instance.nodeName(), sidecarPort); + return new SidecarInstanceImpl(instance.nodeName(), sidecarPort, instance.sidecarInstanceId()); } public static final class ClientConfig