bbotella commented on code in PR #4067:
URL: https://github.com/apache/cassandra/pull/4067#discussion_r2037551003
##########
src/java/org/apache/cassandra/db/monitoring/MonitoringTask.java:
##########
@@ -171,12 +194,27 @@ boolean logSlowOperations(long approxCurrentTimeNanos)
long approxElapsedNanos = approxCurrentTimeNanos -
approxLastLogTimeNanos;
noSpamLogger.info("Some operations were slow, details available at
debug level (debug.log)");
- if (logger.isDebugEnabled())
- logger.debug("{} operations were slow in the last {}
msecs:{}{}",
- slowOperations.num(),
- NANOSECONDS.toMillis(approxElapsedNanos),
- LINE_SEPARATOR,
- slowOperations.getLogMessage());
+ if (slowOperationsLogger.isDebugEnabled())
+ {
+ if (slowOperationsLoggedToVirtualTable)
+ {
+ // This is the crux of the patch for appending to vtable.
+ // Because we can send only String's to debug method (or
objects, on which toString()
Review Comment:
```suggestion
// Because we can send only Strings to debug method (or
objects, on which toString()
```
##########
src/java/org/apache/cassandra/db/monitoring/MonitoringTask.java:
##########
@@ -388,13 +534,21 @@ public String getLogMessage()
/**
* An operation (query) that was reported as slow.
*/
- private final static class SlowOperation extends Operation
+ @VisibleForTesting
+ public final static class SlowOperation extends Operation
{
- SlowOperation(Monitorable operation, long failedAt)
+ // purely for deserialisation purposes
Review Comment:
```suggestion
// purely for deserialization purposes
```
##########
src/java/org/apache/cassandra/db/monitoring/MonitoringTask.java:
##########
@@ -328,31 +379,126 @@ protected abstract static class Operation
* this is set lazily as it takes time to build the query CQL */
private String name;
+ /**
+ * creation time of this Operation object, in ms,
+ * this is different from operation's creationTimeNanos
+ * which does not follow wall clock and is useless for
+ * reporting purposes e.g. in virtual tables
+ */
+ private final long timestamp;
+
+ // optional keyspace and table this operation acts on
+ // used upon deserialisation
Review Comment:
```suggestion
// used upon deserialization
```
##########
src/java/org/apache/cassandra/utils/logging/AbstractVirtualTableAppender.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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.utils.logging;
+
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Optional;
+
+import ch.qos.logback.classic.spi.LoggingEvent;
+import ch.qos.logback.core.AppenderBase;
+import org.apache.cassandra.db.virtual.AbstractLoggerVirtualTable;
+import org.apache.cassandra.db.virtual.SlowQueriesTable;
+import org.apache.cassandra.db.virtual.VirtualKeyspace;
+import org.apache.cassandra.db.virtual.VirtualKeyspaceRegistry;
+import org.apache.cassandra.db.virtual.VirtualTable;
+
+import static org.apache.cassandra.schema.SchemaConstants.VIRTUAL_VIEWS;
+
+public abstract class AbstractVirtualTableAppender extends
AppenderBase<LoggingEvent>
+{
+ private final int defaultRows;
+
+ protected AbstractVirtualTableAppender(int defaultRows)
+ {
+ this.defaultRows = defaultRows;
+ }
+
+ // for holding messages until virtual registry contains logs virtual table
+ // as it takes some time during startup of a node to initialise virtual
tables but messages are
+ // logged already
+ protected final List<LoggingEvent> messageBuffer = new LinkedList<>();
+
+ protected <T> T getVirtualTable(Class<T> vtableClass, String tableName)
+ {
+ VirtualKeyspace keyspace =
VirtualKeyspaceRegistry.instance.getKeyspaceNullable(VIRTUAL_VIEWS);
+
+ if (keyspace == null)
+ return null;
+
+ Optional<VirtualTable> virtualTable = keyspace.tables()
+ .stream()
+ .filter(vt ->
vt.name().equals(tableName))
+ .findFirst();
+
+ if (virtualTable.isEmpty())
+ return null;
+
+ VirtualTable vt = virtualTable.get();
+
+ if (!vt.getClass().equals(vtableClass))
+ throw new IllegalStateException(String.format("Virtual table %s.%s
is not backed by an instance of %s but by %s",
+ VIRTUAL_VIEWS,
+ tableName,
+
vtableClass.getName(),
+
vt.getClass().getName()));
+
+ return (T) vt;
+ }
+
+ /**
+ * This method adds an event to virtual table, when present.
+ * When vtable is null, we will attempt to find it among registered ones.
Then not found, we add it to internal
+ * buffer for later processing. This might happen e.g. for logging tables
when log events
+ * were appended via logging framework sooner than registration of virtual
tables was done so after they are registered,
+ * they would miss logging events happened before being so.
+ *
+ * @param vtable vtable to append to
+ * @param event event to append to
+ * @param tableName table name of virtual table to append to
+ * @return vtable or when null, found vtable
+ */
+ protected AbstractLoggerVirtualTable<?>
appendToVirtualTable(AbstractLoggerVirtualTable<?> vtable, LoggingEvent event,
String tableName)
+ {
+ AbstractLoggerVirtualTable<?> foundVtable;
+ if (vtable == null)
+ {
+ foundVtable = getVirtualTable(SlowQueriesTable.class, tableName);
+ if (foundVtable == null)
+ addToBuffer(event);
+ else
+ foundVtable.add(event);
+ }
+ else
+ {
+ foundVtable = vtable;
+ vtable.add(event);
+ }
+
+ return foundVtable;
+ }
+
+ @Override
+ public void stop()
+ {
+ messageBuffer.clear();
+ super.stop();
+ }
+
+ /**
+ * Flushes all log entries which were appended before virtual table was
registered.
+ *
+ * @see org.apache.cassandra.service.CassandraDaemon#setupVirtualKeyspaces
+ */
+ public void flushBuffer(Class<? extends AbstractLoggerVirtualTable<?>>
vtableClass, String tableName)
+ {
+ Optional.ofNullable(getVirtualTable(vtableClass,
tableName)).ifPresent(vtable -> {
+ messageBuffer.forEach(vtable::add);
+ messageBuffer.clear();
+ });
+ }
+
+ protected void addToBuffer(LoggingEvent eventObject)
+ {
+ // we restrict how many logging events we can put into buffer,
+ // so we are not growing without any bound when things go south
+ if (messageBuffer.size() < defaultRows)
Review Comment:
Should we add a logger debug if message is not added to the buffer due to
buffer size?
##########
test/unit/org/apache/cassandra/db/virtual/AbstractLoggerVirtualTableTest.java:
##########
@@ -0,0 +1,168 @@
+/*
+ * 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.time.Instant;
+import java.util.Date;
+import java.util.Iterator;
+import java.util.LinkedList;
+import java.util.List;
+
+import com.google.common.collect.ImmutableList;
+import org.junit.Test;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.spi.LoggingEvent;
+import com.datastax.driver.core.Row;
+import org.apache.cassandra.cql3.CQLTester;
+import org.apache.cassandra.db.DataRange;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public abstract class AbstractLoggerVirtualTableTest<U> extends CQLTester
+{
+ protected final String keyspace = createKeyspaceName();
+
+ protected AbstractLoggerVirtualTable<U> table;
+
+ @Test
+ public void testTruncate()
+ {
+ registerTable();
+
+ int numberOfRows = 100;
+ List<LoggingEvent> loggingEvents = getLoggingEvents(numberOfRows);
+ loggingEvents.forEach(table::add);
+
+ execute(query("truncate %s"));
+
+ assertTrue(executeNet(query("select timestamp from
%s")).all().isEmpty());
Review Comment:
Can we use `assertEmpty`?
##########
src/java/org/apache/cassandra/db/virtual/AbstractLoggerVirtualTable.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.Collections;
+import java.util.LinkedList;
+import java.util.List;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import ch.qos.logback.classic.spi.LoggingEvent;
+import org.apache.cassandra.schema.TableMetadata;
+
+/**
+ * This table is inherently limited on number of rows it can hold.
+ *
+ * @param <U> type parameter saying what object is stored in internal bounded
list for query purposes
+ */
+public abstract class AbstractLoggerVirtualTable<U> extends
AbstractMutableVirtualTable
+{
+ private static final Logger logger =
LoggerFactory.getLogger(AbstractLoggerVirtualTable.class);
+
+ protected final List<U> buffer;
+
+ @VisibleForTesting
+ protected static int resolveBufferSize(int wantedSize, int min, int max,
int defaultSize)
+ {
+ return (wantedSize < min || wantedSize > max) ? defaultSize :
wantedSize;
+ }
+
+ protected AbstractLoggerVirtualTable(TableMetadata metadata, int maxSize)
+ {
+ super(metadata);
+ this.buffer = BoundedLinkedList.create(maxSize);
+ logger.debug("capacity of virtual table {} is set to be at most {}
rows", metadata().toString(), maxSize);
+ }
+
+ public void add(LoggingEvent event)
+ {
+ List<U> messages = getMessages(event);
+ if (messages != null)
+ {
+ // specifically calling buffer.add to reach BoundedLinkedList's add
+ // instead of linked list's addAll
+ for (U message : messages)
+ buffer.add(message);
+ }
+ }
+
+ public abstract List<U> getMessages(LoggingEvent event);
+
+ @Override
+ public void truncate()
+ {
+ buffer.clear();
Review Comment:
Is it worth adding a debug logger message stating that the table has been
truncated?
--
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]
