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:
[email protected]
