You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/01/29 22:18:22 UTC
[GitHub] [cassandra] azotcsit commented on a change in pull request #1372: CASSANDRA-17225 Add batch_metrics virtual table
azotcsit commented on a change in pull request #1372:
URL: https://github.com/apache/cassandra/pull/1372#discussion_r795093873
##########
File path: test/unit/org/apache/cassandra/db/virtual/BatchMetricsTableTest.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.db.virtual;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import com.google.common.collect.ImmutableList;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import com.codahale.metrics.Snapshot;
+import com.datastax.driver.core.ResultSet;
+import org.apache.cassandra.cql3.CQLTester;
+import org.apache.cassandra.cql3.statements.BatchStatement;
+import org.apache.cassandra.metrics.BatchMetrics;
+
+import static java.lang.String.format;
+import static org.junit.Assert.assertEquals;
+
+public class BatchMetricsTableTest extends CQLTester
+{
+ private static final String KS_NAME = "vts";
+
+ @BeforeClass
+ public static void setUpClass()
+ {
+ CQLTester.setUpClass();
+ }
+
+ @Before
+ public void config()
+ {
+ BatchMetricsTable table = new BatchMetricsTable(KS_NAME);
+ VirtualKeyspaceRegistry.instance.register(new VirtualKeyspace(KS_NAME, ImmutableList.of(table)));
+ }
+
+ @Test
+ public void testSelectAll() throws Throwable
+ {
+ BatchMetrics metrics = BatchStatement.metrics;
+
+ for (int i = 0; i < 10; i++)
+ {
+ metrics.partitionsPerLoggedBatch.update(i);
+ metrics.partitionsPerUnloggedBatch.update(i + 10);
+ metrics.partitionsPerCounterBatch.update(i * 10);
+ }
+
+ ResultSet result = executeNet(format("SELECT * FROM %s.batch_metrics", KS_NAME));
+ assertEquals(5, result.getColumnDefinitions().size());
+ AtomicInteger rowCount = new AtomicInteger(0);
+ result.forEach(r -> {
+ Snapshot snapshot = getExpectedHistogramSnapshot(metrics, r.getString("name"));
+ assertEquals(snapshot.getMedian(), r.getDouble("p50th"), 0.0);
+ assertEquals(snapshot.get99thPercentile(), r.getDouble("p99th"), 0.0);
Review comment:
Shouldn't we have `p999th` and `max` here tested as well?
##########
File path: src/java/org/apache/cassandra/db/virtual/BatchMetricsTable.java
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.db.virtual;
+
+import com.codahale.metrics.Snapshot;
+import org.apache.cassandra.cql3.statements.BatchStatement;
+import org.apache.cassandra.db.marshal.DoubleType;
+import org.apache.cassandra.db.marshal.LongType;
+import org.apache.cassandra.db.marshal.UTF8Type;
+import org.apache.cassandra.dht.LocalPartitioner;
+import org.apache.cassandra.metrics.BatchMetrics;
+import org.apache.cassandra.schema.TableMetadata;
+
+public class BatchMetricsTable extends AbstractVirtualTable
Review comment:
It would be nice to mention this new table in the documentation: https://github.com/apache/cassandra/blob/trunk/doc/modules/cassandra/pages/new/virtualtables.adoc
##########
File path: src/java/org/apache/cassandra/db/virtual/BatchMetricsTable.java
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.db.virtual;
+
+import com.codahale.metrics.Snapshot;
+import org.apache.cassandra.cql3.statements.BatchStatement;
+import org.apache.cassandra.db.marshal.DoubleType;
+import org.apache.cassandra.db.marshal.LongType;
+import org.apache.cassandra.db.marshal.UTF8Type;
+import org.apache.cassandra.dht.LocalPartitioner;
+import org.apache.cassandra.metrics.BatchMetrics;
+import org.apache.cassandra.schema.TableMetadata;
+
+public class BatchMetricsTable extends AbstractVirtualTable
+{
+
+ private static final String PARTITIONS_PER_LOGGED_BATCH = "partitions_per_logged_batch";
+ private static final String PARTITIONS_PER_UNLOGGED_BATCH = "partitions_per_unlogged_batch";
+ private static final String PARTITIONS_PER_COUNTER_BATCH = "partitions_per_counter_batch";
+ private final static String P50 = "p50th";
+ private final static String P99 = "p99th";
+ private final static String P999 = "p999th";
Review comment:
The ticket description mentions `TableMetricTables` as a reference. p999 is not used there. Do we really want to expose it?
@burmanm @ekaterinadimitrova2 WDYT?
##########
File path: test/unit/org/apache/cassandra/db/virtual/BatchMetricsTableTest.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.db.virtual;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import com.google.common.collect.ImmutableList;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import com.codahale.metrics.Snapshot;
+import com.datastax.driver.core.ResultSet;
+import org.apache.cassandra.cql3.CQLTester;
+import org.apache.cassandra.cql3.statements.BatchStatement;
+import org.apache.cassandra.metrics.BatchMetrics;
+
+import static java.lang.String.format;
+import static org.junit.Assert.assertEquals;
+
+public class BatchMetricsTableTest extends CQLTester
+{
+ private static final String KS_NAME = "vts";
+
+ @BeforeClass
+ public static void setUpClass()
+ {
+ CQLTester.setUpClass();
+ }
+
+ @Before
+ public void config()
+ {
+ BatchMetricsTable table = new BatchMetricsTable(KS_NAME);
+ VirtualKeyspaceRegistry.instance.register(new VirtualKeyspace(KS_NAME, ImmutableList.of(table)));
+ }
+
+ @Test
+ public void testSelectAll() throws Throwable
+ {
+ BatchMetrics metrics = BatchStatement.metrics;
+
+ for (int i = 0; i < 10; i++)
+ {
+ metrics.partitionsPerLoggedBatch.update(i);
+ metrics.partitionsPerUnloggedBatch.update(i + 10);
+ metrics.partitionsPerCounterBatch.update(i * 10);
+ }
+
+ ResultSet result = executeNet(format("SELECT * FROM %s.batch_metrics", KS_NAME));
+ assertEquals(5, result.getColumnDefinitions().size());
+ AtomicInteger rowCount = new AtomicInteger(0);
+ result.forEach(r -> {
+ Snapshot snapshot = getExpectedHistogramSnapshot(metrics, r.getString("name"));
+ assertEquals(snapshot.getMedian(), r.getDouble("p50th"), 0.0);
Review comment:
My expectation was to see smth non zero here because you call `update` earlier in the test. What am I missing?
##########
File path: src/java/org/apache/cassandra/db/virtual/BatchMetricsTable.java
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.db.virtual;
+
+import com.codahale.metrics.Snapshot;
+import org.apache.cassandra.cql3.statements.BatchStatement;
+import org.apache.cassandra.db.marshal.DoubleType;
+import org.apache.cassandra.db.marshal.LongType;
+import org.apache.cassandra.db.marshal.UTF8Type;
+import org.apache.cassandra.dht.LocalPartitioner;
+import org.apache.cassandra.metrics.BatchMetrics;
+import org.apache.cassandra.schema.TableMetadata;
+
+public class BatchMetricsTable extends AbstractVirtualTable
+{
+
+ private static final String PARTITIONS_PER_LOGGED_BATCH = "partitions_per_logged_batch";
+ private static final String PARTITIONS_PER_UNLOGGED_BATCH = "partitions_per_unlogged_batch";
+ private static final String PARTITIONS_PER_COUNTER_BATCH = "partitions_per_counter_batch";
+ private final static String P50 = "p50th";
+ private final static String P99 = "p99th";
+ private final static String P999 = "p999th";
+ private final static String MAX = "max";
+
+ BatchMetricsTable(String keyspace)
+ {
+ super(TableMetadata.builder(keyspace, "batch_metrics")
+ .kind(TableMetadata.Kind.VIRTUAL)
Review comment:
Let's add a table description. Example: https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/db/virtual/RolesCacheKeysTable.java#L33
Smth like: "metrics on batch statements"
--
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