You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/07/31 06:50:31 UTC
[GitHub] [cassandra] bereng opened a new pull request #706: CASSANDRA-16003 Improve tooling stdErr checks
bereng opened a new pull request #706:
URL: https://github.com/apache/cassandra/pull/706
----------------------------------------------------------------
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] bereng commented on pull request #706: CASSANDRA-16003 Improve tooling stdErr checks
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #706:
URL: https://github.com/apache/cassandra/pull/706#issuecomment-666974129
[J11 CI](https://app.circleci.com/pipelines/github/bereng/cassandra/83/workflows/386c5b95-371d-4777-8411-fceb9a427c4f)
[j8 CI](https://app.circleci.com/pipelines/github/bereng/cassandra/83/workflows/ca515754-6afb-4f40-ba69-46a1860afd56)
----------------------------------------------------------------
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] bereng closed pull request #706: CASSANDRA-16003 Improve tooling stdErr checks
Posted by GitBox <gi...@apache.org>.
bereng closed pull request #706:
URL: https://github.com/apache/cassandra/pull/706
----------------------------------------------------------------
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 #706: CASSANDRA-16003 Improve tooling stdErr checks
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #706:
URL: https://github.com/apache/cassandra/pull/706#discussion_r464526319
##########
File path: test/unit/org/apache/cassandra/tools/ToolRunner.java
##########
@@ -49,6 +49,8 @@
public class ToolRunner implements AutoCloseable
{
protected static final Logger logger = LoggerFactory.getLogger(ToolRunner.class);
+
+ public List<String> stdErrRegExpCleaners = Arrays.asList("(?im)^picked up.*\\R");
Review comment:
should be final and immutable if public, if private should just be final.
----------------------------------------------------------------
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] bereng commented on a change in pull request #706: CASSANDRA-16003 Improve tooling stdErr checks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #706:
URL: https://github.com/apache/cassandra/pull/706#discussion_r464307542
##########
File path: test/unit/org/apache/cassandra/tools/ToolRunner.java
##########
@@ -330,12 +330,21 @@ public ToolRunner waitAndAssertOnExitCode()
public ToolRunner waitAndAssertOnCleanExit()
{
- return waitAndAssertOnExitCode().assertEmptyStdErr();
+ return waitAndAssertOnExitCode().assertCleanStdErr();
}
- public ToolRunner assertEmptyStdErr()
+ /**
+ * Checks if the stdErr is empty after removing any JVM env info output
+ *
+ * Some JVM configs may output env info on stdErr. We need to remove those to see what was the tool's actual stdErr
+ *
+ * @return
+ */
+ public ToolRunner assertCleanStdErr()
Review comment:
Aha! icywm. Iiuc my latest commit works off an exclude list. I haven't gone as far as implementing the `grepLog` functionality as it sounds more like sthg for another ticket + I want to merger this asap bc of 15583 (the main tooling ticket).
Still I think this addition is nice and a step in the direction you point. Wdyt?
----------------------------------------------------------------
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 #706: CASSANDRA-16003 Improve tooling stdErr checks
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #706:
URL: https://github.com/apache/cassandra/pull/706#discussion_r465213524
##########
File path: test/unit/org/apache/cassandra/tools/ToolRunner.java
##########
@@ -49,6 +49,8 @@
public class ToolRunner implements AutoCloseable
{
protected static final Logger logger = LoggerFactory.getLogger(ToolRunner.class);
+
+ public List<String> stdErrRegExpCleaners = Arrays.asList("(?im)^picked up.*\\R");
Review comment:
I see where you are coming from, but mutable state can still be brittle for tests, so simpler to avoid it in the API.
Since this is public and mutable, anyone is able to modify it
1) public means any other class can have access, and do with it what Is allowed by the object
2) the object is mutable, given the fact that it is public, any class could mutate unexpectedly
3) you can't add to this list, so "modifiable to your linking" wouldn't work
```
jshell> var v =Arrays.asList(1, 2)
v ==> [1, 2]
jshell> v.add(3)
| Exception java.lang.UnsupportedOperationException
| at AbstractList.add (AbstractList.java:153)
| at AbstractList.add (AbstractList.java:111)
| at (#3:1)
jshell> v.remove(1)
| Exception java.lang.UnsupportedOperationException
| at AbstractList.remove (AbstractList.java:167)
| at (#4:1)
jshell> v.set(0, 42)
$5 ==> 1
jshell> v
v ==> [42, 2]
```
Given this, it makes sense to be the `defaultCleanersList`, also makes sense to expose so the caller can add to their own list; but if it is exposed, it should be immutable
----------------------------------------------------------------
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 #706: CASSANDRA-16003 Improve tooling stdErr checks
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #706:
URL: https://github.com/apache/cassandra/pull/706#discussion_r463815976
##########
File path: test/unit/org/apache/cassandra/tools/ToolRunner.java
##########
@@ -330,12 +330,21 @@ public ToolRunner waitAndAssertOnExitCode()
public ToolRunner waitAndAssertOnCleanExit()
{
- return waitAndAssertOnExitCode().assertEmptyStdErr();
+ return waitAndAssertOnExitCode().assertCleanStdErr();
}
- public ToolRunner assertEmptyStdErr()
+ /**
+ * Checks if the stdErr is empty after removing any JVM env info output
+ *
+ * Some JVM configs may output env info on stdErr. We need to remove those to see what was the tool's actual stdErr
+ *
+ * @return
+ */
+ public ToolRunner assertCleanStdErr()
Review comment:
Thanks for this!
To help make this more extendable, it would be good to use a exclude list and filter based off that; this is what happens in python dtests (example: https://github.com/riptano/ccm/blob/master/ccmlib/node.py#L395-L421) today.
With an exclude list, we can add more cases by default over time, but can also let tests control and search for the logs they care about.
----------------------------------------------------------------
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] bereng commented on a change in pull request #706: CASSANDRA-16003 Improve tooling stdErr checks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #706:
URL: https://github.com/apache/cassandra/pull/706#discussion_r465535270
##########
File path: test/unit/org/apache/cassandra/tools/ToolRunner.java
##########
@@ -49,6 +49,8 @@
public class ToolRunner implements AutoCloseable
{
protected static final Logger logger = LoggerFactory.getLogger(ToolRunner.class);
+
+ public List<String> stdErrRegExpCleaners = Arrays.asList("(?im)^picked up.*\\R");
Review comment:
Ok, immutable and public it is :-)
----------------------------------------------------------------
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] bereng commented on a change in pull request #706: CASSANDRA-16003 Improve tooling stdErr checks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #706:
URL: https://github.com/apache/cassandra/pull/706#discussion_r464863322
##########
File path: test/unit/org/apache/cassandra/tools/ToolRunner.java
##########
@@ -49,6 +49,8 @@
public class ToolRunner implements AutoCloseable
{
protected static final Logger logger = LoggerFactory.getLogger(ToolRunner.class);
+
+ public List<String> stdErrRegExpCleaners = Arrays.asList("(?im)^picked up.*\\R");
Review comment:
Icwym you're probably thinking along the lines of `defaultCleanersList` whereas I was thinking along the lines of `cleanersInThisInstance` and that to be modifiable to your liking. That's why I made it public, not static and not final :shrug:. Makes sense or you'd prefer me to change it?
----------------------------------------------------------------
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] bereng commented on a change in pull request #706: CASSANDRA-16003 Improve tooling stdErr checks
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #706:
URL: https://github.com/apache/cassandra/pull/706#discussion_r464307542
##########
File path: test/unit/org/apache/cassandra/tools/ToolRunner.java
##########
@@ -330,12 +330,21 @@ public ToolRunner waitAndAssertOnExitCode()
public ToolRunner waitAndAssertOnCleanExit()
{
- return waitAndAssertOnExitCode().assertEmptyStdErr();
+ return waitAndAssertOnExitCode().assertCleanStdErr();
}
- public ToolRunner assertEmptyStdErr()
+ /**
+ * Checks if the stdErr is empty after removing any JVM env info output
+ *
+ * Some JVM configs may output env info on stdErr. We need to remove those to see what was the tool's actual stdErr
+ *
+ * @return
+ */
+ public ToolRunner assertCleanStdErr()
Review comment:
Aha! icywm. Iiuc my latest commit works off an exclude list. I haven't gone as far as implementing the `grepLog` functionality as it sounds more like sthg for another ticket + I want to merge this asap bc of 15583 (the main tooling ticket).
Still I think this addition is nice and a step in the direction you point. Wdyt?
----------------------------------------------------------------
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