You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "arjunashok (via GitHub)" <gi...@apache.org> on 2024/08/29 23:34:40 UTC
[PR] CASSANDRASC-144 Introduces sidecar endpoint to vend statistics associ… [cassandra-sidecar]
arjunashok opened a new pull request, #135:
URL: https://github.com/apache/cassandra-sidecar/pull/135
…ated with the client connection
## Changes
- Introduces new handler for client-stats
- Supports query params for `client-options`, `by-protocol`, `list-connections` and `verbose`.
- Added integration tests
--
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
Re: [PR] CASSANDRASC-144 Introduces sidecar endpoint to vend statistics associ… [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #135:
URL: https://github.com/apache/cassandra-sidecar/pull/135#discussion_r1776120713
##########
adapters/base/src/main/java/org/apache/cassandra/sidecar/adapters/base/db/ConnectedClientStats.java:
##########
@@ -0,0 +1,210 @@
+/*
+ * 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.adapters.base.db;
+
+import java.util.Map;
+
+import com.datastax.driver.core.Row;
+import org.apache.cassandra.sidecar.common.DataObjectBuilder;
+import org.apache.cassandra.sidecar.db.DataObjectMappingException;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Representation of the connected clients metadata
+ */
+public class ConnectedClientStats
+{
+
+ public final String address;
+ public final int port;
+ public final String hostname;
+ public final String username;
+ public final String connectionStage;
+ public final String protocolVersion;
+ public final Map<String, String> clientOptions;
+ public final String driverName;
+ public final String driverVersion;
+ public final boolean sslEnabled;
+ public final String sslProtocol;
+ public final String sslCipherSuite;
+ public final String keyspaceName;
+ public final int requestCount;
+ public final String authenticationMode;
+ public final Map<String, String> authMetadata;
+
+ public static ConnectedClientStats.Builder builder()
+ {
+ return new ConnectedClientStats.Builder();
+ }
+
+ public static ConnectedClientStats from(@NotNull Row row) throws DataObjectMappingException
+ {
+ ConnectedClientStats.Builder builder = new ConnectedClientStats.Builder();
Review Comment:
builder seems unnecessary here. Can we have a simple constructor and simplify?
##########
adapters/base/src/main/java/org/apache/cassandra/sidecar/adapters/base/db/ConnectedClientStatsSummary.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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.adapters.base.db;
+
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import com.datastax.driver.core.ResultSet;
+import org.apache.cassandra.sidecar.common.DataObjectBuilder;
+import org.apache.cassandra.sidecar.db.DataObjectMappingException;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Representation of a summary of client connections stats
+ */
+public class ConnectedClientStatsSummary
+{
+ public final int totalConnectedClients;
+ public final Map<String, Long> connectionsByUser;
+
+ public static ConnectedClientStatsSummary.Builder builder()
+ {
+ return new ConnectedClientStatsSummary.Builder();
+ }
+
+ public static ConnectedClientStatsSummary from(@NotNull ResultSet resultSet) throws DataObjectMappingException
+ {
+
+ Map<String, Long> resultMap = resultSet.all().stream()
+ .collect(Collectors.toMap(r -> r.getString("username"),
+ r -> r.getLong("connection_count")));
+ int totalConnections = resultMap.values().stream().mapToInt(Math::toIntExact).sum();
+
+ ConnectedClientStatsSummary.Builder builder = new ConnectedClientStatsSummary.Builder();
+ builder.connectionsByUser(resultMap);
+ builder.totalConnectedClients(totalConnections);
+
+ return builder.build();
+ }
+
+ private ConnectedClientStatsSummary(ConnectedClientStatsSummary.Builder builder)
Review Comment:
simplify by removing the builder?
##########
client-common/src/main/java/org/apache/cassandra/sidecar/common/response/data/SidecarSchemaModificationException.java:
##########
@@ -16,7 +16,7 @@
* limitations under the License.
*/
-package org.apache.cassandra.sidecar.exceptions;
+package org.apache.cassandra.sidecar.common.response.data;
Review Comment:
can we preserve the package ?
--
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
Re: [PR] CASSANDRASC-144 Introduces sidecar endpoint to vend statistics associ… [cassandra-sidecar]
Posted by "arjunashok (via GitHub)" <gi...@apache.org>.
arjunashok commented on code in PR #135:
URL: https://github.com/apache/cassandra-sidecar/pull/135#discussion_r1750975083
##########
src/test/integration/org/apache/cassandra/sidecar/routes/ClientStatsHandlerIntegrationTest.java:
##########
@@ -0,0 +1,170 @@
+/*
+ * 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.routes;
+
+import java.util.Collections;
+import java.util.Map;
+
+import org.junit.jupiter.api.extension.ExtendWith;
+
+import io.vertx.core.buffer.Buffer;
+import io.vertx.ext.web.client.HttpResponse;
+import io.vertx.ext.web.client.predicate.ResponsePredicate;
+import io.vertx.junit5.VertxExtension;
+import io.vertx.junit5.VertxTestContext;
+import org.apache.cassandra.sidecar.common.response.ClientStatsResponse;
+import org.apache.cassandra.sidecar.testing.IntegrationTestBase;
+import org.apache.cassandra.testing.CassandraIntegrationTest;
+import org.apache.cassandra.testing.CassandraTestContext;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Test the client-stats endpoint with cassandra container.
+ */
+@ExtendWith(VertxExtension.class)
+public class ClientStatsHandlerIntegrationTest extends IntegrationTestBase
+{
+
+ @CassandraIntegrationTest
+ void retrieveClientStatsDefault(VertxTestContext context, CassandraTestContext cassandraTestContext)
+ throws Exception
+ {
+ String testRoute = "/api/v1/storage/client-stats";
+ testWithClient(context, client -> {
+ client.get(server.actualPort(), "127.0.0.1", testRoute)
+ .expect(ResponsePredicate.SC_OK)
+ .send(context.succeeding(response -> {
+ assertClientStatsResponse(response, Collections.emptyMap(), cassandraTestContext.version.toString());
+ context.completeNow();
+ }));
+ });
+ }
+
+ @CassandraIntegrationTest
+ void retrieveClientStatsWithClientOptions(VertxTestContext context, CassandraTestContext cassandraTestContext)
+ throws Exception
+ {
+ Map<String, Boolean> params = Collections.singletonMap("client-options", true);
+ String testRoute = "/api/v1/storage/client-stats?client-options=true";
+ testWithClient(context, client -> {
+ client.get(server.actualPort(), "127.0.0.1", testRoute)
+ .expect(ResponsePredicate.SC_OK)
+ .send(context.succeeding(response -> {
+ assertClientStatsResponse(response, params, cassandraTestContext.version.toString());
+ context.completeNow();
+ }));
+ });
+ }
+
+ @CassandraIntegrationTest
+ void retrieveClientStatsByProtocol(VertxTestContext context, CassandraTestContext cassandraTestContext)
+ throws Exception
+ {
+ Map<String, Boolean> params = Collections.singletonMap("by-protocol", true);
+ String testRoute = "/api/v1/storage/client-stats?by-protocol=true";
+ testWithClient(context, client -> {
+ client.get(server.actualPort(), "127.0.0.1", testRoute)
+ .expect(ResponsePredicate.SC_OK)
+ .send(context.succeeding(response -> {
+ assertClientStatsResponse(response, params, cassandraTestContext.version.toString());
+ context.completeNow();
+ }));
+ });
+ }
+
+ @CassandraIntegrationTest
+ void retrieveClientStatsVerbose(VertxTestContext context, CassandraTestContext cassandraTestContext)
+ throws Exception
+ {
+ Map<String, Boolean> params = Collections.singletonMap("verbose", true);
+ String testRoute = "/api/v1/storage/client-stats?verbose=true";
Review Comment:
Will do
##########
src/test/integration/org/apache/cassandra/sidecar/routes/ClientStatsHandlerIntegrationTest.java:
##########
@@ -0,0 +1,170 @@
+/*
+ * 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.routes;
+
+import java.util.Collections;
+import java.util.Map;
+
+import org.junit.jupiter.api.extension.ExtendWith;
+
+import io.vertx.core.buffer.Buffer;
+import io.vertx.ext.web.client.HttpResponse;
+import io.vertx.ext.web.client.predicate.ResponsePredicate;
+import io.vertx.junit5.VertxExtension;
+import io.vertx.junit5.VertxTestContext;
+import org.apache.cassandra.sidecar.common.response.ClientStatsResponse;
+import org.apache.cassandra.sidecar.testing.IntegrationTestBase;
+import org.apache.cassandra.testing.CassandraIntegrationTest;
+import org.apache.cassandra.testing.CassandraTestContext;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Test the client-stats endpoint with cassandra container.
+ */
+@ExtendWith(VertxExtension.class)
+public class ClientStatsHandlerIntegrationTest extends IntegrationTestBase
+{
+
+ @CassandraIntegrationTest
+ void retrieveClientStatsDefault(VertxTestContext context, CassandraTestContext cassandraTestContext)
+ throws Exception
+ {
+ String testRoute = "/api/v1/storage/client-stats";
+ testWithClient(context, client -> {
+ client.get(server.actualPort(), "127.0.0.1", testRoute)
+ .expect(ResponsePredicate.SC_OK)
+ .send(context.succeeding(response -> {
+ assertClientStatsResponse(response, Collections.emptyMap(), cassandraTestContext.version.toString());
+ context.completeNow();
+ }));
+ });
+ }
+
+ @CassandraIntegrationTest
+ void retrieveClientStatsWithClientOptions(VertxTestContext context, CassandraTestContext cassandraTestContext)
+ throws Exception
+ {
+ Map<String, Boolean> params = Collections.singletonMap("client-options", true);
+ String testRoute = "/api/v1/storage/client-stats?client-options=true";
+ testWithClient(context, client -> {
+ client.get(server.actualPort(), "127.0.0.1", testRoute)
+ .expect(ResponsePredicate.SC_OK)
+ .send(context.succeeding(response -> {
+ assertClientStatsResponse(response, params, cassandraTestContext.version.toString());
+ context.completeNow();
+ }));
+ });
+ }
+
+ @CassandraIntegrationTest
+ void retrieveClientStatsByProtocol(VertxTestContext context, CassandraTestContext cassandraTestContext)
+ throws Exception
+ {
+ Map<String, Boolean> params = Collections.singletonMap("by-protocol", true);
+ String testRoute = "/api/v1/storage/client-stats?by-protocol=true";
+ testWithClient(context, client -> {
+ client.get(server.actualPort(), "127.0.0.1", testRoute)
Review Comment:
Good point. However, since all the existing integration tests are defined this way, and the `Server` implementation does not expose the host API, I'd like to keep this as-is to keep the scope of this PR confined to the changes related to this API.
##########
src/main/java/org/apache/cassandra/sidecar/routes/ClientStatsHandler.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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.routes;
+
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import com.datastax.driver.core.Metadata;
+import com.google.inject.Inject;
+import io.vertx.core.http.HttpServerRequest;
+import io.vertx.core.net.SocketAddress;
+import io.vertx.ext.web.RoutingContext;
+import org.apache.cassandra.sidecar.cluster.CassandraAdapterDelegate;
+import org.apache.cassandra.sidecar.common.server.MetricsOperations;
+import org.apache.cassandra.sidecar.concurrent.ExecutorPools;
+import org.apache.cassandra.sidecar.utils.InstanceMetadataFetcher;
+
+import static org.apache.cassandra.sidecar.utils.HttpExceptions.cassandraServiceUnavailable;
+
+/**
+ * Handler for retrieving client stats
+ */
+public class ClientStatsHandler extends AbstractHandler<Void>
+{
+ /**
+ * Constructs a handler with the provided {@code metadataFetcher}
+ *
+ * @param metadataFetcher the metadata fetcher
+ * @param executorPools executor pools for blocking executions
+ */
+ @Inject
+ protected ClientStatsHandler(InstanceMetadataFetcher metadataFetcher, ExecutorPools executorPools)
+ {
+ super(metadataFetcher, executorPools, null);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public void handleInternal(RoutingContext context,
+ HttpServerRequest httpRequest,
+ String host,
+ SocketAddress remoteAddress,
+ Void request)
+ {
+
+ CassandraAdapterDelegate delegate = metadataFetcher.delegate(host);
+ if (delegate == null)
+ {
+ context.fail(cassandraServiceUnavailable());
+ return;
+ }
+ MetricsOperations operations = delegate.metricsOperations();
+ Metadata metadata = delegate.metadata();
+ if (operations == null || metadata == null)
+ {
+ context.fail(cassandraServiceUnavailable());
+ return;
+ }
+
+ Map<String, String> params = httpRequest.params().entries()
Review Comment:
Yes, ignoring arbitrary params is intentional, so as to not fail-fast, which is typically common-practice.
As for the invalid/non-boolean values, the existing sidecar APIs default them to `false`. I'll update this to use the existing `parseBooleanQueryParam` utility invoked from the handler.
--
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
Re: [PR] CASSANDRASC-144 Introduces sidecar endpoint to vend statistics associ… [cassandra-sidecar]
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #135:
URL: https://github.com/apache/cassandra-sidecar/pull/135#discussion_r1788286538
##########
adapters/base/src/main/java/org/apache/cassandra/sidecar/adapters/base/CassandraMetricsOperations.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.adapters.base;
+
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import org.apache.cassandra.sidecar.adapters.base.db.ConnectedClientStats;
+import org.apache.cassandra.sidecar.adapters.base.db.ConnectedClientStatsDatabaseAccessor;
+import org.apache.cassandra.sidecar.adapters.base.db.ConnectedClientStatsSummary;
+import org.apache.cassandra.sidecar.adapters.base.db.schema.ClientStatsSchema;
+import org.apache.cassandra.sidecar.common.response.ConnectedClientStatsResponse;
+import org.apache.cassandra.sidecar.common.response.data.ClientConnectionEntry;
+import org.apache.cassandra.sidecar.common.server.CQLSessionProvider;
+import org.apache.cassandra.sidecar.common.server.JmxClient;
+import org.apache.cassandra.sidecar.common.server.MetricsOperations;
+
+/**
+ * Default implementation that pulls methods from the Cassandra Metrics Proxy
+ */
+public class CassandraMetricsOperations implements MetricsOperations
+{
+ private ConnectedClientStatsDatabaseAccessor dbAccessor;
+
+ /**
+ * Creates a new instance with the provided {@link JmxClient}
Review Comment:
Correct JavaDoc
```suggestion
* Creates a new instance with the provided {@link CQLSessionProvider}
```
##########
adapters/base/src/main/java/org/apache/cassandra/sidecar/adapters/base/CassandraMetricsOperations.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.adapters.base;
+
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import org.apache.cassandra.sidecar.adapters.base.db.ConnectedClientStats;
+import org.apache.cassandra.sidecar.adapters.base.db.ConnectedClientStatsDatabaseAccessor;
+import org.apache.cassandra.sidecar.adapters.base.db.ConnectedClientStatsSummary;
+import org.apache.cassandra.sidecar.adapters.base.db.schema.ClientStatsSchema;
+import org.apache.cassandra.sidecar.common.response.ConnectedClientStatsResponse;
+import org.apache.cassandra.sidecar.common.response.data.ClientConnectionEntry;
+import org.apache.cassandra.sidecar.common.server.CQLSessionProvider;
+import org.apache.cassandra.sidecar.common.server.JmxClient;
Review Comment:
Once the JavaDoc below is corrected, this is an unused import.
##########
adapters/base/src/main/java/org/apache/cassandra/sidecar/adapters/base/db/ConnectedClientStatsDatabaseAccessor.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.adapters.base.db;
+
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import com.datastax.driver.core.BoundStatement;
+import com.datastax.driver.core.ResultSet;
+import org.apache.cassandra.sidecar.adapters.base.db.schema.ClientStatsSchema;
+import org.apache.cassandra.sidecar.common.server.CQLSessionProvider;
+import org.apache.cassandra.sidecar.db.DatabaseAccessor;
+
+/**
+ * DataAccessor implementation to read client connection stats from the table represented in {@link ClientStatsSchema}
+ */
+public class ConnectedClientStatsDatabaseAccessor extends DatabaseAccessor<ClientStatsSchema>
+{
+ public ConnectedClientStatsDatabaseAccessor(CQLSessionProvider sessionProvider, ClientStatsSchema tableSchema)
+ {
+ super(tableSchema, sessionProvider);
+ }
+
+ /**
+ * Query for a summary of the client connection stats
+ * @return {@link ConnectedClientStatsSummary} with total connections and counts grouped by user
+ */
+ public ConnectedClientStatsSummary summary()
+ {
+ tableSchema.prepareStatements(session());
+ BoundStatement statement = tableSchema.connectionCountByUser().bind();
+ ResultSet resultSet = execute(statement);
+ return ConnectedClientStatsSummary.from(resultSet);
+ }
+
+ /**
+ * Query for all the client connection metadata with an entry per connection
+ * @return {@link ConnectedClientStats} for each connection
+ */
+ public Set<ConnectedClientStats> stats()
+ {
+ tableSchema.prepareStatements(session());
+ BoundStatement statement = tableSchema.listAll().bind();
+ ResultSet resultSet = execute(statement);
+ return resultSet.all()
+ .stream()
+ .map(ConnectedClientStats::from)
+ .collect(Collectors.toSet());
+ }
Review Comment:
Rather than call `.all()` (which obviously retrieves the whole resultset into memory), how about streaming over the resultSet:
```suggestion
/**
* Query for all the client connection metadata with an entry per connection
* @return {@link ConnectedClientStats} for each connection
*/
public Set<ConnectedClientStats> stats()
{
tableSchema.prepareStatements(session());
BoundStatement statement = tableSchema.listAll().bind();
ResultSet resultSet = execute(statement);
return StreamSupport.stream(resultSet.spliterator(),false)
.map(ConnectedClientStats::from)
.collect(Collectors.toSet());
}
```
##########
server/src/main/java/org/apache/cassandra/sidecar/db/RestoreJobDatabaseAccessor.java:
##########
@@ -55,13 +55,16 @@
public class RestoreJobDatabaseAccessor extends DatabaseAccessor<RestoreJobsSchema>
{
private static final ObjectMapper MAPPER = new ObjectMapper();
+ public final SidecarSchema sidecarSchema;
Review Comment:
Pull this into `SidecarDatabaseAccessor` parent for the all of the classes that use `SidecarSchema`
##########
adapters/base/src/main/java/org/apache/cassandra/sidecar/adapters/base/CassandraMetricsOperations.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.adapters.base;
+
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import org.apache.cassandra.sidecar.adapters.base.db.ConnectedClientStats;
+import org.apache.cassandra.sidecar.adapters.base.db.ConnectedClientStatsDatabaseAccessor;
+import org.apache.cassandra.sidecar.adapters.base.db.ConnectedClientStatsSummary;
+import org.apache.cassandra.sidecar.adapters.base.db.schema.ClientStatsSchema;
+import org.apache.cassandra.sidecar.common.response.ConnectedClientStatsResponse;
+import org.apache.cassandra.sidecar.common.response.data.ClientConnectionEntry;
+import org.apache.cassandra.sidecar.common.server.CQLSessionProvider;
+import org.apache.cassandra.sidecar.common.server.JmxClient;
+import org.apache.cassandra.sidecar.common.server.MetricsOperations;
+
+/**
+ * Default implementation that pulls methods from the Cassandra Metrics Proxy
+ */
+public class CassandraMetricsOperations implements MetricsOperations
+{
+ private ConnectedClientStatsDatabaseAccessor dbAccessor;
Review Comment:
NIT: Should be `final`
##########
adapters/base/src/main/java/org/apache/cassandra/sidecar/adapters/base/db/ConnectedClientStatsSummary.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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.adapters.base.db;
+
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import com.datastax.driver.core.ResultSet;
+import org.apache.cassandra.sidecar.common.DataObjectBuilder;
+import org.apache.cassandra.sidecar.db.DataObjectMappingException;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Representation of a summary of client connections stats
+ */
+public class ConnectedClientStatsSummary
+{
+ public final int totalConnectedClients;
+ public final Map<String, Long> connectionsByUser;
+
+ public static ConnectedClientStatsSummary.Builder builder()
+ {
+ return new ConnectedClientStatsSummary.Builder();
+ }
+
+ public static ConnectedClientStatsSummary from(@NotNull ResultSet resultSet) throws DataObjectMappingException
+ {
+
+ Map<String, Long> resultMap = resultSet.all().stream()
Review Comment:
Same as other call-out of `all`- we should stream the results
##########
adapters/base/src/main/java/org/apache/cassandra/sidecar/adapters/base/db/ConnectedClientStats.java:
##########
@@ -0,0 +1,210 @@
+/*
+ * 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.adapters.base.db;
+
+import java.util.Map;
+
+import com.datastax.driver.core.Row;
+import org.apache.cassandra.sidecar.common.DataObjectBuilder;
+import org.apache.cassandra.sidecar.db.DataObjectMappingException;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Representation of the connected clients metadata
+ */
+public class ConnectedClientStats
+{
+
+ public final String address;
+ public final int port;
+ public final String hostname;
+ public final String username;
+ public final String connectionStage;
+ public final String protocolVersion;
+ public final Map<String, String> clientOptions;
+ public final String driverName;
+ public final String driverVersion;
+ public final boolean sslEnabled;
+ public final String sslProtocol;
+ public final String sslCipherSuite;
+ public final String keyspaceName;
+ public final int requestCount;
+ public final String authenticationMode;
+ public final Map<String, String> authMetadata;
+
+ public static ConnectedClientStats.Builder builder()
+ {
+ return new ConnectedClientStats.Builder();
+ }
+
+ public static ConnectedClientStats from(@NotNull Row row) throws DataObjectMappingException
+ {
+ ConnectedClientStats.Builder builder = new ConnectedClientStats.Builder();
+ builder.address(row.getInet("address").getHostAddress())
+ .port(row.getInt("port"))
+ .hostname(row.getString("hostname"))
+ .username(row.getString("username"))
+ .connectionStage(row.getString("connection_stage"))
+ .protocolVersion(Integer.toString(row.getInt("protocol_version")))
+ .driverName(row.getString("driver_name"))
+ .driverVersion(row.getString("driver_version"))
+ .sslEnabled(row.getBool("ssl_enabled"))
+ .sslProtocol(row.getString("ssl_protocol"))
+ .sslCipherSuite(row.getString("ssl_cipher_suite"));
Review Comment:
There are several members of the ConnetedClientStats class missing here, and it appears the corresponding Builder methods are never called anywhere else. Why do we have all of the extra fields?
```
this.keyspaceName
this.authenticationMode
this.requestCount
this.authMetadata
this.clientOptions
```
Further, it seems odd to create a Builder in a `from` method that ends up just calling the constructor anyway - you could just pass a `row` to a `ConnectedClientStats` constructor and pull out the data directly in the constructor, which allows you to create one of these without the builder at all (and one less object per row of junk to create). This may become significant when you have thousands of connected clients, which we often do. Maybe this was the previously-established pattern in the data access layer, but if we're talking about up to 20k connected clients per request, creating a bunch of extra junk (even if it's short-lived and gets GCed out of eden space) doesn't make a lot of sense if we can avoid it quite easily.
##########
server/src/main/java/org/apache/cassandra/sidecar/db/schema/SidecarSchema.java:
##########
@@ -83,8 +80,8 @@ private void configureSidecarServerEventListeners()
{
EventBus eventBus = vertx.eventBus();
- eventBus.localConsumer(ON_CASSANDRA_CQL_READY.address(), message -> startSidecarSchemaInitializer());
- eventBus.localConsumer(ON_SERVER_STOP.address(), message -> cancelTimer(initializationTimerId.get()));
+ eventBus.localConsumer(SidecarServerEvents.ON_CASSANDRA_CQL_READY.address(), message -> startSidecarSchemaInitializer());
Review Comment:
TODO: Should we make these back to static imports?
--
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
Re: [PR] CASSANDRASC-144 Introduces sidecar endpoint to vend statistics associ… [cassandra-sidecar]
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #135:
URL: https://github.com/apache/cassandra-sidecar/pull/135#discussion_r1817184599
##########
server/src/test/integration/org/apache/cassandra/sidecar/routes/ConnectedConnectedClientStatsHandlerIntegrationTest.java:
##########
@@ -0,0 +1,193 @@
+/*
+ * 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.routes;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import org.junit.jupiter.api.extension.ExtendWith;
+
+import com.datastax.driver.core.Session;
+import io.vertx.core.buffer.Buffer;
+import io.vertx.ext.web.client.HttpResponse;
+import io.vertx.ext.web.client.predicate.ResponsePredicate;
+import io.vertx.junit5.VertxExtension;
+import io.vertx.junit5.VertxTestContext;
+import org.apache.cassandra.sidecar.common.response.ConnectedClientStatsResponse;
+import org.apache.cassandra.sidecar.common.response.data.ClientConnectionEntry;
+import org.apache.cassandra.sidecar.testing.IntegrationTestBase;
+import org.apache.cassandra.sidecar.utils.SimpleCassandraVersion;
+import org.apache.cassandra.testing.CassandraIntegrationTest;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Test the client-stats endpoint with cassandra container.
+ */
+@ExtendWith(VertxExtension.class)
+public class ConnectedConnectedClientStatsHandlerIntegrationTest extends IntegrationTestBase
Review Comment:
NIT: `ConnectedConnected`?
```suggestion
public class ConnectedClientStatsHandlerIntegrationTest extends IntegrationTestBase
```
--
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
Re: [PR] CASSANDRASC-144 Introduces sidecar endpoint to vend statistics associ… [cassandra-sidecar]
Posted by "bbotella (via GitHub)" <gi...@apache.org>.
bbotella commented on code in PR #135:
URL: https://github.com/apache/cassandra-sidecar/pull/135#discussion_r1745598507
##########
src/test/integration/org/apache/cassandra/sidecar/routes/ClientStatsHandlerIntegrationTest.java:
##########
@@ -0,0 +1,170 @@
+/*
+ * 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.routes;
+
+import java.util.Collections;
+import java.util.Map;
+
+import org.junit.jupiter.api.extension.ExtendWith;
+
+import io.vertx.core.buffer.Buffer;
+import io.vertx.ext.web.client.HttpResponse;
+import io.vertx.ext.web.client.predicate.ResponsePredicate;
+import io.vertx.junit5.VertxExtension;
+import io.vertx.junit5.VertxTestContext;
+import org.apache.cassandra.sidecar.common.response.ClientStatsResponse;
+import org.apache.cassandra.sidecar.testing.IntegrationTestBase;
+import org.apache.cassandra.testing.CassandraIntegrationTest;
+import org.apache.cassandra.testing.CassandraTestContext;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Test the client-stats endpoint with cassandra container.
+ */
+@ExtendWith(VertxExtension.class)
+public class ClientStatsHandlerIntegrationTest extends IntegrationTestBase
+{
+
+ @CassandraIntegrationTest
+ void retrieveClientStatsDefault(VertxTestContext context, CassandraTestContext cassandraTestContext)
+ throws Exception
+ {
+ String testRoute = "/api/v1/storage/client-stats";
+ testWithClient(context, client -> {
+ client.get(server.actualPort(), "127.0.0.1", testRoute)
+ .expect(ResponsePredicate.SC_OK)
+ .send(context.succeeding(response -> {
+ assertClientStatsResponse(response, Collections.emptyMap(), cassandraTestContext.version.toString());
+ context.completeNow();
+ }));
+ });
+ }
+
+ @CassandraIntegrationTest
+ void retrieveClientStatsWithClientOptions(VertxTestContext context, CassandraTestContext cassandraTestContext)
+ throws Exception
+ {
+ Map<String, Boolean> params = Collections.singletonMap("client-options", true);
+ String testRoute = "/api/v1/storage/client-stats?client-options=true";
+ testWithClient(context, client -> {
+ client.get(server.actualPort(), "127.0.0.1", testRoute)
+ .expect(ResponsePredicate.SC_OK)
+ .send(context.succeeding(response -> {
+ assertClientStatsResponse(response, params, cassandraTestContext.version.toString());
+ context.completeNow();
+ }));
+ });
+ }
+
+ @CassandraIntegrationTest
+ void retrieveClientStatsByProtocol(VertxTestContext context, CassandraTestContext cassandraTestContext)
+ throws Exception
+ {
+ Map<String, Boolean> params = Collections.singletonMap("by-protocol", true);
+ String testRoute = "/api/v1/storage/client-stats?by-protocol=true";
+ testWithClient(context, client -> {
+ client.get(server.actualPort(), "127.0.0.1", testRoute)
Review Comment:
Can we get the actual `127.0.0.1` from `server` instead of hardcoded?
##########
src/test/integration/org/apache/cassandra/sidecar/routes/ClientStatsHandlerIntegrationTest.java:
##########
@@ -0,0 +1,170 @@
+/*
+ * 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.routes;
+
+import java.util.Collections;
+import java.util.Map;
+
+import org.junit.jupiter.api.extension.ExtendWith;
+
+import io.vertx.core.buffer.Buffer;
+import io.vertx.ext.web.client.HttpResponse;
+import io.vertx.ext.web.client.predicate.ResponsePredicate;
+import io.vertx.junit5.VertxExtension;
+import io.vertx.junit5.VertxTestContext;
+import org.apache.cassandra.sidecar.common.response.ClientStatsResponse;
+import org.apache.cassandra.sidecar.testing.IntegrationTestBase;
+import org.apache.cassandra.testing.CassandraIntegrationTest;
+import org.apache.cassandra.testing.CassandraTestContext;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Test the client-stats endpoint with cassandra container.
+ */
+@ExtendWith(VertxExtension.class)
+public class ClientStatsHandlerIntegrationTest extends IntegrationTestBase
+{
+
+ @CassandraIntegrationTest
+ void retrieveClientStatsDefault(VertxTestContext context, CassandraTestContext cassandraTestContext)
+ throws Exception
+ {
+ String testRoute = "/api/v1/storage/client-stats";
+ testWithClient(context, client -> {
+ client.get(server.actualPort(), "127.0.0.1", testRoute)
+ .expect(ResponsePredicate.SC_OK)
+ .send(context.succeeding(response -> {
+ assertClientStatsResponse(response, Collections.emptyMap(), cassandraTestContext.version.toString());
+ context.completeNow();
+ }));
+ });
+ }
+
+ @CassandraIntegrationTest
+ void retrieveClientStatsWithClientOptions(VertxTestContext context, CassandraTestContext cassandraTestContext)
+ throws Exception
+ {
+ Map<String, Boolean> params = Collections.singletonMap("client-options", true);
+ String testRoute = "/api/v1/storage/client-stats?client-options=true";
+ testWithClient(context, client -> {
+ client.get(server.actualPort(), "127.0.0.1", testRoute)
+ .expect(ResponsePredicate.SC_OK)
+ .send(context.succeeding(response -> {
+ assertClientStatsResponse(response, params, cassandraTestContext.version.toString());
+ context.completeNow();
+ }));
+ });
+ }
+
+ @CassandraIntegrationTest
+ void retrieveClientStatsByProtocol(VertxTestContext context, CassandraTestContext cassandraTestContext)
+ throws Exception
+ {
+ Map<String, Boolean> params = Collections.singletonMap("by-protocol", true);
+ String testRoute = "/api/v1/storage/client-stats?by-protocol=true";
+ testWithClient(context, client -> {
+ client.get(server.actualPort(), "127.0.0.1", testRoute)
+ .expect(ResponsePredicate.SC_OK)
+ .send(context.succeeding(response -> {
+ assertClientStatsResponse(response, params, cassandraTestContext.version.toString());
+ context.completeNow();
+ }));
+ });
+ }
+
+ @CassandraIntegrationTest
+ void retrieveClientStatsVerbose(VertxTestContext context, CassandraTestContext cassandraTestContext)
+ throws Exception
+ {
+ Map<String, Boolean> params = Collections.singletonMap("verbose", true);
+ String testRoute = "/api/v1/storage/client-stats?verbose=true";
Review Comment:
Maybe add tests with more than one param?
##########
src/main/java/org/apache/cassandra/sidecar/routes/ClientStatsHandler.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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.routes;
+
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import com.datastax.driver.core.Metadata;
+import com.google.inject.Inject;
+import io.vertx.core.http.HttpServerRequest;
+import io.vertx.core.net.SocketAddress;
+import io.vertx.ext.web.RoutingContext;
+import org.apache.cassandra.sidecar.cluster.CassandraAdapterDelegate;
+import org.apache.cassandra.sidecar.common.server.MetricsOperations;
+import org.apache.cassandra.sidecar.concurrent.ExecutorPools;
+import org.apache.cassandra.sidecar.utils.InstanceMetadataFetcher;
+
+import static org.apache.cassandra.sidecar.utils.HttpExceptions.cassandraServiceUnavailable;
+
+/**
+ * Handler for retrieving client stats
+ */
+public class ClientStatsHandler extends AbstractHandler<Void>
+{
+ /**
+ * Constructs a handler with the provided {@code metadataFetcher}
+ *
+ * @param metadataFetcher the metadata fetcher
+ * @param executorPools executor pools for blocking executions
+ */
+ @Inject
+ protected ClientStatsHandler(InstanceMetadataFetcher metadataFetcher, ExecutorPools executorPools)
+ {
+ super(metadataFetcher, executorPools, null);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public void handleInternal(RoutingContext context,
+ HttpServerRequest httpRequest,
+ String host,
+ SocketAddress remoteAddress,
+ Void request)
+ {
+
+ CassandraAdapterDelegate delegate = metadataFetcher.delegate(host);
+ if (delegate == null)
+ {
+ context.fail(cassandraServiceUnavailable());
+ return;
+ }
+ MetricsOperations operations = delegate.metricsOperations();
+ Metadata metadata = delegate.metadata();
+ if (operations == null || metadata == null)
+ {
+ context.fail(cassandraServiceUnavailable());
+ return;
+ }
+
+ Map<String, String> params = httpRequest.params().entries()
Review Comment:
Do we need some kind of validation here? I see that we are using `Boolean.valueOf` on the actual delegate. That means that if, for example, `verbose=foo` param is passed, it would be evaluated to false. Is that the intended behavior?
Similar comment for the fact that we can pass in arbitrary params (that will be ignored). Is that something we want?
--
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
Re: [PR] CASSANDRASC-144 Introduces sidecar endpoint to vend statistics associ… [cassandra-sidecar]
Posted by "arjunashok (via GitHub)" <gi...@apache.org>.
arjunashok commented on code in PR #135:
URL: https://github.com/apache/cassandra-sidecar/pull/135#discussion_r1775610251
##########
client-common/src/main/java/org/apache/cassandra/sidecar/common/ApiEndpointsV1.java:
##########
@@ -109,6 +109,8 @@ public final class ApiEndpointsV1
public static final String ABORT_RESTORE_JOB_ROUTE = RESTORE_JOB_ROUTE + ABORT;
public static final String RESTORE_JOB_PROGRESS_ROUTE = RESTORE_JOB_ROUTE + PROGRESS;
+ public static final String CLIENT_STATS_ROUTE = API_V1 + CASSANDRA + "/client-stats";
Review Comment:
I would like to have additional "context" in the URI between `cassandra` and the operation - `client-stats`. This was `storage` previously. Reviewers - Lmk your thoughts.
--
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
Re: [PR] CASSANDRASC-144 Introduces sidecar endpoint to vend statistics associ… [cassandra-sidecar]
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug merged PR #135:
URL: https://github.com/apache/cassandra-sidecar/pull/135
--
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
Re: [PR] CASSANDRASC-144 Introduces sidecar endpoint to vend statistics associ… [cassandra-sidecar]
Posted by "arjunashok (via GitHub)" <gi...@apache.org>.
arjunashok commented on code in PR #135:
URL: https://github.com/apache/cassandra-sidecar/pull/135#discussion_r1775613779
##########
adapters/base/src/main/java/org/apache/cassandra/sidecar/adapters/base/JmxGaugeOperations.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.adapters.base;
+
+/**
+ * An interface that pulls methods from the Cassandra Gauge Metrics Proxy
+ */
+public interface JmxGaugeOperations
Review Comment:
Not needed anymore. Will remove.
--
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