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 2022/03/09 20:04:02 UTC
[GitHub] [accumulo] jmark99 opened a new pull request #2558: Specify when return values are unused
jmark99 opened a new pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558
ErrorProne insists that return values be checked/used or specifically
acknowledged as unused. There are two error patterns related to these
issues:
CheckReturnValue:
(https://errorprone.info/bugpattern/CheckReturnValue)
and
ReturnValueIgnored:
(https://errorprone.info/bugpattern/ReturnValueIgnored)
In order to remove these cases from the ErrorProne configuration,
occurrences of unused return values were modified by creating an
'unusedRetVal' return value for each instance. The use of a variable
beginning with 'unused' prevents that instance from being flagged
as an ErrorProne error.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r824880313
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportPool.java
##########
@@ -836,7 +836,7 @@ public synchronized void setIdleTime(long time) {
}
void startCheckerThread() {
- checkThreadFactory.get();
+ var unusedRetVal = checkThreadFactory.get();
Review comment:
For the moment assigned a return value to the method call. Added note to re-work once #2554 is merged. Will create ticket to update once #2554 is merged.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r824867945
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportKey.java
##########
@@ -91,7 +91,7 @@ public boolean equals(Object o) {
}
public final void precomputeHashCode() {
- hashCode();
+ var unusedRetVal = hashCode();
Review comment:
Removed precomputeHashCode method and moved call to hashCode into constructors.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r824902199
##########
File path: test/src/main/java/org/apache/accumulo/test/SampleIT.java
##########
@@ -490,7 +490,7 @@ private void assertSampleNotPresent(SamplerConfiguration sc, ScannerBase... scan
scanner.clearSamplerConfiguration();
for (Entry<Key,Value> entry : scanner) {
- entry.getKey();
+ var unusedRetVal = entry.getKey();
Review comment:
Wrapped calls to entry.getKey() with assertNotNull in SampleIT, TImeoutIT, and ServerSideErrorIT.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 edited a comment on pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
jmark99 edited a comment on pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#issuecomment-1063318686
I put this PR in as a consideration as I am trying to remove ErrorProne errors without having to put an exception in the configuration. In order to do so, instances of method that are non-void must either use/check the return value or specifically acknowlege the return values are unused.
If this PR add too much 'wordiness' to the code then we can continue to keep the two checks turned off within the ErrorProne configuration and ignore this PR.
Let me know your thoughts.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r824857754
##########
File path: core/src/main/java/org/apache/accumulo/core/client/summary/CounterSummary.java
##########
@@ -54,7 +54,8 @@ public CounterSummary(Summary summary, boolean checkType) {
if (checkType) {
String className = summary.getSummarizerConfiguration().getClassName();
try {
- getClass().getClassLoader().loadClass(className).asSubclass(CountingSummarizer.class);
+ var unusedRetVal =
+ getClass().getClassLoader().loadClass(className).asSubclass(CountingSummarizer.class);
Review comment:
Added log.trace message to make use of unused return value from loadclass method.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r824917074
##########
File path: test/src/main/java/org/apache/accumulo/test/DetectDeadTabletServersIT.java
##########
@@ -56,7 +56,8 @@ protected void configure(MiniAccumuloConfigImpl cfg, Configuration hadoopCoreSit
public void test() throws Exception {
try (AccumuloClient c = Accumulo.newClient().from(getClientProperties()).build()) {
log.info("verifying that everything is up");
- Iterators.size(c.createScanner(MetadataTable.NAME, Authorizations.EMPTY).iterator());
+ var unusedRetVal =
+ Iterators.size(c.createScanner(MetadataTable.NAME, Authorizations.EMPTY).iterator());
Review comment:
Wrapped calls to Iterators.size with an asset to verify size > 0 in following classes:
DetectDeadTabletServersIT
GarbageCollectWALIT
ManagerRepairsDualAssignmentIT
MetaGetsReadersIT
MultiTableRecoveryIT
RecoveryCompactionsAreFlushesIT
UnusedWALIT
VerifySerialRecoveryIT
WaitForBalanceIT
GarbageCollectorIT
ReadWriteIT
KerberosReplicationIT
MultiInstanceReplicationIT
ReplicationIT
ShellServerIT
UnorderedWorkAssignerReplicationIT
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r823044360
##########
File path: core/src/main/java/org/apache/accumulo/core/client/summary/CounterSummary.java
##########
@@ -54,7 +54,8 @@ public CounterSummary(Summary summary, boolean checkType) {
if (checkType) {
String className = summary.getSummarizerConfiguration().getClassName();
try {
- getClass().getClassLoader().loadClass(className).asSubclass(CountingSummarizer.class);
+ var unusedRetVal =
+ getClass().getClassLoader().loadClass(className).asSubclass(CountingSummarizer.class);
Review comment:
This might make errorprone happy, but it adds compiler warnings about unused variables, at least in Eclipse (maybe also with javac?). One option is to actually use it. Could add a trace log message that uses it after it is assigned.
##########
File path: test/src/main/java/org/apache/accumulo/test/AuditMessageIT.java
##########
@@ -468,7 +468,7 @@ public void testDeniedAudits() throws AccumuloSecurityException, AccumuloExcepti
auditAccumuloClient.tableOperations().offline(OLD_TEST_TABLE_NAME);
} catch (AccumuloSecurityException ex) {}
try (Scanner scanner = auditAccumuloClient.createScanner(OLD_TEST_TABLE_NAME, auths)) {
- scanner.iterator().next().getKey();
+ var unusedRetVal = scanner.iterator().next().getKey();
} catch (RuntimeException ex) {}
Review comment:
It looks like this is a pattern where we are expecting the error. This should be refactored to use `assertThrows` instead of trying to ignore the return value.
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportKey.java
##########
@@ -91,7 +91,7 @@ public boolean equals(Object o) {
}
public final void precomputeHashCode() {
- hashCode();
+ var unusedRetVal = hashCode();
Review comment:
Instead of doing this, could just do this computation in the constructor and delete this method. The only place where this is used is immediately after construction in one case, and the only other time this object is constructed is in ServerClient where it is not going to hurt to precompute.
##########
File path: test/src/main/java/org/apache/accumulo/test/SampleIT.java
##########
@@ -490,7 +490,7 @@ private void assertSampleNotPresent(SamplerConfiguration sc, ScannerBase... scan
scanner.clearSamplerConfiguration();
for (Entry<Key,Value> entry : scanner) {
- entry.getKey();
+ var unusedRetVal = entry.getKey();
Review comment:
The call to `.getKey` was only ever added to avoid a previous warning about the loop variable being unused. It would probably be sufficient to have an assert that the loop variable is non-null.
##########
File path: test/src/main/java/org/apache/accumulo/test/CloseScannerIT.java
##########
@@ -58,7 +58,7 @@ public void testManyScans() throws Exception {
for (int j = 0; j < i % 7 + 1; j++) {
// only read a little data and quit, this should leave a session open on the tserver
- Iterators.get(scanner.iterator(), 10);
+ var unusedRetVal = Iterators.get(scanner.iterator(), 10);
Review comment:
An assertNotNull() or similar around this would probably suffice.
##########
File path: test/src/main/java/org/apache/accumulo/test/DetectDeadTabletServersIT.java
##########
@@ -56,7 +56,8 @@ protected void configure(MiniAccumuloConfigImpl cfg, Configuration hadoopCoreSit
public void test() throws Exception {
try (AccumuloClient c = Accumulo.newClient().from(getClientProperties()).build()) {
log.info("verifying that everything is up");
- Iterators.size(c.createScanner(MetadataTable.NAME, Authorizations.EMPTY).iterator());
+ var unusedRetVal =
+ Iterators.size(c.createScanner(MetadataTable.NAME, Authorizations.EMPTY).iterator());
Review comment:
This could verify the size is `> 0`.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
jmark99 commented on pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#issuecomment-1065368402
With the current changes it is possible to remove the EP exceptions in the configuration for both CheckReturnValue error and ReturnValueIgnored warning.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r840260357
##########
File path: test/src/main/java/org/apache/accumulo/test/server/security/SystemCredentialsIT.java
##########
@@ -85,8 +84,8 @@ public static void main(final String[] args) throws AccumuloException, TableNotF
.as(creds.getPrincipal(), creds.getToken()).build()) {
client.securityOperations().authenticateUser(creds.getPrincipal(), creds.getToken());
try (Scanner scan = client.createScanner(RootTable.NAME, Authorizations.EMPTY)) {
- for (Entry<Key,Value> e : scan) {
- e.hashCode();
+ while (scan.iterator().hasNext()) {
+ scan.iterator().next();
}
Review comment:
This won't work... calling `iterator()` multiple times on the scanner creates a new scan each time. You'd have to save the iterator to a variable and keep calling hasNext and next on that variable. Alternatively, you can do almost anything else with the `e` variable from the original code. The only reason `e.hashCode()` was called was to avoid a warning about the unused variable, `e`. Now, you could probably do something like:
```suggestion
scan.forEach((k,v) -> {});
```
If that generates a warning, you could maybe assign it to a variable: `private static final BiConsumer<Entry<Key,Value>> NOOP = (k,v) -> {};`
This same strategy could be used to replace other uses of Iterators.size, where we don't care about the size. That was only ever used as an easy way to iterate fully over the scanner without triggering a warning about unused variables. But, if it's causing warnings, it might be best to find a different solution that doesn't, instead of trying to chase the warnings by using the return value that we never cared about to begin with.
##########
File path: test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java
##########
@@ -1460,7 +1461,9 @@ public void listscans() throws Exception {
SlowIterator.setSleepTime(cfg, 500);
s.addScanIterator(cfg);
- Thread thread = new Thread(() -> Iterators.size(s.iterator()));
+ var iterSize = new AtomicInteger(0);
+ Thread thread = new Thread(() -> iterSize.set(Iterators.size(s.iterator())));
+ assertTrue(iterSize.get() > 0);
Review comment:
I'm pretty sure we only added .size here because it was the easiest way to fully iterate without causing a warning elsewhere about an unused iteration variable. But, if we're getting a warning anyway, a while-has-next-call-next loop might be simpler than trying to figure out what to do with the ignored size.
##########
File path: test/src/main/java/org/apache/accumulo/test/functional/ScannerContextIT.java
##########
@@ -71,7 +72,7 @@ protected int defaultTimeoutSeconds() {
@Before
public void checkCluster() throws Exception {
assumeTrue(getClusterType() == ClusterType.MINI);
- MiniAccumuloClusterImpl.class.cast(getCluster());
+ assertNotNull(MiniAccumuloClusterImpl.class.cast(getCluster()));
Review comment:
Would this work better, without warnings? It seems like it would make more sense, if it works.
```suggestion
assertTrue(MiniAccumuloClusterImpl.class.isInstance(getCluster()));
```
##########
File path: test/src/main/java/org/apache/accumulo/test/functional/ServerSideErrorIT.java
##########
@@ -63,69 +66,51 @@ public void run() throws Exception {
bw.addMutation(m);
}
- boolean caught = false;
// try to scan table
try (Scanner scanner = c.createScanner(tableName, Authorizations.EMPTY)) {
-
- try {
- for (Entry<Key,Value> entry : scanner) {
- entry.getKey();
+ Iterator<Entry<Key,Value>> iterator = scanner.iterator();
+ assertThrows(RuntimeException.class, () -> {
+ while (iterator.hasNext()) {
+ iterator.next();
}
- } 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()));
+ Iterator<Entry<Key,Value>> iterator = bs.iterator();
+ assertThrows(RuntimeException.class, () -> {
+ while (iterator.hasNext()) {
+ iterator.next();
}
- }
-
- 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 {
- for (Entry<Key,Value> entry : scanner) {
- // should error
- entry.getKey();
+ Iterator<Entry<Key,Value>> iterator = scanner.iterator();
+ assertThrows(RuntimeException.class, () -> {
+ while (iterator.hasNext()) {
+ iterator.next();
Review comment:
Is this expected to throw on `hasNext` or `next`? This assertThrows statement could be simplified by being more specific (if `hasNext`, no need to do anything else; if the first `next`, then no need to call `hasNext` or loop).
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r824885809
##########
File path: test/src/main/java/org/apache/accumulo/test/CloseScannerIT.java
##########
@@ -58,7 +58,7 @@ public void testManyScans() throws Exception {
for (int j = 0; j < i % 7 + 1; j++) {
// only read a little data and quit, this should leave a session open on the tserver
- Iterators.get(scanner.iterator(), 10);
+ var unusedRetVal = Iterators.get(scanner.iterator(), 10);
Review comment:
Wrapped call with assertNotNull to remove flagged EP error.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r823066645
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportKey.java
##########
@@ -91,7 +91,7 @@ public boolean equals(Object o) {
}
public final void precomputeHashCode() {
- hashCode();
+ var unusedRetVal = hashCode();
Review comment:
seems like in this case you could compute the `hash` in the ctor and get rid of this method
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r824869870
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportKey.java
##########
@@ -91,7 +91,7 @@ public boolean equals(Object o) {
}
public final void precomputeHashCode() {
- hashCode();
+ var unusedRetVal = hashCode();
Review comment:
Assigning the result of the hashCode to final int removes EP error. Is that going to cause other issues in Eclipse or elsewhere? If so, can re-work to make use of log.trace message or find another means to use return value. Did not want to add assertNonNull or similar in non-test 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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r824867784
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportKey.java
##########
@@ -91,7 +91,7 @@ public boolean equals(Object o) {
}
public final void precomputeHashCode() {
- hashCode();
+ var unusedRetVal = hashCode();
Review comment:
Removed precomputeHashCode method and moved call to hashCode into constructors.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r824917074
##########
File path: test/src/main/java/org/apache/accumulo/test/DetectDeadTabletServersIT.java
##########
@@ -56,7 +56,8 @@ protected void configure(MiniAccumuloConfigImpl cfg, Configuration hadoopCoreSit
public void test() throws Exception {
try (AccumuloClient c = Accumulo.newClient().from(getClientProperties()).build()) {
log.info("verifying that everything is up");
- Iterators.size(c.createScanner(MetadataTable.NAME, Authorizations.EMPTY).iterator());
+ var unusedRetVal =
+ Iterators.size(c.createScanner(MetadataTable.NAME, Authorizations.EMPTY).iterator());
Review comment:
Wrapped calls to Iterators.size with an asset to verify size > 0 in following classes:
DetectDeadTabletServersIT
GarbageCollectWALIT
ManagerRepairsDualAssignmentIT
MetaGetsReadersIT
MultiTableRecoveryIT
RecoveryCompactionsAreFlushesIT
UnusedWALIT
VerifySerialRecoveryIT
WaitForBalanceIT
GarbageCollectorIT
ReadWriteIT
KerberosReplicationIT
MultiInstanceReplicationIT
ReplicationIT
ShellServerIT
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r837864215
##########
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:
I reworked to replace the hashCode() call with an iterator.next(). Think that should serve similar purpose. @ctubbsii when you have a moment can you take another look at the changes I have made and see if they address your comments.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r837863217
##########
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:
Re-worked to separate the iterator() call from the next() call.
##########
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:
Re-worked to separate the iterator() call from the next() call.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
jmark99 commented on pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#issuecomment-1063318686
I put this PR in as a consideration as I am trying to remove ErrorProne errors without having to put an exception in the configuration. In order to do so, instances of method that are non-void must either use/check the return value or specifically acknowlege the return values are unused.
If this PR is too 'wordy' then we can continue to keep the two checks turned off within the ErrorProne configuration and ignore this PR.
Let me know your thoughts.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r823068958
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportPool.java
##########
@@ -836,7 +836,7 @@ public synchronized void setIdleTime(long time) {
}
void startCheckerThread() {
- checkThreadFactory.get();
+ var unusedRetVal = checkThreadFactory.get();
Review comment:
This can probably be reworked after #2554 is merged as ClientContext will now have a thread pool for close/cleanup type tasks
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on a change in pull request #2558: Specify when return values are unused
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #2558:
URL: https://github.com/apache/accumulo/pull/2558#discussion_r824871521
##########
File path: test/src/main/java/org/apache/accumulo/test/AuditMessageIT.java
##########
@@ -468,7 +468,7 @@ public void testDeniedAudits() throws AccumuloSecurityException, AccumuloExcepti
auditAccumuloClient.tableOperations().offline(OLD_TEST_TABLE_NAME);
} catch (AccumuloSecurityException ex) {}
try (Scanner scanner = auditAccumuloClient.createScanner(OLD_TEST_TABLE_NAME, auths)) {
- scanner.iterator().next().getKey();
+ var unusedRetVal = scanner.iterator().next().getKey();
} catch (RuntimeException ex) {}
Review comment:
Re-worked quite a few instances within AuditMessageIT to use assertThrows. Think I found all code portions that could use assertThrows regardless of whether it affected EP error patterns.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org