ctubbsii commented on code in PR #3508:
URL: https://github.com/apache/accumulo/pull/3508#discussion_r1239328065
##########
test/src/main/java/org/apache/accumulo/test/functional/MemoryFreeingIterator.java:
##########
@@ -28,23 +28,39 @@
import org.apache.accumulo.core.iterators.IteratorEnvironment;
import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
import org.apache.accumulo.core.iterators.WrappingIterator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
public class MemoryFreeingIterator extends WrappingIterator {
- @Override
- public void init(SortedKeyValueIterator<Key,Value> source,
Map<String,String> options,
- IteratorEnvironment env) throws IOException {
- super.init(source, options, env);
+ private static final Logger LOG =
LoggerFactory.getLogger(MemoryFreeingIterator.class);
+
+ @SuppressFBWarnings(value = "DM_GC", justification = "gc is okay for test")
+ public MemoryFreeingIterator() {
+ super();
Review Comment:
Don't need this
```suggestion
```
##########
test/src/main/java/org/apache/accumulo/test/functional/MemoryFreeingIterator.java:
##########
@@ -28,23 +28,39 @@
import org.apache.accumulo.core.iterators.IteratorEnvironment;
import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
import org.apache.accumulo.core.iterators.WrappingIterator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
public class MemoryFreeingIterator extends WrappingIterator {
- @Override
- public void init(SortedKeyValueIterator<Key,Value> source,
Map<String,String> options,
- IteratorEnvironment env) throws IOException {
- super.init(source, options, env);
+ private static final Logger LOG =
LoggerFactory.getLogger(MemoryFreeingIterator.class);
+
+ @SuppressFBWarnings(value = "DM_GC", justification = "gc is okay for test")
+ public MemoryFreeingIterator() {
+ super();
+ LOG.info("Freeing consumed memory");
MemoryConsumingIterator.freeBuffers();
while (this.isRunningLowOnMemory()) {
+ System.gc();
// wait for LowMemoryDetector to recognize the memory is free.
try {
Thread.sleep(SECONDS.toMillis(1));
} catch (InterruptedException ex) {
Thread.currentThread().interrupt();
- throw new IOException("wait for low memory detector interrupted", ex);
+ throw new RuntimeException("wait for low memory detector interrupted",
ex);
}
}
+ LOG.info("Consumed memory freed");
+ }
+
+ @Override
+ @SuppressFBWarnings(value = "DM_GC", justification = "gc is okay for test")
+ public void init(SortedKeyValueIterator<Key,Value> source,
Map<String,String> options,
+ IteratorEnvironment env) throws IOException {
+ super.init(source, options, env);
+ LOG.info("init call - should free consumed memory");
Review Comment:
What's the reasoning for moving the content of this method into the
constructor? If you do need to move stuff to the constructor, you probably
don't need this method at all.
Even if you keep it for some kind of log message, you don't need the
warnings suppression... there is no garbage collection happening in this method
anymore. Also, the log message itself seems incorrect, since it's not freeing
anything.
##########
test/src/main/java/org/apache/accumulo/test/functional/MemoryFreeingIterator.java:
##########
@@ -28,23 +28,39 @@
import org.apache.accumulo.core.iterators.IteratorEnvironment;
import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
import org.apache.accumulo.core.iterators.WrappingIterator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
public class MemoryFreeingIterator extends WrappingIterator {
- @Override
- public void init(SortedKeyValueIterator<Key,Value> source,
Map<String,String> options,
- IteratorEnvironment env) throws IOException {
- super.init(source, options, env);
+ private static final Logger LOG =
LoggerFactory.getLogger(MemoryFreeingIterator.class);
+
+ @SuppressFBWarnings(value = "DM_GC", justification = "gc is okay for test")
+ public MemoryFreeingIterator() {
+ super();
+ LOG.info("Freeing consumed memory");
MemoryConsumingIterator.freeBuffers();
while (this.isRunningLowOnMemory()) {
+ System.gc();
Review Comment:
Calling this may do nothing at all. Is that okay in this test?
##########
test/src/main/java/org/apache/accumulo/test/util/Wait.java:
##########
@@ -48,4 +50,13 @@ public static boolean waitFor(final Condition condition,
final long duration,
}
return conditionSatisfied;
}
+
+ /**
+ * A retry for use in junit tests when testing asynchronous conditions that
are expected to
+ * eventually meet the condition or fail the test. Using this method should
be used instead of an
+ * arbitrary sleep and hoping to catch the condition in the expected state.
+ */
+ public static void assertTrueWithRetry(final Wait.Condition condition)
throws Exception {
+ assertTrue(waitFor(condition));
+ }
Review Comment:
Using this method isn't any better than not using it. I don't think this is
really adding much value here.
```java
assertTrueWithRetry(x);
assertTrue(waitFor(x));
```
Also, it seems like somebody was trying to avoid adding JUnit assertions
into this class directly, otherwise, I'd expect the waitFor methods to already
do this assertion... because everywhere we use it, we already assertTrue on the
returned value... except in a few places where it looks like we forgot to check
the return value on the assumption that something gets thrown if the timeout is
exceeded... but that's not true.
So, if we want to do anything here in this Wait class, it'd be better to
just bake in the assertTrue at the end of the existing waitFor method.
##########
test/src/main/java/org/apache/accumulo/test/functional/MemoryStarvedScanIT.java:
##########
@@ -162,13 +173,11 @@ private void consumeServerMemory(BatchScanner scanner) {
assertTrue(iter.hasNext());
}
- static void freeServerMemory(AccumuloClient client, String table) throws
Exception {
- try (Scanner scanner = client.createScanner(table)) {
- scanner.addScanIterator(new IteratorSetting(11,
MemoryFreeingIterator.class, Map.of()));
- @SuppressWarnings("unused")
- Iterator<Entry<Key,Value>> iter = scanner.iterator(); // init'ing the
iterator should be
- // enough to free
the memory
- }
+ static void freeServerMemory(AccumuloClient client) throws Exception {
+ // Instantiating this class on the TabletServer will free the memory as it
+ // frees the buffers created by the MemoryConsumingIterator in its
constructor.
+
client.instanceOperations().testClassLoad(MemoryFreeingIterator.class.getName(),
+ WrappingIterator.class.getCanonicalName());
Review Comment:
In this case, it doesn't matter, but you can't use canonical name to specify
the type in the general case.
```suggestion
WrappingIterator.class.getName());
```
##########
test/src/main/java/org/apache/accumulo/test/functional/MemoryFreeingIterator.java:
##########
@@ -28,23 +28,39 @@
import org.apache.accumulo.core.iterators.IteratorEnvironment;
import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
import org.apache.accumulo.core.iterators.WrappingIterator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
public class MemoryFreeingIterator extends WrappingIterator {
- @Override
- public void init(SortedKeyValueIterator<Key,Value> source,
Map<String,String> options,
- IteratorEnvironment env) throws IOException {
- super.init(source, options, env);
+ private static final Logger LOG =
LoggerFactory.getLogger(MemoryFreeingIterator.class);
+
+ @SuppressFBWarnings(value = "DM_GC", justification = "gc is okay for test")
+ public MemoryFreeingIterator() {
+ super();
+ LOG.info("Freeing consumed memory");
Review Comment:
Well, it's attempting to anyway... there's no guarantee here.
```suggestion
LOG.info("Attempting to free consumed memory");
```
--
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]
