abhishekagarwal87 commented on a change in pull request #12298:
URL: https://github.com/apache/druid/pull/12298#discussion_r820646525
##########
File path:
core/src/main/java/org/apache/druid/java/util/http/client/pool/ResourcePool.java
##########
@@ -189,17 +190,63 @@ private ImmediateCreationResourceHolder(
);
}
}
+ }
+
+ private static class LazyCreationResourceHolder<K, V> extends
ResourceHolderPerKey<K, V>
+ {
+ private LazyCreationResourceHolder(
+ int maxSize,
+ long unusedResourceTimeoutMillis,
+ K key,
+ ResourceFactory<K, V> factory
+ )
+ {
+ super(maxSize, unusedResourceTimeoutMillis, key, factory);
+ }
+ }
+
+ private static class ResourceHolderPerKey<K, V> implements Closeable
+ {
+ protected final int maxSize;
+ private final K key;
+ private final ResourceFactory<K, V> factory;
+ private final long unusedResourceTimeoutMillis;
+ // Hold previously created / returned resources
+ protected final ArrayDeque<ResourceHolder<V>> resourceHolderList;
+ // Maintains count of times when get() threw an exception.
Review comment:
can you add more details @AmatyaAvadhanula. Is it just a counter? What
is its purpose
##########
File path:
core/src/main/java/org/apache/druid/java/util/http/client/pool/ResourcePool.java
##########
@@ -189,17 +190,63 @@ private ImmediateCreationResourceHolder(
);
}
}
+ }
+
+ private static class LazyCreationResourceHolder<K, V> extends
ResourceHolderPerKey<K, V>
+ {
+ private LazyCreationResourceHolder(
+ int maxSize,
+ long unusedResourceTimeoutMillis,
+ K key,
+ ResourceFactory<K, V> factory
+ )
+ {
+ super(maxSize, unusedResourceTimeoutMillis, key, factory);
+ }
+ }
+
+ private static class ResourceHolderPerKey<K, V> implements Closeable
+ {
+ protected final int maxSize;
+ private final K key;
+ private final ResourceFactory<K, V> factory;
+ private final long unusedResourceTimeoutMillis;
+ // Hold previously created / returned resources
+ protected final ArrayDeque<ResourceHolder<V>> resourceHolderList;
+ // Maintains count of times when get() threw an exception.
+ private int deficit = 0;
+ // Maintains count of tiems when get() successfully returned a resource.
+ private int numLentResources = 0;
+ private boolean closed = false;
+
+ protected ResourceHolderPerKey(
+ int maxSize,
+ long unusedResourceTimeoutMillis,
+ K key,
+ ResourceFactory<K, V> factory
+ )
+ {
+ this.maxSize = maxSize;
+ this.key = key;
+ this.factory = factory;
+ this.unusedResourceTimeoutMillis = unusedResourceTimeoutMillis;
+ this.resourceHolderList = new ArrayDeque<>();
+ }
/**
* Returns a resource or null if this holder is already closed or the
current thread is interrupted.
+ *
+ * Try to return a previously created resource if it isGood(). Else,
generate a new resource
+ *
+ * If an exception occurs, keep count so that the deficit can be filled
with newly generated resources as required.
Review comment:
can you elaborate further and put in more details about the relationship
between numLentResources and deficit?
##########
File path:
core/src/main/java/org/apache/druid/java/util/http/client/pool/ResourcePool.java
##########
@@ -249,6 +302,7 @@ V get()
throw new RuntimeException(e);
}
+ numLentResources++;
Review comment:
Please move it to right after you take an item out of
resourceHolderList. That will instil more confidence that this variable is not
out of sync with resourceHolderList.
##########
File path:
core/src/main/java/org/apache/druid/java/util/http/client/pool/ResourcePool.java
##########
@@ -212,15 +259,21 @@ V get()
if (closed) {
log.info(StringUtils.format("get() called even though I'm closed.
key[%s]", key));
return null;
- } else if (!resourceHolderList.isEmpty()) {
- ResourceHolder<V> holder = resourceHolderList.removeFirst();
- if (System.currentTimeMillis() - holder.getLastAccessedTime() >
unusedResourceTimeoutMillis) {
- factory.close(holder.getResource());
+ } else if (numLentResources + deficit < maxSize) {
+ // Attempt to take an existing resource or create one if list is
empty
+ if (resourceHolderList.isEmpty()) {
poolVal = factory.generate(key);
} else {
- poolVal = holder.getResource();
+ ResourceHolder<V> holder = resourceHolderList.removeFirst();
+ if (System.currentTimeMillis() - holder.getLastAccessedTime() >
unusedResourceTimeoutMillis) {
+ factory.close(holder.getResource());
Review comment:
hmm. what happens if there is an exception here. we would have removed
an item from the list but the item would not have been handed over to the
consumer. so our pool effectively shrinks by 1. This is an existing behaviour
though.
am I thinking it wrong?
##########
File path:
core/src/main/java/org/apache/druid/java/util/http/client/pool/ResourcePool.java
##########
@@ -212,15 +259,21 @@ V get()
if (closed) {
log.info(StringUtils.format("get() called even though I'm closed.
key[%s]", key));
return null;
- } else if (!resourceHolderList.isEmpty()) {
- ResourceHolder<V> holder = resourceHolderList.removeFirst();
- if (System.currentTimeMillis() - holder.getLastAccessedTime() >
unusedResourceTimeoutMillis) {
- factory.close(holder.getResource());
+ } else if (numLentResources + deficit < maxSize) {
+ // Attempt to take an existing resource or create one if list is
empty
+ if (resourceHolderList.isEmpty()) {
poolVal = factory.generate(key);
} else {
- poolVal = holder.getResource();
+ ResourceHolder<V> holder = resourceHolderList.removeFirst();
+ if (System.currentTimeMillis() - holder.getLastAccessedTime() >
unusedResourceTimeoutMillis) {
+ factory.close(holder.getResource());
+ poolVal = factory.generate(key);
+ } else {
+ poolVal = holder.getResource();
+ }
}
} else if (deficit > 0) {
+ // We are allowed to generate to fill the deficit objects
Review comment:
what are deficit objects? From your earlier comment, deficit is just the
number of exceptions.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
