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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
