[GitHub] [bookkeeper] eolivelli commented on a change in pull request #3061: Optimize memory:Support shrinking in ConcurrentLongLongPairHashMap

2022-02-19 Thread GitBox


eolivelli commented on a change in pull request #3061:
URL: https://github.com/apache/bookkeeper/pull/3061#discussion_r810505974



##
File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/util/collections/ConcurrentLongLongPairHashMap.java
##
@@ -220,22 +329,36 @@ public void forEach(BiConsumerLongPair processor) {
 }
 
 // A section is a portion of the hash map that is covered by a single
+@Setter
 @SuppressWarnings("serial")
 private static final class Section extends StampedLock {
 // Keys and values are stored interleaved in the table array
 private volatile long[] table;
 
 private volatile int capacity;
 private volatile int size;
-private int usedBuckets;
-private int resizeThreshold;
-
-Section(int capacity) {
+private volatile int usedBuckets;
+private int resizeThresholdUp;
+private int resizeThresholdBelow;
+private volatile float mapFillFactor;

Review comment:
   Why do we want these to be modifiable at runtime?
   I cannot find a good usecase.
   
   We should have a smaller API, please remember that every public method is 
'public API' and we will have to maintain it I'm the future

##
File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/util/collections/ConcurrentLongLongPairHashMap.java
##
@@ -30,6 +30,8 @@
 import java.util.Map;
 import java.util.concurrent.locks.StampedLock;
 
+import lombok.Setter;

Review comment:
   This probably useless 




-- 
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: issues-unsubscr...@bookkeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [bookkeeper] eolivelli commented on a change in pull request #3061: Optimize memory:Support shrinking in ConcurrentLongLongPairHashMap

2022-02-17 Thread GitBox


eolivelli commented on a change in pull request #3061:
URL: https://github.com/apache/bookkeeper/pull/3061#discussion_r809730137



##
File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/util/collections/ConcurrentLongLongPairHashMap.java
##
@@ -75,26 +133,21 @@
 boolean test(long key1, long key2, long value1, long value2);
 }
 
-public ConcurrentLongLongPairHashMap() {
-this(DefaultExpectedItems);
-}
-
-public ConcurrentLongLongPairHashMap(int expectedItems) {
-this(expectedItems, DefaultConcurrencyLevel);
-}
-
-public ConcurrentLongLongPairHashMap(int expectedItems, int 
concurrencyLevel) {
+private ConcurrentLongLongPairHashMap(int expectedItems, int 
concurrencyLevel,

Review comment:
   I would keep and Deprecate all the public constructors.
   Otherwise this is a breaking change and we cannot port it to older releases




-- 
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: issues-unsubscr...@bookkeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org