kevinrr888 commented on code in PR #5853:
URL: https://github.com/apache/accumulo/pull/5853#discussion_r2322768633
##########
test/src/main/java/org/apache/accumulo/test/functional/CompactionIT.java:
##########
@@ -1067,21 +1067,26 @@ public void testOfflineAndCompactions() throws
Exception {
// start a bunch of compactions in the background
var executor = Executors.newCachedThreadPool();
- List<Future<?>> futures = new ArrayList<>();
+ final int numTasks = 20;
+ List<Future<?>> futures = new ArrayList<>(numTasks);
+ CountDownLatch startLatch = new CountDownLatch(numTasks);
// start user compactions on a subset of the tables tablets, system
compactions should attempt
// to run on all tablets. With concurrency should get a mix.
- for (int i = 1; i < 20; i++) {
+ for (int i = 0; i < numTasks; i++) {
var startRow = new Text(String.format("r:%04d", i - 1));
var endRow = new Text(String.format("r:%04d", i));
Review Comment:
Seems like this change from i starting at 1 to 0 might be an issue with the
(i - 1). May want to change this back or address this (i - 1) in some way.
```suggestion
for (int i = 1; i < numTasks + 1; i++) {
var startRow = new Text(String.format("r:%04d", i - 1));
var endRow = new Text(String.format("r:%04d", i));
```
##########
test/src/main/java/org/apache/accumulo/test/UniqueNameAllocatorIT.java:
##########
@@ -68,10 +69,14 @@ void testConcurrentNameGeneration() throws Exception {
var executorService = Executors.newCachedThreadPool();
List<Future<Integer>> futures = new ArrayList<>();
+ CountDownLatch startLatch = new CountDownLatch(32); // start a portion of
threads at the same
+ // time
Review Comment:
```suggestion
// start a portion of threads at the same time
CountDownLatch startLatch = new CountDownLatch(32);
```
##########
test/src/main/java/org/apache/accumulo/test/functional/FateStarvationIT.java:
##########
@@ -83,20 +85,26 @@ public void run() throws Exception {
List<Text> splits = new ArrayList<>(TestIngest.getSplitPoints(0, 100000,
67));
- List<Future<?>> futures = new ArrayList<>();
+ int numTasks = 100;
+ List<Future<?>> futures = new ArrayList<>(numTasks);
var executor = Executors.newCachedThreadPool();
+ CountDownLatch startLatch = new CountDownLatch(32); // wait for a
portion of the tasks to be
+ // ready
Review Comment:
```suggestion
// wait for a portion of the tasks to be ready
CountDownLatch startLatch = new CountDownLatch(32);
```
Again here, is there a reason to not sync start of them all: `new
CountDownLatch(numTasks)`
##########
test/src/main/java/org/apache/accumulo/test/WriteAfterCloseIT.java:
##########
@@ -141,12 +142,24 @@ public void testWriteAfterClose(TimeType timeType,
boolean killTservers, long ti
try (AccumuloClient c = Accumulo.newClient().from(props).build()) {
c.tableOperations().create(table, ntc);
- List<Future<?>> futures = new ArrayList<>();
-
- for (int i = 0; i < 100; i++) {
- futures.add(
- executor.submit(createWriteTask(i * 1000, c, table, timeout,
useConditionalWriter)));
+ int numTasks = 100;
+ List<Future<?>> futures = new ArrayList<>(numTasks);
+ CountDownLatch startLatch = new CountDownLatch(32); // synchronize start
of a portion of the
+ // tasks
Review Comment:
why not all of them?
##########
core/src/test/java/org/apache/accumulo/core/file/rfile/bcfile/CompressionTest.java:
##########
@@ -285,8 +299,9 @@ public void testThereCanBeOnlyOne() throws
InterruptedException, ExecutionExcept
return true;
});
}
+ assertEquals(numTasks, list.size());
- results.addAll(service.invokeAll(list));
+ ArrayList<Future<Boolean>> results = new
ArrayList<>(service.invokeAll(list));
// ensure that we
Review Comment:
Could update or delete this comment
##########
core/src/test/java/org/apache/accumulo/core/clientImpl/ClientTabletCacheImplTest.java:
##########
@@ -2024,6 +2029,7 @@ public void testMultithreadedLookups() throws Exception {
futures.add(future);
}
}
+ assertEquals(roundCount * lookupCount, futures.size());
Review Comment:
```suggestion
assertEquals(numTasks, futures.size());
```
##########
test/src/main/java/org/apache/accumulo/test/functional/FateStarvationIT.java:
##########
@@ -83,20 +85,26 @@ public void run() throws Exception {
List<Text> splits = new ArrayList<>(TestIngest.getSplitPoints(0, 100000,
67));
- List<Future<?>> futures = new ArrayList<>();
+ int numTasks = 100;
+ List<Future<?>> futures = new ArrayList<>(numTasks);
var executor = Executors.newCachedThreadPool();
+ CountDownLatch startLatch = new CountDownLatch(32); // wait for a
portion of the tasks to be
+ // ready
- for (int i = 0; i < 100; i++) {
+ for (int i = 0; i < numTasks; i++) {
int idx1 = RANDOM.get().nextInt(splits.size() - 1);
int idx2 = RANDOM.get().nextInt(splits.size() - (idx1 + 1)) + idx1 + 1;
var future = executor.submit(() -> {
- c.tableOperations().compact(tableName, splits.get(idx1),
splits.get(idx2), false, true);
+ startLatch.countDown();
+ startLatch.await();
+ c.tableOperations().compact(tableName, splits.get(idx1),
splits.get(idx2), false, false);
Review Comment:
```suggestion
c.tableOperations().compact(tableName, splits.get(idx1),
splits.get(idx2), false, true);
```
We still want to wait for these compactions to complete, otherwise the
completion of the future doesn't necessarily mean the completion of the
compaction, which the test relies on
##########
test/src/main/java/org/apache/accumulo/test/functional/BulkNewIT.java:
##########
@@ -1102,8 +1111,11 @@ public void testManyTabletAndFiles() throws Exception {
.collect(Collectors.toCollection(TreeSet::new));
c.tableOperations().addSplits(tableName, splits);
- var executor = Executors.newFixedThreadPool(16);
- var futures = new ArrayList<Future<?>>();
+ final int numTasks = 16;
+ var executor = Executors.newFixedThreadPool(numTasks);
+ var futures = new ArrayList<Future<?>>(numTasks);
+ CountDownLatch startLatch = new CountDownLatch(numTasks); // wait for a
portion of the tasks
+ // to be ready
Review Comment:
```suggestion
// wait for a portion of the tasks to be ready
CountDownLatch startLatch = new CountDownLatch(numTasks);
```
##########
test/src/main/java/org/apache/accumulo/test/ScanServerMultipleScansIT.java:
##########
@@ -190,7 +191,8 @@ public void testMultipleScansDifferentTablets() throws
Exception {
final int threadNum = i;
var future = executor.submit(() -> {
try {
- latch.await();
+ startLatch.countDown();
+ startLatch.await();
} catch (InterruptedException e1) {
fail("InterruptedException waiting for latch");
Review Comment:
```suggestion
fail("InterruptedException waiting for startLatch");
```
for consistency with other test
##########
test/src/main/java/org/apache/accumulo/test/WriteAfterCloseIT.java:
##########
@@ -141,12 +142,24 @@ public void testWriteAfterClose(TimeType timeType,
boolean killTservers, long ti
try (AccumuloClient c = Accumulo.newClient().from(props).build()) {
c.tableOperations().create(table, ntc);
- List<Future<?>> futures = new ArrayList<>();
-
- for (int i = 0; i < 100; i++) {
- futures.add(
- executor.submit(createWriteTask(i * 1000, c, table, timeout,
useConditionalWriter)));
+ int numTasks = 100;
+ List<Future<?>> futures = new ArrayList<>(numTasks);
+ CountDownLatch startLatch = new CountDownLatch(32); // synchronize start
of a portion of the
+ // tasks
Review Comment:
```suggestion
// synchronize start of a portion of the tasks
CountDownLatch startLatch = new CountDownLatch(32);
```
##########
core/src/test/java/org/apache/accumulo/core/util/LockMapTest.java:
##########
@@ -97,13 +101,17 @@ public void testConcurrency() throws Exception {
var booleans = new AtomicBoolean[] {new AtomicBoolean(), new
AtomicBoolean(),
new AtomicBoolean(), new AtomicBoolean(), new AtomicBoolean()};
- var futures = new ArrayList<Future<Boolean>>();
+ var futures = new ArrayList<Future<Boolean>>(numTasks);
+ CountDownLatch startLatch = new CountDownLatch(numThreads); // start a
portion of threads at
+ // the same
time
Review Comment:
```suggestion
var futures = new ArrayList<Future<Boolean>>(numTasks);
// start a portion of threads at the same time
CountDownLatch startLatch = new CountDownLatch(numThreads);
```
--
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]
