dsmiley commented on a change in pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#discussion_r429352375
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -38,7 +40,13 @@
public class SolrRequestInfo {
- protected final static ThreadLocal<SolrRequestInfo> threadLocal = new
ThreadLocal<>();
+
+ protected final static int capacity = 150;
+
+ protected final static ThreadLocal<Deque<SolrRequestInfo>> threadLocal =
ThreadLocal.withInitial(() -> {
+ Deque<SolrRequestInfo> stack = new ArrayDeque<>(capacity);
Review comment:
I think you can simply do a lambda reference to ArrayDequeue::new. No
need initialize with an internal capacity. Honestly I'd prefer a LinkedList
because in practice this stack will be extremely small (zero, one, sometimes
two, very unlikely more). But I leave that impl choice to you.
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -38,7 +40,13 @@
public class SolrRequestInfo {
- protected final static ThreadLocal<SolrRequestInfo> threadLocal = new
ThreadLocal<>();
+
+ protected final static int capacity = 150;
Review comment:
This should be MAX_STACK_SIZE I think.
Can anyone ( @mkhludnev ?) venture to guess how, realistically, we might
reach upwards of 150? I can't imagine more than a few, let alone 150.
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +60,48 @@
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static SolrRequestInfo getRequestInfo() {
- return threadLocal.get();
+ if (threadLocal.get().isEmpty()) return null;
+ return threadLocal.get().peek();
}
public static void setRequestInfo(SolrRequestInfo info) {
- // TODO: temporary sanity check... this can be changed to just an assert
in the future
- SolrRequestInfo prev = threadLocal.get();
- if (prev != null) {
- log.error("Previous SolrRequestInfo was not closed! req={}",
prev.req.getOriginalParams());
- log.error("prev == info : {}", prev.req == info.req, new
RuntimeException());
+ if (info == null) {
+ throw new IllegalArgumentException("SolrRequestInfo is null");
+ } else {
+ if (threadLocal.get().size() <= capacity) {
+ threadLocal.get().push(info);
+ } else {
+ log.error("SolrRequestInfo Stack is full");
+ }
}
- assert prev == null;
-
- threadLocal.set(info);
}
public static void clearRequestInfo() {
- try {
- SolrRequestInfo info = threadLocal.get();
- if (info != null && info.closeHooks != null) {
- for (Closeable hook : info.closeHooks) {
- try {
- hook.close();
- } catch (Exception e) {
- SolrException.log(log, "Exception during close hook", e);
- }
+ if (threadLocal.get().isEmpty()) {
+ log.error("clearRequestInfo called too many times");
+ } else {
+ SolrRequestInfo info = threadLocal.get().pop();
+ closeHooks(info);
+ }
+ }
+
+ public static void reset() {
Review comment:
Add javadoc please
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +60,48 @@
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static SolrRequestInfo getRequestInfo() {
- return threadLocal.get();
+ if (threadLocal.get().isEmpty()) return null;
+ return threadLocal.get().peek();
}
public static void setRequestInfo(SolrRequestInfo info) {
Review comment:
Add javadoc please
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +60,48 @@
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static SolrRequestInfo getRequestInfo() {
- return threadLocal.get();
+ if (threadLocal.get().isEmpty()) return null;
+ return threadLocal.get().peek();
}
public static void setRequestInfo(SolrRequestInfo info) {
- // TODO: temporary sanity check... this can be changed to just an assert
in the future
- SolrRequestInfo prev = threadLocal.get();
- if (prev != null) {
- log.error("Previous SolrRequestInfo was not closed! req={}",
prev.req.getOriginalParams());
- log.error("prev == info : {}", prev.req == info.req, new
RuntimeException());
+ if (info == null) {
+ throw new IllegalArgumentException("SolrRequestInfo is null");
+ } else {
+ if (threadLocal.get().size() <= capacity) {
+ threadLocal.get().push(info);
+ } else {
+ log.error("SolrRequestInfo Stack is full");
+ }
}
- assert prev == null;
-
- threadLocal.set(info);
}
public static void clearRequestInfo() {
Review comment:
Add javadoc please
----------------------------------------------------------------
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
