You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/05/18 20:35:33 UTC
[GitHub] [lucene-solr] NazerkeBS opened a new pull request #1527: SOLR-14384 Stack SolrRequestInfo
NazerkeBS opened a new pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Lucene or Solr:
* https://issues.apache.org/jira/projects/LUCENE
* https://issues.apache.org/jira/projects/SOLR
You will need to create an account in Jira in order to create an issue.
The title of the PR should reference the Jira issue number in the form:
* LUCENE-####: <short description of problem or changes>
* SOLR-####: <short description of problem or changes>
LUCENE and SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# Description
Please provide a short description of the changes you're making with this pull request.
# Solution
Please provide a short description of the approach taken to implement your solution.
# Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `master` branch.
- [ ] I have run `ant precommit` and the appropriate test suite.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Ref Guide](https://github.com/apache/lucene-solr/tree/master/solr/solr-ref-guide) (for Solr changes only).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#discussion_r434752288
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +56,60 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static SolrRequestInfo getRequestInfo() {
- return threadLocal.get();
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (stack.isEmpty()) return null;
+ return stack.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());
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (info == null) {
+ throw new IllegalArgumentException("SolrRequestInfo is null");
+ } else {
+ if (stack.size() <= MAX_STACK_SIZE) {
+ stack.push(info);
+ } else {
+ assert true : "SolrRequestInfo Stack is full";
Review comment:
assert false
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +56,60 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static SolrRequestInfo getRequestInfo() {
- return threadLocal.get();
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (stack.isEmpty()) return null;
+ return stack.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());
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (info == null) {
+ throw new IllegalArgumentException("SolrRequestInfo is null");
+ } else {
+ if (stack.size() <= MAX_STACK_SIZE) {
+ stack.push(info);
+ } else {
+ assert true : "SolrRequestInfo Stack is full";
+ 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);
- }
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (stack.isEmpty()) {
+ log.error("clearRequestInfo called too many times");
+ } else {
+ SolrRequestInfo info = stack.pop();
+ closeHooks(info);
+ }
+ }
+
+ /**
+ * This reset method is more of a protection mechanism as
+ * we expect it to be empty by now because all "set" calls need to be balanced with a "clear".
+ */
+ public static void reset() {
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ boolean isEmpty = stack.isEmpty();
+ while (!stack.isEmpty()) {
+ SolrRequestInfo info = stack.pop();
+ closeHooks(info);
+ }
+ assert isEmpty : "SolrRequestInfo Stack should have been cleared.";
+ }
+
+ private static void closeHooks(SolrRequestInfo info) {
+ if (info != null && info.closeHooks != null) {
Review comment:
but it cannot be null any more?
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +56,60 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static SolrRequestInfo getRequestInfo() {
- return threadLocal.get();
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (stack.isEmpty()) return null;
+ return stack.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());
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (info == null) {
+ throw new IllegalArgumentException("SolrRequestInfo is null");
+ } else {
+ if (stack.size() <= MAX_STACK_SIZE) {
+ stack.push(info);
+ } else {
+ assert true : "SolrRequestInfo Stack is full";
+ 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);
- }
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (stack.isEmpty()) {
+ log.error("clearRequestInfo called too many times");
Review comment:
add assertion
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] NazerkeBS commented on a change in pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#discussion_r435401394
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +56,60 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static SolrRequestInfo getRequestInfo() {
- return threadLocal.get();
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (stack.isEmpty()) return null;
+ return stack.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());
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (info == null) {
+ throw new IllegalArgumentException("SolrRequestInfo is null");
+ } else {
+ if (stack.size() <= MAX_STACK_SIZE) {
+ stack.push(info);
+ } else {
+ assert true : "SolrRequestInfo Stack is full";
+ 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);
- }
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (stack.isEmpty()) {
+ log.error("clearRequestInfo called too many times");
+ } else {
+ SolrRequestInfo info = stack.pop();
+ closeHooks(info);
+ }
+ }
+
+ /**
+ * This reset method is more of a protection mechanism as
+ * we expect it to be empty by now because all "set" calls need to be balanced with a "clear".
+ */
+ public static void reset() {
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ boolean isEmpty = stack.isEmpty();
+ while (!stack.isEmpty()) {
+ SolrRequestInfo info = stack.pop();
+ closeHooks(info);
+ }
+ assert isEmpty : "SolrRequestInfo Stack should have been cleared.";
+ }
+
+ private static void closeHooks(SolrRequestInfo info) {
+ if (info != null && info.closeHooks != null) {
Review comment:
it can be null; when getRequestInfo is called, in case of the stack.isEmpty(), it returns null;
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] NazerkeBS commented on a change in pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#discussion_r435544642
##########
File path: solr/core/src/java/org/apache/solr/client/solrj/embedded/EmbeddedSolrServer.java
##########
@@ -271,7 +273,9 @@ public void writeResults(ResultContext ctx, JavaBinCodec codec) throws IOExcepti
throw new SolrServerException(ex);
} finally {
if (req != null) req.close();
- SolrRequestInfo.clearRequestInfo();
+ if (mustClearSolrRequestInfo) {
Review comment:
actually that's enough
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#discussion_r427323443
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +54,44 @@
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 {
+ threadLocal.get().push(info);
Review comment:
Good idea!
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mkhludnev commented on a change in pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
mkhludnev commented on a change in pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#discussion_r427066314
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
##########
@@ -456,6 +457,7 @@ public void doFilter(ServletRequest _request, ServletResponse _response, FilterC
GlobalTracer.get().clearContext();
consumeInputFully(request, response);
+ SolrRequestInfo.reset();
Review comment:
there we need to check that stack have fully cleared and disposed
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] NazerkeBS commented on a change in pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#discussion_r429763566
##########
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:
Initially I set it to 100, then there was one test (CloudAuthStreamTest) was failing due to this capacity. So increased it to 150.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#discussion_r430017058
##########
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:
I suggest debugging/investigating there to understand _why_. Maybe this cap has exposed an actual problem?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] NazerkeBS commented on a change in pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#discussion_r427552166
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +54,44 @@
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 {
+ threadLocal.get().push(info);
Review comment:
what would you suggest for the stack size limit?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mkhludnev commented on a change in pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
mkhludnev commented on a change in pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#discussion_r427625556
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +54,44 @@
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 {
+ threadLocal.get().push(info);
Review comment:
100 for sure
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#issuecomment-633650898
Perhaps the reset method should furthermore have an assertion to check that the stack is already empty? If we decide that reset() is there as a safety measure that isn't to be used instead of clear(), then the assertion can alert us to problems (in tests) rather than hiding them.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#discussion_r435405488
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +56,60 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static SolrRequestInfo getRequestInfo() {
- return threadLocal.get();
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (stack.isEmpty()) return null;
+ return stack.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());
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (info == null) {
+ throw new IllegalArgumentException("SolrRequestInfo is null");
+ } else {
+ if (stack.size() <= MAX_STACK_SIZE) {
+ stack.push(info);
+ } else {
+ assert true : "SolrRequestInfo Stack is full";
+ 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);
- }
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (stack.isEmpty()) {
+ log.error("clearRequestInfo called too many times");
+ } else {
+ SolrRequestInfo info = stack.pop();
+ closeHooks(info);
+ }
+ }
+
+ /**
+ * This reset method is more of a protection mechanism as
+ * we expect it to be empty by now because all "set" calls need to be balanced with a "clear".
+ */
+ public static void reset() {
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ boolean isEmpty = stack.isEmpty();
+ while (!stack.isEmpty()) {
+ SolrRequestInfo info = stack.pop();
+ closeHooks(info);
+ }
+ assert isEmpty : "SolrRequestInfo Stack should have been cleared.";
+ }
+
+ private static void closeHooks(SolrRequestInfo info) {
+ if (info != null && info.closeHooks != null) {
Review comment:
The callers of this method will not pass null though; right?
##########
File path: solr/core/src/java/org/apache/solr/client/solrj/embedded/EmbeddedSolrServer.java
##########
@@ -271,7 +273,9 @@ public void writeResults(ResultContext ctx, JavaBinCodec codec) throws IOExcepti
throw new SolrServerException(ex);
} finally {
if (req != null) req.close();
- SolrRequestInfo.clearRequestInfo();
+ if (mustClearSolrRequestInfo) {
Review comment:
Hmm; might it simply be enough to call clearRequestInfo if req != null, thus no new boolean?
##########
File path: solr/core/src/java/org/apache/solr/client/solrj/embedded/EmbeddedSolrServer.java
##########
@@ -71,6 +71,7 @@
protected final String coreName;
private final SolrRequestParsers _parser;
private final RequestWriterSupplier supplier;
+ private boolean mustClearSolrRequestInfo = false;
Review comment:
This change is highly suspicious. Why is this boolean now a field of EmbeddedSolrServer? It doesn't seem like *state* of this object and so I don't think it belongs here. It seems like a transient status in the course of work within the method that sets/clears the requestInfo.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mkhludnev commented on a change in pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
mkhludnev commented on a change in pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#discussion_r427064514
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +54,44 @@
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 {
+ threadLocal.get().push(info);
Review comment:
Shouldn't we limit maximum stack depth?
What if one component leak SolrRequestInfo accidentally, like push it on every request but refuse to pop it when exception occur. Thus it may steadily flood heap. Or I miss something?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#discussion_r427323730
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
##########
@@ -456,6 +457,7 @@ public void doFilter(ServletRequest _request, ServletResponse _response, FilterC
GlobalTracer.get().clearContext();
consumeInputFully(request, response);
+ SolrRequestInfo.reset();
Review comment:
That is what `reset` _does_
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] NazerkeBS commented on a change in pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#discussion_r435401394
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +56,60 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static SolrRequestInfo getRequestInfo() {
- return threadLocal.get();
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (stack.isEmpty()) return null;
+ return stack.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());
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (info == null) {
+ throw new IllegalArgumentException("SolrRequestInfo is null");
+ } else {
+ if (stack.size() <= MAX_STACK_SIZE) {
+ stack.push(info);
+ } else {
+ assert true : "SolrRequestInfo Stack is full";
+ 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);
- }
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (stack.isEmpty()) {
+ log.error("clearRequestInfo called too many times");
+ } else {
+ SolrRequestInfo info = stack.pop();
+ closeHooks(info);
+ }
+ }
+
+ /**
+ * This reset method is more of a protection mechanism as
+ * we expect it to be empty by now because all "set" calls need to be balanced with a "clear".
+ */
+ public static void reset() {
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ boolean isEmpty = stack.isEmpty();
+ while (!stack.isEmpty()) {
+ SolrRequestInfo info = stack.pop();
+ closeHooks(info);
+ }
+ assert isEmpty : "SolrRequestInfo Stack should have been cleared.";
+ }
+
+ private static void closeHooks(SolrRequestInfo info) {
+ if (info != null && info.closeHooks != null) {
Review comment:
it can be null; when getRequestInfo is called, if the stack.isEmpty(), it returns null;
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley closed pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
dsmiley closed pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] NazerkeBS commented on a change in pull request #1527: SOLR-14384 Stack SolrRequestInfo
Posted by GitBox <gi...@apache.org>.
NazerkeBS commented on a change in pull request #1527:
URL: https://github.com/apache/lucene-solr/pull/1527#discussion_r435545076
##########
File path: solr/core/src/java/org/apache/solr/request/SolrRequestInfo.java
##########
@@ -52,35 +56,60 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static SolrRequestInfo getRequestInfo() {
- return threadLocal.get();
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (stack.isEmpty()) return null;
+ return stack.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());
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (info == null) {
+ throw new IllegalArgumentException("SolrRequestInfo is null");
+ } else {
+ if (stack.size() <= MAX_STACK_SIZE) {
+ stack.push(info);
+ } else {
+ assert true : "SolrRequestInfo Stack is full";
+ 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);
- }
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ if (stack.isEmpty()) {
+ log.error("clearRequestInfo called too many times");
+ } else {
+ SolrRequestInfo info = stack.pop();
+ closeHooks(info);
+ }
+ }
+
+ /**
+ * This reset method is more of a protection mechanism as
+ * we expect it to be empty by now because all "set" calls need to be balanced with a "clear".
+ */
+ public static void reset() {
+ Deque<SolrRequestInfo> stack = threadLocal.get();
+ boolean isEmpty = stack.isEmpty();
+ while (!stack.isEmpty()) {
+ SolrRequestInfo info = stack.pop();
+ closeHooks(info);
+ }
+ assert isEmpty : "SolrRequestInfo Stack should have been cleared.";
+ }
+
+ private static void closeHooks(SolrRequestInfo info) {
+ if (info != null && info.closeHooks != null) {
Review comment:
in fact, it's true; as it is a private method and called inside SolrRequestInfo
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org