You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/05/02 20:14:53 UTC
[GitHub] [accumulo] EdColeman opened a new pull request #2073: Fix #2070 address ShellServerIT test failures
EdColeman opened a new pull request #2073:
URL: https://github.com/apache/accumulo/pull/2073
- Move table clean-up from after action to afterClass so tables not deleted during tests.
- Add updated FooConstraint jar (FooConstraint_2_1.jar) updated to use 2.1 Constraint package.
- Address checkstyle issues
TODO:
- Relacing the jar allows test to pass, but does not address legacy code.
--
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
[GitHub] [accumulo] EdColeman commented on a change in pull request #2073: Fix #2070 address ShellServerIT test failures
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #2073:
URL: https://github.com/apache/accumulo/pull/2073#discussion_r624992065
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1717,7 +1719,7 @@ public void testPertableClasspath() throws Exception {
fooFilterJar.deleteOnExit();
File fooConstraintJar = File.createTempFile("FooConstraint", ".jar", new File(rootPath));
- FileUtils.copyInputStreamToFile(this.getClass().getResourceAsStream("/FooConstraint.jar"),
+ FileUtils.copyInputStreamToFile(this.getClass().getResourceAsStream("/FooConstraint_2_1.jar"),
Review comment:
Yes - this uses the new interface. When I tested with something built with the older interface it failed. I think one way to handle this is to add a tests that tests a jar built with the each version - step one was just to see if this would get the test to pass (which it does now) in case that would help point a way forward - this was the motivation to be able to build jars for multiple versions.
--
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
[GitHub] [accumulo] EdColeman commented on pull request #2073: Fix #2070 address ShellServerIT test failures
Posted by GitBox <gi...@apache.org>.
EdColeman commented on pull request #2073:
URL: https://github.com/apache/accumulo/pull/2073#issuecomment-831273898
Just to be clear - I was not proposing that this iteration of the test(s) is the final solution. I wanted to show that there is a problem with the legacy jar that is causing the test to fail and we need to address the code so that we can accept both legacy jars and jars created with the updated Constraint paths. Ultimately I think we need tests for both types of jars.
--
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
[GitHub] [accumulo] EdColeman commented on pull request #2073: Fix #2070 address ShellServerIT test failures
Posted by GitBox <gi...@apache.org>.
EdColeman commented on pull request #2073:
URL: https://github.com/apache/accumulo/pull/2073#issuecomment-836073388
Replaced with https://github.com/apache/accumulo/pull/2087
--
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
[GitHub] [accumulo] EdColeman commented on pull request #2073: Fix #2070 address ShellServerIT test failures
Posted by GitBox <gi...@apache.org>.
EdColeman commented on pull request #2073:
URL: https://github.com/apache/accumulo/pull/2073#issuecomment-830873816
I was uncertain where the FooConstraint.jar used in the ShellServerIT test comes from - to change the classes imported by the class in the jar, I create a small project that can create new jars for each Accumulo version. The 1_10 version and 2_1 sub-projects were used for this PR - 2_0 is just an untested stub.
This is a [staging repo](https://github.com/EdColeman/AccumuloConstraintTestJars) until I know where / how / if this should become part of Accumulo project proper. The projects are just a rough draft to explore the concept until we decide what to do w.r.t this as an approach. Can't really say rough draft too many times - but really just hoping for direction on approach before continuing.
--
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
[GitHub] [accumulo] EdColeman closed pull request #2073: Fix #2070 address ShellServerIT test failures
Posted by GitBox <gi...@apache.org>.
EdColeman closed pull request #2073:
URL: https://github.com/apache/accumulo/pull/2073
--
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
[GitHub] [accumulo] EdColeman commented on pull request #2073: Fix #2070 address ShellServerIT test failures
Posted by GitBox <gi...@apache.org>.
EdColeman commented on pull request #2073:
URL: https://github.com/apache/accumulo/pull/2073#issuecomment-832285580
I did not intentionally move code around - not sure if auto format did something on my behalf though.
--
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
[GitHub] [accumulo] EdColeman commented on a change in pull request #2073: Fix #2070 address ShellServerIT test failures
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #2073:
URL: https://github.com/apache/accumulo/pull/2073#discussion_r624994059
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -2687,13 +2705,15 @@ public void testCreateTableWithBinarySplitsFile1()
try (AccumuloClient client = Accumulo.newClient().from(getClientProps()).build()) {
splitsFile = System.getProperty("user.dir") + "/target/splitFile";
generateSplitsFile(splitsFile, 200, 12, true, true, true, false, false);
- SortedSet<Text> expectedSplits = readSplitsFromFile(splitsFile, false);
+ SortedSet<Text> expectedSplits = readSplitsFromFile(splitsFile);
final String tableName = name.getMethodName() + "_table";
ts.exec("createtable " + tableName + " -sf " + splitsFile, true);
Collection<Text> createdSplits = client.tableOperations().listSplits(tableName);
assertEquals(expectedSplits, new TreeSet<>(createdSplits));
} finally {
- Files.delete(Paths.get(splitsFile));
+ if (Objects.nonNull(splitsFile)) {
Review comment:
I'll change these - the "reason" is the code analysis flags these as possibility returning null and didn't want to add suppression(s)
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2073: Fix #2070 address ShellServerIT test failures
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2073:
URL: https://github.com/apache/accumulo/pull/2073#discussion_r628611072
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -345,21 +345,33 @@ public static void tearDownAfterClass() {
traceProcess.destroy();
}
+ logUndeletedTables();
SharedMiniClusterBase.stopMiniCluster();
}
- @After
- public void deleteTables() throws Exception {
+ /**
+ * Tests should be cleaning up tables as they complete - this checks at the end of this test and
+ * prints diagnostic messages if a table remains.
+ */
+ private static void logUndeletedTables() {
try (AccumuloClient c = Accumulo.newClient().from(getClientProps()).build()) {
for (String table : c.tableOperations().list()) {
- if (!table.startsWith(Namespace.ACCUMULO.name() + ".") && !table.equals("trace"))
- try {
- c.tableOperations().delete(table);
- } catch (TableNotFoundException e) {
- // don't care
- }
+ if (!table.startsWith(Namespace.ACCUMULO.name() + ".") && !table.equals("trace")) {
+ log.warn("Clean up of Table left after test: {}", table);
+ }
}
Review comment:
If the `getUniqueNames()` method is used for the table names and namespaces for each test, then this `@After` method could be modified to clean up only the most recently run method's tables, rather than clean up each test separately in each test method body. Doing it that way would keep the cleanup code minimal, while not deleting tables that might be in use by another test.
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2073: Fix #2070 address ShellServerIT test failures
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2073:
URL: https://github.com/apache/accumulo/pull/2073#discussion_r624870598
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -2900,9 +2931,7 @@ private void generateSplitsFile(final String splitsFile, final int numItems, fin
}
private Text getRandomText(final int len) {
- int desiredLen = len;
- if (len > 32)
- desiredLen = 32;
+ var desiredLen = Math.min(len, 32);
Review comment:
This isn't a great use of `var`. `int` would be better here, since the type isn't obvious from the variable name or its assignment.
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1717,7 +1719,7 @@ public void testPertableClasspath() throws Exception {
fooFilterJar.deleteOnExit();
File fooConstraintJar = File.createTempFile("FooConstraint", ".jar", new File(rootPath));
- FileUtils.copyInputStreamToFile(this.getClass().getResourceAsStream("/FooConstraint.jar"),
+ FileUtils.copyInputStreamToFile(this.getClass().getResourceAsStream("/FooConstraint_2_1.jar"),
Review comment:
I assume this new jar has constraints using the new public API package? I'm not sure it matters for this test. We should support older constraints as well, so this test should pass whether or not the jar contains classes that implement the new or old constraint interface.
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -2687,13 +2705,15 @@ public void testCreateTableWithBinarySplitsFile1()
try (AccumuloClient client = Accumulo.newClient().from(getClientProps()).build()) {
splitsFile = System.getProperty("user.dir") + "/target/splitFile";
generateSplitsFile(splitsFile, 200, 12, true, true, true, false, false);
- SortedSet<Text> expectedSplits = readSplitsFromFile(splitsFile, false);
+ SortedSet<Text> expectedSplits = readSplitsFromFile(splitsFile);
final String tableName = name.getMethodName() + "_table";
ts.exec("createtable " + tableName + " -sf " + splitsFile, true);
Collection<Text> createdSplits = client.tableOperations().listSplits(tableName);
assertEquals(expectedSplits, new TreeSet<>(createdSplits));
} finally {
- Files.delete(Paths.get(splitsFile));
+ if (Objects.nonNull(splitsFile)) {
Review comment:
`Objects.nonNull` exists for use as a lambda when a Predicate is required (`stream.filter(Objects::nonNull)`, for example). For plain old if statements, it's much more clear to just write:
```suggestion
if (splitsFile != null) {
```
However, there's no reason for it to ever be null or for us to check for it being null. We can just assign it to `System.getProperty("user.dir") + "/target/splitFile";` in the first place. There's no reason that needs to be done inside the try block.
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -513,7 +515,7 @@ public void du() throws Exception {
String o = ts.output.get();
// for some reason, there's a bit of fluctuation
assertTrue("Output did not match regex: '" + o + "'",
- o.matches(".*[1-9][0-9][0-9]\\s\\[" + table + "\\]\\n"));
+ o.matches(".*[1-9][0-9][0-9]\\s\\[" + table + "]\\n"));
Review comment:
Was this an unnecessary escape?
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -340,16 +341,17 @@ public void setupShell() throws Exception {
}
@AfterClass
- public static void tearDownAfterClass() {
+ public static void tearDownAfterClass() throws Exception {
if (traceProcess != null) {
traceProcess.destroy();
}
+ deleteTables();
+
SharedMiniClusterBase.stopMiniCluster();
}
- @After
- public void deleteTables() throws Exception {
+ public static void deleteTables() throws Exception {
Review comment:
I'm not sure it's worth bothering deleting tables at this point if we're killing mini anyway.
I think this change would only matter if we were using the `parallel` options in `maven-failsafe-plugin`, which we aren't, and shouldn't because that would likely cause all sorts of other problems. The test methods that are run inside the class are always run serially. This delete tables method merely cleans up between different test methods in case they use the same table names or makes the system use too many resources. I don't think your change here accomplishes anything, except leaving tables around longer, possibly causing some methods to collide if they use the same table names.
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2073: Fix #2070 address ShellServerIT test failures
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2073:
URL: https://github.com/apache/accumulo/pull/2073#discussion_r628609441
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -2687,13 +2705,15 @@ public void testCreateTableWithBinarySplitsFile1()
try (AccumuloClient client = Accumulo.newClient().from(getClientProps()).build()) {
splitsFile = System.getProperty("user.dir") + "/target/splitFile";
generateSplitsFile(splitsFile, 200, 12, true, true, true, false, false);
- SortedSet<Text> expectedSplits = readSplitsFromFile(splitsFile, false);
+ SortedSet<Text> expectedSplits = readSplitsFromFile(splitsFile);
final String tableName = name.getMethodName() + "_table";
ts.exec("createtable " + tableName + " -sf " + splitsFile, true);
Collection<Text> createdSplits = client.tableOperations().listSplits(tableName);
assertEquals(expectedSplits, new TreeSet<>(createdSplits));
} finally {
- Files.delete(Paths.get(splitsFile));
+ if (Objects.nonNull(splitsFile)) {
Review comment:
Right, but they are only possibly returning null because null is assigned to the variable first. However, that's an unnecessary assignment. The assignment inside each try block can be moved to just before the try block, so they are immediately assigned to the value from reading the system property, and never assign null in the first place. Then, they can never be null, and you don't need the checks to see if they are null in the finally blocks.
--
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
[GitHub] [accumulo] ctubbsii commented on pull request #2073: Fix #2070 address ShellServerIT test failures
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2073:
URL: https://github.com/apache/accumulo/pull/2073#issuecomment-831016291
I think I'd like to know why legacy constraints are failing in this test. I'm not sure that merely swapping out in favor of the new constraints is helpful.
--
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
[GitHub] [accumulo] EdColeman commented on a change in pull request #2073: Fix #2070 address ShellServerIT test failures
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #2073:
URL: https://github.com/apache/accumulo/pull/2073#discussion_r624990213
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -513,7 +515,7 @@ public void du() throws Exception {
String o = ts.output.get();
// for some reason, there's a bit of fluctuation
assertTrue("Output did not match regex: '" + o + "'",
- o.matches(".*[1-9][0-9][0-9]\\s\\[" + table + "\\]\\n"));
+ o.matches(".*[1-9][0-9][0-9]\\s\\[" + table + "]\\n"));
Review comment:
Yes
--
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