busbey commented on a change in pull request #2685:
URL: https://github.com/apache/hbase/pull/2685#discussion_r527709447
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java
##########
@@ -294,7 +294,14 @@ private static CompressionCodec getCompressor(final
Configuration conf) {
* @return the maximum pool size
*/
private static int getPoolSize(Configuration config) {
- return config.getInt(HConstants.HBASE_CLIENT_IPC_POOL_SIZE, 1);
+ int poolSize = config.getInt(HConstants.HBASE_CLIENT_IPC_POOL_SIZE, 1);
+
+ if (poolSize <= 0) {
+ LOG.warn("{} must be positive.", HConstants.HBASE_CLIENT_IPC_POOL_SIZE);
Review comment:
include in the message that we're ignoring the passed config and using 1.
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/util/PoolMap.java
##########
@@ -45,6 +38,11 @@
* key. A size of {@link Integer#MAX_VALUE} is interpreted as an unbounded
pool.
* </p>
*
+ * <p>
+ * Pool is not thread-safe. It must be synchronized when used by multiple
threads. Pool also does
Review comment:
PoolMap here, yeah?
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/util/PoolMap.java
##########
@@ -275,43 +272,70 @@ public static PoolType fuzzyMatch(String name) {
*
*/
@SuppressWarnings("serial")
- static class RoundRobinPool<R> extends CopyOnWriteArrayList<R> implements
Pool<R> {
- private int maxSize;
- private int nextResource = 0;
+ static class RoundRobinPool<R> implements Pool<R> {
+ private final List<R> resources;
+ private final int maxSize;
+
+ private int nextIndex;
public RoundRobinPool(int maxSize) {
+ if (maxSize <= 0) {
+ throw new IllegalArgumentException("maxSize must be positive");
+ }
+
+ resources = new ArrayList<>(maxSize);
this.maxSize = maxSize;
}
@Override
- public R put(R resource) {
- if (super.size() < maxSize) {
- add(resource);
+ public R get() {
+ int size = resources.size();
+
+ /* letting pool to grow */
+ if (size < maxSize) {
+ return null;
}
+
+ R resource = resources.get(nextIndex);
+
+ /* at this point size cannot be 0 */
+ nextIndex = (nextIndex + 1) % size;
+
+ return resource;
+ }
+
+ @Override
+ public R put(R resource) {
+ resources.add(resource);
Review comment:
should we be returning null? I don't see any place we actually use the
returned value.
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/util/PoolMap.java
##########
@@ -275,43 +272,70 @@ public static PoolType fuzzyMatch(String name) {
*
*/
@SuppressWarnings("serial")
- static class RoundRobinPool<R> extends CopyOnWriteArrayList<R> implements
Pool<R> {
- private int maxSize;
- private int nextResource = 0;
+ static class RoundRobinPool<R> implements Pool<R> {
+ private final List<R> resources;
+ private final int maxSize;
+
+ private int nextIndex;
public RoundRobinPool(int maxSize) {
+ if (maxSize <= 0) {
+ throw new IllegalArgumentException("maxSize must be positive");
+ }
+
+ resources = new ArrayList<>(maxSize);
this.maxSize = maxSize;
}
@Override
- public R put(R resource) {
- if (super.size() < maxSize) {
- add(resource);
+ public R get() {
+ int size = resources.size();
+
+ /* letting pool to grow */
Review comment:
this feels backwards, since we return null to indicate that something
outside of the pool should create another pooled resource.
Can we define the Pool interface such that a given instance of Pool can
handle this creation as an internal detail? something analogous to TheadLocal's
initialize method?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
