aherbert commented on code in PR #476:
URL:
https://github.com/apache/commons-collections/pull/476#discussion_r1562945737
##########
src/main/java/org/apache/commons/collections4/bloomfilter/LayeredBloomFilter.java:
##########
@@ -372,9 +372,18 @@ public boolean merge(IndexProducer indexProducer) {
* Forces and advance to the next layer. Executes the same logic as when
* LayerManager.extendCheck returns {@code true}
*
- * @see LayerManager
+ * @see LayerManager#next()
*/
public void next() {
layerManager.next();
}
+
+ /**
+ * Forces the execution of {@code LayerManager.clean()}.
+ *
+ * @see LayerManager#clear()
Review Comment:
Do you mean clear or clean?
You cannot reference clean as it is package-private. If you are referencing
clear then we should add a description to emphasis that clear will remove all
filters, whereas clean will run the clean-up filter.
Given the similarity between **clean** and **clear** perhaps we need more a
verbose method name here, e.g. cleanUp.
##########
src/main/java/org/apache/commons/collections4/bloomfilter/LayeredBloomFilter.java:
##########
@@ -372,9 +372,18 @@ public boolean merge(IndexProducer indexProducer) {
* Forces and advance to the next layer. Executes the same logic as when
* LayerManager.extendCheck returns {@code true}
*
- * @see LayerManager
+ * @see LayerManager#next()
*/
public void next() {
layerManager.next();
}
+
+ /**
+ * Forces the execution of {@code LayerManager.clean()}.
Review Comment:
The clean method in the LayerManager is package-private. So we cannot refer
to it using `{@link LayerManager#clean()}`. As such refering to it using
`@code` tags also makes no sense. I think this should be a text description of
what will happen (taken from the clean method):
```
Executes the configured LayerManager clean-up filter to remove expired
layers.
When this method returns there will be at least 1 layer in the filter.
Note this is different from a call to {@link #next()} which will invoke the
clean-up filter
<em>and</em> add a new layer. This method will only add a new layer if the
clean-up
removes all existing layers.
```
##########
src/test/java/org/apache/commons/collections4/bloomfilter/LayeredBloomFilterTest.java:
##########
@@ -311,4 +313,36 @@ public final void testNext() {
assertFalse(filter.get(1).contains(TestingHashers.FROM11));
assertTrue(filter.get(1).contains(new IncrementingHasher(11, 2)));
}
+
+ @Test
+ public void testClean() {
Review Comment:
I think this test is made redundant by testing the method on the
LayeredBloomFilter. In the interest of removing code duplication I would drop
this and just leave the test calling the public API on the filter.
##########
src/main/java/org/apache/commons/collections4/bloomfilter/LayerManager.java:
##########
@@ -380,4 +380,15 @@ void next() {
this.filterCleanup.accept(filters);
addFilter();
}
+
+ /**
+ * Forces execution the filterCleanup without creating a new filter except
in cases
Review Comment:
"of the configured clean-up filter used to manage expired layers"
##########
src/test/java/org/apache/commons/collections4/bloomfilter/LayerManagerTest.java:
##########
@@ -192,6 +193,37 @@ public void testNextAndGetDepth() {
assertEquals(2, underTest.getDepth());
}
+ @Test
+ public void testClean() {
+ int[] sequence = {1};
+ LayerManager underTest = LayerManager.builder()
+ .setSupplier(() -> new NumberedBloomFilter(shape, 3,
sequence[0]++))
+ .setExtendCheck(ExtendCheck.neverAdvance())
+ .setCleanup(ll -> ll.removeIf( f -> (((NumberedBloomFilter)
f).value-- == 0))).build();
Review Comment:
Remove space before f: `(f -> ((`
--
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]
