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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
