dsmiley commented on a change in pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#discussion_r434081511
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +56,53 @@
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static SolrRequestInfo getRequestInfo() {
- return threadLocal.get();
+ if (threadLocal.get().isEmpty()) return null;
Review comment:
Instead of calling threadLocal.get() twice, can you please just call it
once? This goes for all the other methods in this class too.
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +56,53 @@
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();
}
+ /** Adds the SolrRequestInfo onto the stack provided that the stack is not
reached MAX_STACK_SIZE */
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() <= MAX_STACK_SIZE) {
+ threadLocal.get().push(info);
+ } else {
+ log.error("SolrRequestInfo Stack is full");
Review comment:
add assertion
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +56,53 @@
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();
}
+ /** Adds the SolrRequestInfo onto the stack provided that the stack is not
reached MAX_STACK_SIZE */
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() <= MAX_STACK_SIZE) {
+ threadLocal.get().push(info);
+ } else {
+ log.error("SolrRequestInfo Stack is full");
+ }
}
- assert prev == null;
-
- threadLocal.set(info);
}
+ /** Removes the most recent SolrRequestInfo from the stack */
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);
+ }
+ }
+
+ /** Removes all the SolrRequestInfos from the stack */
Review comment:
comment that we expect it to be empty by now because all "set" calls
need to be balanced with a "clear". Thus this reset method is more of a
protection mechanism.
##########
File path: solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java
##########
@@ -145,6 +145,8 @@
public static final String INTERNAL_REQUEST_COUNT = "_forwardedCount";
public static final Random random;
+
+ private boolean pushedSolrRequestInfo = false;
Review comment:
I know I suggested this name but lets call this
"mustClearSolrRequestInfo"
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
