[GitHub] [cassandra] dcapwell opened a new pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

dcapwell opened a new pull request #748:
URL: https://github.com/apache/cassandra/pull/748


   


----------------------------------------------------------------
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.

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

[GitHub] [cassandra] yifan-c commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

yifan-c commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r486552432



##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,23 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()
+    {
+        return true;
+    }
+
+    @Override
+    public LogAction logs()
+    {
+        // ./build/test/logs/${cassandra.testtag}/${suitename}/${cluster_id}/${instance_id}/system.log

Review comment:
       nit: add reference to the `test/conf/logback-dtest.xml` file so that it is more clear to know where the path comes from

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -18,13 +18,25 @@
 
 package org.apache.cassandra.distributed.test;
 
+import java.io.File;

Review comment:
       nit: remove unused 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.

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

[GitHub] [cassandra] ifesdjeen commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

ifesdjeen commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r487093544



##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       Nit: maybe `logsSupported` ? I would say `areLogsSupported` but that also doesn't sound very good.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/FileLogAction.java
##########
@@ -0,0 +1,117 @@
+package org.apache.cassandra.distributed.impl;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.io.UncheckedIOException;
+import java.util.Objects;
+import java.util.function.Predicate;
+
+import com.google.common.io.Closeables;
+
+import org.apache.cassandra.utils.AbstractIterator;
+import org.apache.cassandra.distributed.api.LogAction;
+import org.apache.cassandra.distributed.api.LineIterator;
+
+public class FileLogAction implements LogAction
+{
+    private final File file;
+
+    public FileLogAction(File file)
+    {
+        this.file = Objects.requireNonNull(file);
+    }
+
+    @Override
+    public long mark()
+    {
+        return file.length();
+    }
+
+    @Override
+    public LineIterator matching(long startPosition, Predicate<String> fn)

Review comment:
       Nit: maybe just `match`? 

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       Nit: predend?

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Nit: together?
   
   Just to make sure I understand: we can check for `fail withouot fail`, we just can't do it with the same `watchFor`, right?

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       Nit: maybe `logsSupported` ? I would say `areLogsSupported` but that also doesn't sound very good.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/FileLogAction.java
##########
@@ -0,0 +1,117 @@
+package org.apache.cassandra.distributed.impl;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.io.UncheckedIOException;
+import java.util.Objects;
+import java.util.function.Predicate;
+
+import com.google.common.io.Closeables;
+
+import org.apache.cassandra.utils.AbstractIterator;
+import org.apache.cassandra.distributed.api.LogAction;
+import org.apache.cassandra.distributed.api.LineIterator;
+
+public class FileLogAction implements LogAction
+{
+    private final File file;
+
+    public FileLogAction(File file)
+    {
+        this.file = Objects.requireNonNull(file);
+    }
+
+    @Override
+    public long mark()
+    {
+        return file.length();
+    }
+
+    @Override
+    public LineIterator matching(long startPosition, Predicate<String> fn)

Review comment:
       Nit: maybe just `match`? 

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       Nit: predend?

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Nit: together?
   
   Just to make sure I understand: we can check for `fail withouot fail`, we just can't do it with the same `watchFor`, right?

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       Nit: maybe `logsSupported` ? I would say `areLogsSupported` but that also doesn't sound very good.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/FileLogAction.java
##########
@@ -0,0 +1,117 @@
+package org.apache.cassandra.distributed.impl;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.io.UncheckedIOException;
+import java.util.Objects;
+import java.util.function.Predicate;
+
+import com.google.common.io.Closeables;
+
+import org.apache.cassandra.utils.AbstractIterator;
+import org.apache.cassandra.distributed.api.LogAction;
+import org.apache.cassandra.distributed.api.LineIterator;
+
+public class FileLogAction implements LogAction
+{
+    private final File file;
+
+    public FileLogAction(File file)
+    {
+        this.file = Objects.requireNonNull(file);
+    }
+
+    @Override
+    public long mark()
+    {
+        return file.length();
+    }
+
+    @Override
+    public LineIterator matching(long startPosition, Predicate<String> fn)

Review comment:
       Nit: maybe just `match`? 

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       Nit: predend?

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Nit: together?
   
   Just to make sure I understand: we can check for `fail withouot fail`, we just can't do it with the same `watchFor`, right?

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       Nit: maybe `logsSupported` ? I would say `areLogsSupported` but that also doesn't sound very good.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/FileLogAction.java
##########
@@ -0,0 +1,117 @@
+package org.apache.cassandra.distributed.impl;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.io.UncheckedIOException;
+import java.util.Objects;
+import java.util.function.Predicate;
+
+import com.google.common.io.Closeables;
+
+import org.apache.cassandra.utils.AbstractIterator;
+import org.apache.cassandra.distributed.api.LogAction;
+import org.apache.cassandra.distributed.api.LineIterator;
+
+public class FileLogAction implements LogAction
+{
+    private final File file;
+
+    public FileLogAction(File file)
+    {
+        this.file = Objects.requireNonNull(file);
+    }
+
+    @Override
+    public long mark()
+    {
+        return file.length();
+    }
+
+    @Override
+    public LineIterator matching(long startPosition, Predicate<String> fn)

Review comment:
       Nit: maybe just `match`? 

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       Nit: predend?

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Nit: together?
   
   Just to make sure I understand: we can check for `fail withouot fail`, we just can't do it with the same `watchFor`, right?

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       Nit: maybe `logsSupported` ? I would say `areLogsSupported` but that also doesn't sound very good.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/FileLogAction.java
##########
@@ -0,0 +1,117 @@
+package org.apache.cassandra.distributed.impl;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.io.UncheckedIOException;
+import java.util.Objects;
+import java.util.function.Predicate;
+
+import com.google.common.io.Closeables;
+
+import org.apache.cassandra.utils.AbstractIterator;
+import org.apache.cassandra.distributed.api.LogAction;
+import org.apache.cassandra.distributed.api.LineIterator;
+
+public class FileLogAction implements LogAction
+{
+    private final File file;
+
+    public FileLogAction(File file)
+    {
+        this.file = Objects.requireNonNull(file);
+    }
+
+    @Override
+    public long mark()
+    {
+        return file.length();
+    }
+
+    @Override
+    public LineIterator matching(long startPosition, Predicate<String> fn)

Review comment:
       Nit: maybe just `match`? 

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       Nit: predend?

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Nit: together?
   
   Just to make sure I understand: we can check for `fail withouot fail`, we just can't do it with the same `watchFor`, right?

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       Nit: maybe `logsSupported` ? I would say `areLogsSupported` but that also doesn't sound very good.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/FileLogAction.java
##########
@@ -0,0 +1,117 @@
+package org.apache.cassandra.distributed.impl;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.io.UncheckedIOException;
+import java.util.Objects;
+import java.util.function.Predicate;
+
+import com.google.common.io.Closeables;
+
+import org.apache.cassandra.utils.AbstractIterator;
+import org.apache.cassandra.distributed.api.LogAction;
+import org.apache.cassandra.distributed.api.LineIterator;
+
+public class FileLogAction implements LogAction
+{
+    private final File file;
+
+    public FileLogAction(File file)
+    {
+        this.file = Objects.requireNonNull(file);
+    }
+
+    @Override
+    public long mark()
+    {
+        return file.length();
+    }
+
+    @Override
+    public LineIterator matching(long startPosition, Predicate<String> fn)

Review comment:
       Nit: maybe just `match`? 

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       Nit: predend?

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Nit: together?
   
   Just to make sure I understand: we can check for `fail withouot fail`, we just can't do it with the same `watchFor`, right?

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       Nit: maybe `logsSupported` ? I would say `areLogsSupported` but that also doesn't sound very good.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/FileLogAction.java
##########
@@ -0,0 +1,117 @@
+package org.apache.cassandra.distributed.impl;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.io.UncheckedIOException;
+import java.util.Objects;
+import java.util.function.Predicate;
+
+import com.google.common.io.Closeables;
+
+import org.apache.cassandra.utils.AbstractIterator;
+import org.apache.cassandra.distributed.api.LogAction;
+import org.apache.cassandra.distributed.api.LineIterator;
+
+public class FileLogAction implements LogAction
+{
+    private final File file;
+
+    public FileLogAction(File file)
+    {
+        this.file = Objects.requireNonNull(file);
+    }
+
+    @Override
+    public long mark()
+    {
+        return file.length();
+    }
+
+    @Override
+    public LineIterator matching(long startPosition, Predicate<String> fn)

Review comment:
       Nit: maybe just `match`? 

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       Nit: predend?

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Nit: together?
   
   Just to make sure I understand: we can check for `fail withouot fail`, we just can't do it with the same `watchFor`, right?

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       Nit: maybe `logsSupported` ? I would say `areLogsSupported` but that also doesn't sound very good.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/FileLogAction.java
##########
@@ -0,0 +1,117 @@
+package org.apache.cassandra.distributed.impl;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.io.UncheckedIOException;
+import java.util.Objects;
+import java.util.function.Predicate;
+
+import com.google.common.io.Closeables;
+
+import org.apache.cassandra.utils.AbstractIterator;
+import org.apache.cassandra.distributed.api.LogAction;
+import org.apache.cassandra.distributed.api.LineIterator;
+
+public class FileLogAction implements LogAction
+{
+    private final File file;
+
+    public FileLogAction(File file)
+    {
+        this.file = Objects.requireNonNull(file);
+    }
+
+    @Override
+    public long mark()
+    {
+        return file.length();
+    }
+
+    @Override
+    public LineIterator matching(long startPosition, Predicate<String> fn)

Review comment:
       Nit: maybe just `match`? 

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       Nit: predend?

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Nit: together?
   
   Just to make sure I understand: we can check for `fail withouot fail`, we just can't do it with the same `watchFor`, right?




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] dcapwell commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

dcapwell commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r487178151



##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       should be `pretend`




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] dcapwell commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

dcapwell commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r487223604



##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`
   
   so this should be `getLogsEnabled()` to match C* pattern




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] dcapwell commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

dcapwell commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r486481990



##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -339,6 +359,12 @@ public void forceCompact(String keyspace, String table)
         });
     }
 
+    @Override
+    public List<Throwable> getUncaughtExceptions()

Review comment:
       this looks like a breaking change in 0.0.5, so had to add it to compile




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] dcapwell removed a comment on pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

dcapwell removed a comment on pull request #748:
URL: https://github.com/apache/cassandra/pull/748#issuecomment-690763008


   @yifan-c added tests for all base methods in the interface (didn't do each permutations, only the core implementation).


----------------------------------------------------------------
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.

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

[GitHub] [cassandra] yifan-c commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

yifan-c commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r487192641



##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       IMO, prefixing with `is`, `should`, `can`, etc. is more expressive and reads better. 
   The `Logs` here is actually log inspection feature. 
   Maybe the full form is `isLogInspectionSupported`, or since it is a feature can or cannot be supported, it could also be `canInspectLogs`.  




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] dcapwell commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

dcapwell commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r487177898



##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       was mostly going off java convention, but we don't always follow it in C*, so maybe I should follow the feature enabled convention? 
   
   I do find your methods better English though =D.

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       should be `pretend`

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Correct, this was trying to match the semantics of python dtest (aka ccm).  grepForErrors stitches logs together but watchFor didn't.
   
   ccm watch for https://github.com/riptano/ccm/blob/master/ccmlib/node.py#L504-L512
   ccm grep for errors https://github.com/riptano/ccm/blob/5c7b782bf072368480fb81fd1ef70a57b09b8c5a/ccmlib/node.py#L2098-L2118
   
   watchFor matches a single line, which might be a single `    at class.method(42)`, or the start of a log line, or the exception message.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`
   
   so this should be `getLogsEnabled()` to match C* pattern

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       was mostly going off java convention, but we don't always follow it in C*, so maybe I should follow the feature enabled convention? 
   
   I do find your methods better English though =D.

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       should be `pretend`

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Correct, this was trying to match the semantics of python dtest (aka ccm).  grepForErrors stitches logs together but watchFor didn't.
   
   ccm watch for https://github.com/riptano/ccm/blob/master/ccmlib/node.py#L504-L512
   ccm grep for errors https://github.com/riptano/ccm/blob/5c7b782bf072368480fb81fd1ef70a57b09b8c5a/ccmlib/node.py#L2098-L2118
   
   watchFor matches a single line, which might be a single `    at class.method(42)`, or the start of a log line, or the exception message.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`
   
   so this should be `getLogsEnabled()` to match C* pattern

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       was mostly going off java convention, but we don't always follow it in C*, so maybe I should follow the feature enabled convention? 
   
   I do find your methods better English though =D.

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       should be `pretend`

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Correct, this was trying to match the semantics of python dtest (aka ccm).  grepForErrors stitches logs together but watchFor didn't.
   
   ccm watch for https://github.com/riptano/ccm/blob/master/ccmlib/node.py#L504-L512
   ccm grep for errors https://github.com/riptano/ccm/blob/5c7b782bf072368480fb81fd1ef70a57b09b8c5a/ccmlib/node.py#L2098-L2118
   
   watchFor matches a single line, which might be a single `    at class.method(42)`, or the start of a log line, or the exception message.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`
   
   so this should be `getLogsEnabled()` to match C* pattern

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       was mostly going off java convention, but we don't always follow it in C*, so maybe I should follow the feature enabled convention? 
   
   I do find your methods better English though =D.

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       should be `pretend`

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Correct, this was trying to match the semantics of python dtest (aka ccm).  grepForErrors stitches logs together but watchFor didn't.
   
   ccm watch for https://github.com/riptano/ccm/blob/master/ccmlib/node.py#L504-L512
   ccm grep for errors https://github.com/riptano/ccm/blob/5c7b782bf072368480fb81fd1ef70a57b09b8c5a/ccmlib/node.py#L2098-L2118
   
   watchFor matches a single line, which might be a single `    at class.method(42)`, or the start of a log line, or the exception message.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`
   
   so this should be `getLogsEnabled()` to match C* pattern

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       was mostly going off java convention, but we don't always follow it in C*, so maybe I should follow the feature enabled convention? 
   
   I do find your methods better English though =D.

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       should be `pretend`

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Correct, this was trying to match the semantics of python dtest (aka ccm).  grepForErrors stitches logs together but watchFor didn't.
   
   ccm watch for https://github.com/riptano/ccm/blob/master/ccmlib/node.py#L504-L512
   ccm grep for errors https://github.com/riptano/ccm/blob/5c7b782bf072368480fb81fd1ef70a57b09b8c5a/ccmlib/node.py#L2098-L2118
   
   watchFor matches a single line, which might be a single `    at class.method(42)`, or the start of a log line, or the exception message.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`
   
   so this should be `getLogsEnabled()` to match C* pattern

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       was mostly going off java convention, but we don't always follow it in C*, so maybe I should follow the feature enabled convention? 
   
   I do find your methods better English though =D.

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       should be `pretend`

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Correct, this was trying to match the semantics of python dtest (aka ccm).  grepForErrors stitches logs together but watchFor didn't.
   
   ccm watch for https://github.com/riptano/ccm/blob/master/ccmlib/node.py#L504-L512
   ccm grep for errors https://github.com/riptano/ccm/blob/5c7b782bf072368480fb81fd1ef70a57b09b8c5a/ccmlib/node.py#L2098-L2118
   
   watchFor matches a single line, which might be a single `    at class.method(42)`, or the start of a log line, or the exception message.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`
   
   so this should be `getLogsEnabled()` to match C* pattern

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       was mostly going off java convention, but we don't always follow it in C*, so maybe I should follow the feature enabled convention? 
   
   I do find your methods better English though =D.

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       should be `pretend`

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Correct, this was trying to match the semantics of python dtest (aka ccm).  grepForErrors stitches logs together but watchFor didn't.
   
   ccm watch for https://github.com/riptano/ccm/blob/master/ccmlib/node.py#L504-L512
   ccm grep for errors https://github.com/riptano/ccm/blob/5c7b782bf072368480fb81fd1ef70a57b09b8c5a/ccmlib/node.py#L2098-L2118
   
   watchFor matches a single line, which might be a single `    at class.method(42)`, or the start of a log line, or the exception message.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`
   
   so this should be `getLogsEnabled()` to match C* pattern




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] dcapwell commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

dcapwell commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r487177898



##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       was mostly going off java convention, but we don't always follow it in C*, so maybe I should follow the feature enabled convention? 
   
   I do find your methods better English though =D.




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] yifan-c commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

yifan-c commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r486541315



##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -339,6 +359,12 @@ public void forceCompact(String keyspace, String table)
         });
     }
 
+    @Override
+    public List<Throwable> getUncaughtExceptions()

Review comment:
       FYI, there is an unmerged commit https://github.com/krummas/cassandra/commit/7cec7bd563fac3089e45fec89ac3f493683f0a85 that implements the new interface. (whoever merges later needs to do a rebase)

##########
File path: test/conf/logback-dtest.xml
##########
@@ -70,7 +53,7 @@
   <logger name="org.apache.hadoop" level="WARN"/>
 
   <root level="DEBUG">
-    <appender-ref ref="INSTANCEASYNCFILE" />
+    <appender-ref ref="INSTANCEFILE" /> <!-- use blocking to avoid race conditions with appending and searching -->

Review comment:
       👍  
   Make sense. The log volume should be small from a jvm dtest. Async is not necessary. 




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] dcapwell commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

dcapwell commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r486487948



##########
File path: test/conf/logback-dtest.xml
##########
@@ -18,35 +18,18 @@
 -->
 
 <configuration debug="false" scan="true" scanPeriod="60 seconds">
+  <define name="cluster_id" class="org.apache.cassandra.distributed.impl.ClusterIDDefiner" />
   <define name="instance_id" class="org.apache.cassandra.distributed.impl.InstanceIDDefiner" />
 
   <!-- Shutdown hook ensures that async appender flushes -->
   <shutdownHook class="ch.qos.logback.core.hook.DelayingShutdownHook"/>
 
-  <appender name="INSTANCEFILE" class="ch.qos.logback.core.rolling.RollingFileAppender">
-
-    <file>./build/test/logs/${cassandra.testtag}/TEST-${suitename}.log</file>
-    <rollingPolicy class="ch.qos.logback.core.rolling.FixedWindowRollingPolicy">
-      <fileNamePattern>./build/test/logs/${cassandra.testtag}/TEST-${suitename}.log.%i.gz</fileNamePattern>
-      <minIndex>1</minIndex>
-      <maxIndex>20</maxIndex>
-    </rollingPolicy>
-
-    <triggeringPolicy class="ch.qos.logback.core.rolling.SizeBasedTriggeringPolicy">
-      <maxFileSize>20MB</maxFileSize>
-    </triggeringPolicy>
-
+  <appender name="INSTANCEFILE" class="ch.qos.logback.core.FileAppender">

Review comment:
       if we roll it makes reading the files more complex.  Also, in CI, the suite name should exist, so we would be rolling for each




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] dcapwell commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

dcapwell commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r487177898



##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       was mostly going off java convention, but we don't always follow it in C*, so maybe I should follow the feature enabled convention? 
   
   I do find your methods better English though =D.

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       should be `pretend`

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Correct, this was trying to match the semantics of python dtest (aka ccm).  grepForErrors stitches logs together but watchFor didn't.
   
   ccm watch for https://github.com/riptano/ccm/blob/master/ccmlib/node.py#L504-L512
   ccm grep for errors https://github.com/riptano/ccm/blob/5c7b782bf072368480fb81fd1ef70a57b09b8c5a/ccmlib/node.py#L2098-L2118
   
   watchFor matches a single line, which might be a single `    at class.method(42)`, or the start of a log line, or the exception message.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`
   
   so this should be `getLogsEnabled()` to match C* pattern

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       was mostly going off java convention, but we don't always follow it in C*, so maybe I should follow the feature enabled convention? 
   
   I do find your methods better English though =D.

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       should be `pretend`

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Correct, this was trying to match the semantics of python dtest (aka ccm).  grepForErrors stitches logs together but watchFor didn't.
   
   ccm watch for https://github.com/riptano/ccm/blob/master/ccmlib/node.py#L504-L512
   ccm grep for errors https://github.com/riptano/ccm/blob/5c7b782bf072368480fb81fd1ef70a57b09b8c5a/ccmlib/node.py#L2098-L2118
   
   watchFor matches a single line, which might be a single `    at class.method(42)`, or the start of a log line, or the exception message.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`
   
   so this should be `getLogsEnabled()` to match C* pattern




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] ifesdjeen commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

ifesdjeen commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r487093544



##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       Nit: maybe `logsSupported` ? I would say `areLogsSupported` but that also doesn't sound very good.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/FileLogAction.java
##########
@@ -0,0 +1,117 @@
+package org.apache.cassandra.distributed.impl;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.io.UncheckedIOException;
+import java.util.Objects;
+import java.util.function.Predicate;
+
+import com.google.common.io.Closeables;
+
+import org.apache.cassandra.utils.AbstractIterator;
+import org.apache.cassandra.distributed.api.LogAction;
+import org.apache.cassandra.distributed.api.LineIterator;
+
+public class FileLogAction implements LogAction
+{
+    private final File file;
+
+    public FileLogAction(File file)
+    {
+        this.file = Objects.requireNonNull(file);
+    }
+
+    @Override
+    public long mark()
+    {
+        return file.length();
+    }
+
+    @Override
+    public LineIterator matching(long startPosition, Predicate<String> fn)

Review comment:
       Nit: maybe just `match`? 

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       Nit: predend?

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Nit: together?
   
   Just to make sure I understand: we can check for `fail withouot fail`, we just can't do it with the same `watchFor`, right?




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] dcapwell commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

dcapwell commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r487181360



##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Correct, this was trying to match the semantics of python dtest (aka ccm).  grepForErrors stitches logs together but watchFor didn't.
   
   ccm watch for https://github.com/riptano/ccm/blob/master/ccmlib/node.py#L504-L512
   ccm grep for errors https://github.com/riptano/ccm/blob/5c7b782bf072368480fb81fd1ef70a57b09b8c5a/ccmlib/node.py#L2098-L2118
   
   watchFor matches a single line, which might be a single `    at class.method(42)`, or the start of a log line, or the exception message.




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] dcapwell commented on pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

dcapwell commented on pull request #748:
URL: https://github.com/apache/cassandra/pull/748#issuecomment-690763008


   @yifan-c added tests for all base methods in the interface (didn't do each permutations, only the core implementation).


----------------------------------------------------------------
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.

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

[GitHub] [cassandra] dcapwell commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

dcapwell commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r487223604



##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       According to https://issues.apache.org/jira/browse/CASSANDRA-15234 the pattern is
   
   yaml:
   ```
   <feature>_enabled
   ```
   
   with getter, so `get<feature>Enabled()`




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] yifan-c commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

yifan-c commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r487192641



##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       IMO, prefixing with `is`, `should`, `can`, etc. is more expressive and reads better. 
   The `Logs` here is actually log inspection feature. 
   Maybe the full form is `isLogInspectionSupported`, or since it is a feature can or cannot be supported, it could also be `canInspectLogs`.  




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] ifesdjeen commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

ifesdjeen commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r487093544



##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       Nit: maybe `logsSupported` ? I would say `areLogsSupported` but that also doesn't sound very good.

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/FileLogAction.java
##########
@@ -0,0 +1,117 @@
+package org.apache.cassandra.distributed.impl;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.io.UncheckedIOException;
+import java.util.Objects;
+import java.util.function.Predicate;
+
+import com.google.common.io.Closeables;
+
+import org.apache.cassandra.utils.AbstractIterator;
+import org.apache.cassandra.distributed.api.LogAction;
+import org.apache.cassandra.distributed.api.LineIterator;
+
+public class FileLogAction implements LogAction
+{
+    private final File file;
+
+    public FileLogAction(File file)
+    {
+        this.file = Objects.requireNonNull(file);
+    }
+
+    @Override
+    public long mark()
+    {
+        return file.length();
+    }
+
+    @Override
+    public LineIterator matching(long startPosition, Predicate<String> fn)

Review comment:
       Nit: maybe just `match`? 

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown

Review comment:
       Nit: predend?

##########
File path: test/distributed/org/apache/cassandra/distributed/test/JVMDTestTest.java
##########
@@ -50,4 +56,30 @@ public void insertTimestampTest() throws IOException
             assertEquals(1000, (long) res[0][0]);
         }
     }
+
+    @Test
+    public void instanceLogs() throws IOException, TimeoutException
+    {
+        try (Cluster cluster = init(Cluster.build(2).withConfig(c -> c.with(Feature.values())).start()))
+        {
+            // debug logging is turned on so we will see debug logs
+            Assertions.assertThat(cluster.get(1).logs().grep("^DEBUG")).isNotEmpty();
+            // make sure an exception is thrown in the cluster
+            LogAction logs = cluster.get(2).logs();
+            long mark = logs.mark(); // get the current position so watching doesn't see any previous exceptions
+            cluster.get(2).runOnInstance(() -> {
+                // pretent that an uncaught exception was thrown
+                CassandraDaemon.uncaughtException(Thread.currentThread(), new RuntimeException("fail without fail"));
+            });
+            List<String> errors = logs.watchFor(mark, "^ERROR");
+            Assertions.assertThat(errors)
+                      // can't check for "fail without fail" since thats on the next line, and watchFor doesn't
+                      // stitch lines togher like grepForError does

Review comment:
       Nit: together?
   
   Just to make sure I understand: we can check for `fail withouot fail`, we just can't do it with the same `watchFor`, right?




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] yifan-c commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

yifan-c commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r487192641



##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       IMO, prefixing with `is`, `should`, `can`, etc. is more expressive and reads better. 
   The `Logs` here is actually log inspection feature. 
   Maybe the full form is `isLogInspectionSupported`, or since it is a feature can or cannot be supported, it could also be `canInspectLogs`.  

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       IMO, prefixing with `is`, `should`, `can`, etc. is more expressive and reads better. 
   The `Logs` here is actually log inspection feature. 
   Maybe the full form is `isLogInspectionSupported`, or since it is a feature can or cannot be supported, it could also be `canInspectLogs`.  

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       IMO, prefixing with `is`, `should`, `can`, etc. is more expressive and reads better. 
   The `Logs` here is actually log inspection feature. 
   Maybe the full form is `isLogInspectionSupported`, or since it is a feature can or cannot be supported, it could also be `canInspectLogs`.  

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       IMO, prefixing with `is`, `should`, `can`, etc. is more expressive and reads better. 
   The `Logs` here is actually log inspection feature. 
   Maybe the full form is `isLogInspectionSupported`, or since it is a feature can or cannot be supported, it could also be `canInspectLogs`.  

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       IMO, prefixing with `is`, `should`, `can`, etc. is more expressive and reads better. 
   The `Logs` here is actually log inspection feature. 
   Maybe the full form is `isLogInspectionSupported`, or since it is a feature can or cannot be supported, it could also be `canInspectLogs`.  

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       IMO, prefixing with `is`, `should`, `can`, etc. is more expressive and reads better. 
   The `Logs` here is actually log inspection feature. 
   Maybe the full form is `isLogInspectionSupported`, or since it is a feature can or cannot be supported, it could also be `canInspectLogs`.  




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] dcapwell commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

dcapwell commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r486647402



##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -339,6 +359,12 @@ public void forceCompact(String keyspace, String table)
         });
     }
 
+    @Override
+    public List<Throwable> getUncaughtExceptions()

Review comment:
       yep.  Wanted to call out that this isn't added by this PR so "don't look here" =D




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] yifan-c commented on a change in pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

yifan-c commented on a change in pull request #748:
URL: https://github.com/apache/cassandra/pull/748#discussion_r487192641



##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       IMO, prefixing with `is`, `should`, `can`, etc. is more expressive and reads better. 
   The `Logs` here is actually log inspection feature. 
   Maybe the full form is `isLogInspectionSupported`, or since it is a feature can or cannot be supported, it could also be `canInspectLogs`.  

##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -151,6 +154,24 @@
         StreamingInboundHandler.trackInboundHandlers();
     }
 
+    @Override
+    public boolean isLogsSupported()

Review comment:
       IMO, prefixing with `is`, `should`, `can`, etc. is more expressive and reads better. 
   The `Logs` here is actually log inspection feature. 
   Maybe the full form is `isLogInspectionSupported`, or since it is a feature can or cannot be supported, it could also be `canInspectLogs`.  




----------------------------------------------------------------
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.

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

[GitHub] [cassandra] smiklosovic closed pull request #748: CASSANDRA-16120 - Add ability for jvm-dtest to grep instance logs

[GitBox <gi...@apache.org>]

smiklosovic closed pull request #748:
URL: https://github.com/apache/cassandra/pull/748


   


-- 
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