ctubbsii commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r825914247
##########
File path:
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportKey.java
##########
@@ -62,6 +63,7 @@ public ThriftTransportKey(HostAndPort server, long timeout,
ClientContext contex
this.timeout = timeout;
this.sslParams = sslParams;
this.saslParams = saslParams;
+ final int hashCode = hashCode();
Review comment:
This should just set the field directly, so the field itself can be
final, and the `hashCode()` method can just return the field.
```suggestion
this.hash = Objects.hash(server, timeout, sslParams, saslParams);
```
```suggestion
final int hashCode = hashCode();
```
##########
File path:
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportKey.java
##########
@@ -50,6 +50,7 @@ public ThriftTransportKey(HostAndPort server, long timeout,
ClientContext contex
throw new RuntimeException("Cannot use both SSL and SASL thrift
transports");
}
}
+ final int hashCode = hashCode();
Review comment:
To simplify this, this method should just call `this(server, timeout,
context.getClientSslParams(), context.getSaslParams());` and then do the mutual
exclusivity check on saslParams and sslParams. That way, you don't have to set
the hash field here and worry about it drifting from the implementation in the
other constructor.
##########
File path: test/src/main/java/org/apache/accumulo/test/AuditMessageIT.java
##########
@@ -451,33 +452,31 @@ public void testDeniedAudits() throws
AccumuloSecurityException, AccumuloExcepti
// We don't want the thrown exceptions to stop our tests, and we are not
testing that the
// Exceptions are thrown.
- try {
- auditAccumuloClient.tableOperations().create(NEW_TEST_TABLE_NAME);
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().rename(OLD_TEST_TABLE_NAME,
NEW_TEST_TABLE_NAME);
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().clone(OLD_TEST_TABLE_NAME,
NEW_TEST_TABLE_NAME, false,
- Collections.emptyMap(), Collections.emptySet());
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().delete(OLD_TEST_TABLE_NAME);
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().offline(OLD_TEST_TABLE_NAME);
- } catch (AccumuloSecurityException ex) {}
+ assertThrows(AccumuloSecurityException.class,
+ () ->
auditAccumuloClient.tableOperations().create(NEW_TEST_TABLE_NAME));
Review comment:
These would probably be simpler if you pulled out the tableOps:
```suggestion
final var tableOps = auditAccumuloClient.tableOperations();
assertThrows(AccumuloSecurityException.class,
() -> tableOps.create(NEW_TEST_TABLE_NAME));
```
##########
File path:
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportPool.java
##########
@@ -835,8 +833,9 @@ public synchronized void setIdleTime(long time) {
log.debug("Set thrift transport pool idle time to {}", time);
}
+ // TODO consider re-working after #2554 is merged
void startCheckerThread() {
- checkThreadFactory.get();
+ final Thread thread = checkThreadFactory.get();
Review comment:
I agree this could be reworked, but this should be made a follow-on
ticket and the TODO removed from the code, prior to this PR being merged.
##########
File path:
test/src/main/java/org/apache/accumulo/test/functional/ServerSideErrorIT.java
##########
@@ -63,69 +65,50 @@ public void run() throws Exception {
bw.addMutation(m);
}
- boolean caught = false;
// try to scan table
try (Scanner scanner = c.createScanner(tableName, Authorizations.EMPTY))
{
-
- try {
+ assertThrows(Exception.class, () -> {
for (Entry<Key,Value> entry : scanner) {
entry.getKey();
}
- } catch (Exception e) {
- caught = true;
- }
-
- if (!caught)
- throw new Exception("Scan did not fail");
-
- // try to batch scan the table
- try (BatchScanner bs = c.createBatchScanner(tableName,
Authorizations.EMPTY, 2)) {
- bs.setRanges(Collections.singleton(new Range()));
+ });
+ }
- caught = false;
- try {
- for (Entry<Key,Value> entry : bs) {
- entry.getKey();
- }
- } catch (Exception e) {
- caught = true;
+ // try to batch scan the table
+ try (BatchScanner bs = c.createBatchScanner(tableName,
Authorizations.EMPTY, 2)) {
+ bs.setRanges(Collections.singleton(new Range()));
+ assertThrows(Exception.class, () -> {
+ for (Entry<Key,Value> entry : bs) {
+ entry.getKey();
}
- }
-
- if (!caught)
- throw new Exception("batch scan did not fail");
-
- // remove the bad agg so accumulo can shutdown
- TableOperations to = c.tableOperations();
- for (Entry<String,String> e : to.getProperties(tableName)) {
- to.removeProperty(tableName, e.getKey());
- }
+ });
+ }
- sleepUninterruptibly(500, TimeUnit.MILLISECONDS);
+ // remove the bad agg so accumulo can shutdown
+ TableOperations to = c.tableOperations();
+ for (Entry<String,String> e : to.getProperties(tableName)) {
+ to.removeProperty(tableName, e.getKey());
}
+ sleepUninterruptibly(500, TimeUnit.MILLISECONDS);
+
// should be able to scan now
try (Scanner scanner = c.createScanner(tableName, Authorizations.EMPTY))
{
for (Entry<Key,Value> entry : scanner) {
- entry.getKey();
+ assertNotNull(entry.getKey());
}
// set a nonexistent iterator, should cause scan to fail on server side
scanner.addScanIterator(new IteratorSetting(100, "bogus",
"com.bogus.iterator"));
- caught = false;
- try {
+ assertThrows(Exception.class, () -> {
for (Entry<Key,Value> entry : scanner) {
// should error
entry.getKey();
}
- } catch (Exception e) {
- caught = true;
- }
-
- if (!caught)
- throw new Exception("Scan did not fail");
+ });
Review comment:
This could get more specific on the expected exception and the method
that throws.
##########
File path:
test/src/main/java/org/apache/accumulo/test/server/security/SystemCredentialsIT.java
##########
@@ -86,7 +87,7 @@ public static void main(final String[] args) throws
AccumuloException, TableNotF
client.securityOperations().authenticateUser(creds.getPrincipal(),
creds.getToken());
try (Scanner scan = client.createScanner(RootTable.NAME,
Authorizations.EMPTY)) {
for (Entry<Key,Value> e : scan) {
- e.hashCode();
+ assertNotNull(e.hashCode());
Review comment:
assertNotNull would force an auto-boxing of the hashCode int. That
part's probably okay, since we don't actually care about the value of the hash
code. However, I'd probably try to avoid a JUnit assert call inside the main
method. I'm not sure how the test would behave if the main method ended up
throwing an AssertionError instead of the expected System.exit from the below
checks.
##########
File path: test/src/main/java/org/apache/accumulo/test/AuditMessageIT.java
##########
@@ -451,33 +452,31 @@ public void testDeniedAudits() throws
AccumuloSecurityException, AccumuloExcepti
// We don't want the thrown exceptions to stop our tests, and we are not
testing that the
// Exceptions are thrown.
- try {
- auditAccumuloClient.tableOperations().create(NEW_TEST_TABLE_NAME);
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().rename(OLD_TEST_TABLE_NAME,
NEW_TEST_TABLE_NAME);
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().clone(OLD_TEST_TABLE_NAME,
NEW_TEST_TABLE_NAME, false,
- Collections.emptyMap(), Collections.emptySet());
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().delete(OLD_TEST_TABLE_NAME);
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().offline(OLD_TEST_TABLE_NAME);
- } catch (AccumuloSecurityException ex) {}
+ assertThrows(AccumuloSecurityException.class,
+ () ->
auditAccumuloClient.tableOperations().create(NEW_TEST_TABLE_NAME));
+
+ assertThrows(AccumuloSecurityException.class, () ->
auditAccumuloClient.tableOperations()
+ .rename(OLD_TEST_TABLE_NAME, NEW_TEST_TABLE_NAME));
+
+ assertThrows(AccumuloSecurityException.class,
+ () -> auditAccumuloClient.tableOperations().clone(OLD_TEST_TABLE_NAME,
NEW_TEST_TABLE_NAME,
+ false, Collections.emptyMap(), Collections.emptySet()));
+
+ assertThrows(AccumuloSecurityException.class,
+ () ->
auditAccumuloClient.tableOperations().delete(OLD_TEST_TABLE_NAME));
+
+ assertThrows(AccumuloSecurityException.class,
+ () ->
auditAccumuloClient.tableOperations().offline(OLD_TEST_TABLE_NAME));
+
try (Scanner scanner =
auditAccumuloClient.createScanner(OLD_TEST_TABLE_NAME, auths)) {
- scanner.iterator().next().getKey();
- } catch (RuntimeException ex) {}
- try {
- auditAccumuloClient.tableOperations().deleteRows(OLD_TEST_TABLE_NAME,
new Text("myRow"),
- new Text("myRow~"));
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().flush(OLD_TEST_TABLE_NAME, new
Text("myRow"),
- new Text("myRow~"), false);
- } catch (AccumuloSecurityException ex) {}
+ assertThrows(RuntimeException.class, () ->
scanner.iterator().next().getKey());
Review comment:
From this, it's not obvious which method is expected to throw. I think
it throws on the call to `next()`, but am not sure. If so, then the `getKey()`
method call should be dropped and the `scanner.iterator()` part be pulled out
to a variable outside the `assertThrows`.
##########
File path: test/src/main/java/org/apache/accumulo/test/AuditMessageIT.java
##########
@@ -451,33 +452,31 @@ public void testDeniedAudits() throws
AccumuloSecurityException, AccumuloExcepti
// We don't want the thrown exceptions to stop our tests, and we are not
testing that the
// Exceptions are thrown.
- try {
- auditAccumuloClient.tableOperations().create(NEW_TEST_TABLE_NAME);
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().rename(OLD_TEST_TABLE_NAME,
NEW_TEST_TABLE_NAME);
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().clone(OLD_TEST_TABLE_NAME,
NEW_TEST_TABLE_NAME, false,
- Collections.emptyMap(), Collections.emptySet());
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().delete(OLD_TEST_TABLE_NAME);
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().offline(OLD_TEST_TABLE_NAME);
- } catch (AccumuloSecurityException ex) {}
+ assertThrows(AccumuloSecurityException.class,
+ () ->
auditAccumuloClient.tableOperations().create(NEW_TEST_TABLE_NAME));
Review comment:
These would probably be simpler if you pulled out the tableOps:
```suggestion
final var tableOps = auditAccumuloClient.tableOperations();
assertThrows(AccumuloSecurityException.class,
() -> tableOps.create(NEW_TEST_TABLE_NAME));
```
##########
File path:
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportKey.java
##########
@@ -50,6 +50,7 @@ public ThriftTransportKey(HostAndPort server, long timeout,
ClientContext contex
throw new RuntimeException("Cannot use both SSL and SASL thrift
transports");
}
}
+ final int hashCode = hashCode();
Review comment:
To simplify this, this method should just call `this(server, timeout,
context.getClientSslParams(), context.getSaslParams());` and then do the mutual
exclusivity check on saslParams and sslParams. That way, you don't have to set
the hash field here and worry about it drifting from the implementation in the
other constructor.
##########
File path:
test/src/main/java/org/apache/accumulo/test/functional/ServerSideErrorIT.java
##########
@@ -63,69 +65,50 @@ public void run() throws Exception {
bw.addMutation(m);
}
- boolean caught = false;
// try to scan table
try (Scanner scanner = c.createScanner(tableName, Authorizations.EMPTY))
{
-
- try {
+ assertThrows(Exception.class, () -> {
for (Entry<Key,Value> entry : scanner) {
entry.getKey();
}
- } catch (Exception e) {
- caught = true;
- }
-
- if (!caught)
- throw new Exception("Scan did not fail");
-
- // try to batch scan the table
- try (BatchScanner bs = c.createBatchScanner(tableName,
Authorizations.EMPTY, 2)) {
- bs.setRanges(Collections.singleton(new Range()));
+ });
Review comment:
Does this throw on `scanner.iterator()` or `iterator.next()`? I think
this could get more specific on the assertThrows, both in the exception that is
thrown and the method that is expected to throw, rather than wrap the whole
loop.
##########
File path:
test/src/main/java/org/apache/accumulo/test/functional/ServerSideErrorIT.java
##########
@@ -63,69 +65,50 @@ public void run() throws Exception {
bw.addMutation(m);
}
- boolean caught = false;
// try to scan table
try (Scanner scanner = c.createScanner(tableName, Authorizations.EMPTY))
{
-
- try {
+ assertThrows(Exception.class, () -> {
for (Entry<Key,Value> entry : scanner) {
entry.getKey();
}
- } catch (Exception e) {
- caught = true;
- }
-
- if (!caught)
- throw new Exception("Scan did not fail");
-
- // try to batch scan the table
- try (BatchScanner bs = c.createBatchScanner(tableName,
Authorizations.EMPTY, 2)) {
- bs.setRanges(Collections.singleton(new Range()));
+ });
Review comment:
Does this throw on `scanner.iterator()` or `iterator.next()`? I think
this could get more specific on the assertThrows, both in the exception that is
thrown and the method that is expected to throw, rather than wrap the whole
loop.
##########
File path: test/src/main/java/org/apache/accumulo/test/AuditMessageIT.java
##########
@@ -451,33 +452,31 @@ public void testDeniedAudits() throws
AccumuloSecurityException, AccumuloExcepti
// We don't want the thrown exceptions to stop our tests, and we are not
testing that the
// Exceptions are thrown.
- try {
- auditAccumuloClient.tableOperations().create(NEW_TEST_TABLE_NAME);
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().rename(OLD_TEST_TABLE_NAME,
NEW_TEST_TABLE_NAME);
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().clone(OLD_TEST_TABLE_NAME,
NEW_TEST_TABLE_NAME, false,
- Collections.emptyMap(), Collections.emptySet());
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().delete(OLD_TEST_TABLE_NAME);
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().offline(OLD_TEST_TABLE_NAME);
- } catch (AccumuloSecurityException ex) {}
+ assertThrows(AccumuloSecurityException.class,
+ () ->
auditAccumuloClient.tableOperations().create(NEW_TEST_TABLE_NAME));
+
+ assertThrows(AccumuloSecurityException.class, () ->
auditAccumuloClient.tableOperations()
+ .rename(OLD_TEST_TABLE_NAME, NEW_TEST_TABLE_NAME));
+
+ assertThrows(AccumuloSecurityException.class,
+ () -> auditAccumuloClient.tableOperations().clone(OLD_TEST_TABLE_NAME,
NEW_TEST_TABLE_NAME,
+ false, Collections.emptyMap(), Collections.emptySet()));
+
+ assertThrows(AccumuloSecurityException.class,
+ () ->
auditAccumuloClient.tableOperations().delete(OLD_TEST_TABLE_NAME));
+
+ assertThrows(AccumuloSecurityException.class,
+ () ->
auditAccumuloClient.tableOperations().offline(OLD_TEST_TABLE_NAME));
+
try (Scanner scanner =
auditAccumuloClient.createScanner(OLD_TEST_TABLE_NAME, auths)) {
- scanner.iterator().next().getKey();
- } catch (RuntimeException ex) {}
- try {
- auditAccumuloClient.tableOperations().deleteRows(OLD_TEST_TABLE_NAME,
new Text("myRow"),
- new Text("myRow~"));
- } catch (AccumuloSecurityException ex) {}
- try {
- auditAccumuloClient.tableOperations().flush(OLD_TEST_TABLE_NAME, new
Text("myRow"),
- new Text("myRow~"), false);
- } catch (AccumuloSecurityException ex) {}
+ assertThrows(RuntimeException.class, () ->
scanner.iterator().next().getKey());
Review comment:
From this, it's not obvious which method is expected to throw. I think
it throws on the call to `next()`, but am not sure. If so, then the `getKey()`
method call should be dropped and the `scanner.iterator()` part be pulled out
to a variable outside the `assertThrows`.
##########
File path: test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java
##########
@@ -1460,7 +1460,9 @@ public void listscans() throws Exception {
SlowIterator.setSleepTime(cfg, 500);
s.addScanIterator(cfg);
- Thread thread = new Thread(() -> Iterators.size(s.iterator()));
+ Thread thread = new Thread(() -> {
+ assertTrue(Iterators.size(s.iterator()) > 0);
+ });
Review comment:
This is a weird one, because the assertion is happening inside a thread.
I'm not sure that will fail visibly. Could do:
```suggestion
var iterSize = new AtomicInteger(0);
Thread thread = new Thread(() ->
iterSize.set(Iterators.size(s.iterator())));
assertTrue(iterSize.get() > 0);
```
##########
File path:
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportPool.java
##########
@@ -835,8 +833,9 @@ public synchronized void setIdleTime(long time) {
log.debug("Set thrift transport pool idle time to {}", time);
}
+ // TODO consider re-working after #2554 is merged
void startCheckerThread() {
- checkThreadFactory.get();
+ final Thread thread = checkThreadFactory.get();
Review comment:
I agree this could be reworked, but this should be made a follow-on
ticket and the TODO removed from the code, prior to this PR being merged.
##########
File path: test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java
##########
@@ -1460,7 +1460,9 @@ public void listscans() throws Exception {
SlowIterator.setSleepTime(cfg, 500);
s.addScanIterator(cfg);
- Thread thread = new Thread(() -> Iterators.size(s.iterator()));
+ Thread thread = new Thread(() -> {
+ assertTrue(Iterators.size(s.iterator()) > 0);
+ });
Review comment:
This is a weird one, because the assertion is happening inside a thread.
I'm not sure that will fail visibly. Could do:
```suggestion
var iterSize = new AtomicInteger(0);
Thread thread = new Thread(() ->
iterSize.set(Iterators.size(s.iterator())));
assertTrue(iterSize.get() > 0);
```
##########
File path:
test/src/main/java/org/apache/accumulo/test/functional/ServerSideErrorIT.java
##########
@@ -63,69 +65,50 @@ public void run() throws Exception {
bw.addMutation(m);
}
- boolean caught = false;
// try to scan table
try (Scanner scanner = c.createScanner(tableName, Authorizations.EMPTY))
{
-
- try {
+ assertThrows(Exception.class, () -> {
for (Entry<Key,Value> entry : scanner) {
entry.getKey();
}
- } catch (Exception e) {
- caught = true;
- }
-
- if (!caught)
- throw new Exception("Scan did not fail");
-
- // try to batch scan the table
- try (BatchScanner bs = c.createBatchScanner(tableName,
Authorizations.EMPTY, 2)) {
- bs.setRanges(Collections.singleton(new Range()));
+ });
+ }
- caught = false;
- try {
- for (Entry<Key,Value> entry : bs) {
- entry.getKey();
- }
- } catch (Exception e) {
- caught = true;
+ // try to batch scan the table
+ try (BatchScanner bs = c.createBatchScanner(tableName,
Authorizations.EMPTY, 2)) {
+ bs.setRanges(Collections.singleton(new Range()));
+ assertThrows(Exception.class, () -> {
+ for (Entry<Key,Value> entry : bs) {
+ entry.getKey();
}
- }
-
- if (!caught)
- throw new Exception("batch scan did not fail");
-
- // remove the bad agg so accumulo can shutdown
- TableOperations to = c.tableOperations();
- for (Entry<String,String> e : to.getProperties(tableName)) {
- to.removeProperty(tableName, e.getKey());
- }
+ });
+ }
- sleepUninterruptibly(500, TimeUnit.MILLISECONDS);
+ // remove the bad agg so accumulo can shutdown
+ TableOperations to = c.tableOperations();
+ for (Entry<String,String> e : to.getProperties(tableName)) {
+ to.removeProperty(tableName, e.getKey());
}
+ sleepUninterruptibly(500, TimeUnit.MILLISECONDS);
+
// should be able to scan now
try (Scanner scanner = c.createScanner(tableName, Authorizations.EMPTY))
{
for (Entry<Key,Value> entry : scanner) {
- entry.getKey();
+ assertNotNull(entry.getKey());
}
// set a nonexistent iterator, should cause scan to fail on server side
scanner.addScanIterator(new IteratorSetting(100, "bogus",
"com.bogus.iterator"));
- caught = false;
- try {
+ assertThrows(Exception.class, () -> {
for (Entry<Key,Value> entry : scanner) {
// should error
entry.getKey();
}
- } catch (Exception e) {
- caught = true;
- }
-
- if (!caught)
- throw new Exception("Scan did not fail");
+ });
Review comment:
This could get more specific on the expected exception and the method
that throws.
##########
File path:
test/src/main/java/org/apache/accumulo/test/server/security/SystemCredentialsIT.java
##########
@@ -86,7 +87,7 @@ public static void main(final String[] args) throws
AccumuloException, TableNotF
client.securityOperations().authenticateUser(creds.getPrincipal(),
creds.getToken());
try (Scanner scan = client.createScanner(RootTable.NAME,
Authorizations.EMPTY)) {
for (Entry<Key,Value> e : scan) {
- e.hashCode();
+ assertNotNull(e.hashCode());
Review comment:
assertNotNull would force an auto-boxing of the hashCode int. That
part's probably okay, since we don't actually care about the value of the hash
code. However, I'd probably try to avoid a JUnit assert call inside the main
method. I'm not sure how the test would behave if the main method ended up
throwing an AssertionError instead of the expected System.exit from the below
checks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
