You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "stillalex (via GitHub)" <gi...@apache.org> on 2023/03/01 03:55:17 UTC
[GitHub] [solr] stillalex opened a new pull request, #1410: SOLR-16676 Http2SolrClient loss of MDC context
stillalex opened a new pull request, #1410:
URL: https://github.com/apache/solr/pull/1410
https://issues.apache.org/jira/browse/SOLR-16676
<!--
_(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 Solr:
* https://issues.apache.org/jira/projects/SOLR
For something minor (i.e. that wouldn't be worth putting in release notes), you can skip JIRA.
To create a Jira issue, you will need to create an account there first.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
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
Adding a helper class to attach MDC context to the request and to clean it up once it finishes. there is a new instance of the helper created on each request to avoid sharing of context data.
# 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 `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #1410:
URL: https://github.com/apache/solr/pull/1410#issuecomment-1459013554
thank you @dsmiley, really appreciate your help on getting this merged.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #1410:
URL: https://github.com/apache/solr/pull/1410#issuecomment-1453570691
deadlock avoided by setting corePoolSzize to 3. thanks @risdenk! waiting for test results but PR should be open for review again.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1410:
URL: https://github.com/apache/solr/pull/1410#discussion_r1125812863
##########
solr/core/src/test/org/apache/solr/handler/TestHttpRequestId.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler;
+
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.SynchronousQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import org.apache.solr.SolrJettyTestBase;
+import org.apache.solr.client.solrj.impl.Http2SolrClient;
+import org.apache.solr.client.solrj.impl.HttpListenerFactory.RequestResponseListener;
+import org.apache.solr.client.solrj.request.SolrPing;
+import org.apache.solr.client.solrj.util.AsyncListener;
+import org.apache.solr.common.util.ExecutorUtil;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SolrNamedThreadFactory;
+import org.eclipse.jetty.client.api.Request;
+import org.eclipse.jetty.client.api.Result;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.slf4j.MDC;
+
+public class TestHttpRequestId extends SolrJettyTestBase {
Review Comment:
I'm confused. You are basically saying that the improvements here are un-observable? I wonder what motivated this dubious improvement then -- something custom you have?
FWIW I don't think it'd be awkward if you added a trace log. And FYI it's easy to configure a log level for just one test via the `LogLevel` annotation -- a wonderful thing.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley merged pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #1410:
URL: https://github.com/apache/solr/pull/1410
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1410:
URL: https://github.com/apache/solr/pull/1410#discussion_r1127371987
##########
solr/CHANGES.txt:
##########
@@ -236,6 +236,8 @@ Bug Fixes
* SOLR-16656: rid parameter missing from query logs (Alex Deparvu)
+* SOLR-16676: Http2SolrClient loss of MDC context (Alex Deparvu)
Review Comment:
I like succinctness but here it's unclear.
```suggestion
* SOLR-16676: Logs: Http2SolrClient.async() lacked MDC (Alex Deparvu)
```
If this helps distributed search (which uses this I think) then mention that. But I haven't noticed such a problem so maybe it doesn't?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1410:
URL: https://github.com/apache/solr/pull/1410#issuecomment-1449319907
For testing, see the LogListener utility and see how it's used. Perhaps you should modify TestRequestId to add an async test of the same using Http2... maybe that makes sense? Granted that test isn't at an Http level so instead maybe a new TestHttpRequestId subclassing SolrJettyTestBase.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1410:
URL: https://github.com/apache/solr/pull/1410#discussion_r1125194334
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -1273,4 +1287,33 @@ static SslContextFactory.Client getDefaultSslContextFactory() {
return sslContextFactory;
}
+
+ private static class MDCCopyHelper extends RequestResponseListener {
Review Comment:
it felt like a good representation of the lifecycle I needed to hook into, so I used it. in the end it turned out it's not stritly needed but I liked the idea of reusing an existing model instead of inventing a new set of names.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1410:
URL: https://github.com/apache/solr/pull/1410#discussion_r1125193069
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -469,6 +474,43 @@ public void testGetById() throws Exception {
}
}
+ @Test
+ public void testGetByIdArrayBlockingQueue() throws Exception {
+ DebugServlet.clear();
+
+ // client setup needs to be same as HttpShardHandlerFactory
+
Review Comment:
it's not strictly needed anymore, was only trying to reproduce the deadlock. I can remove it if it doesn't fit with the rest of the tests
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1410:
URL: https://github.com/apache/solr/pull/1410#discussion_r1127114676
##########
solr/core/src/test/org/apache/solr/handler/TestHttpRequestId.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler;
+
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.SynchronousQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import org.apache.solr.SolrJettyTestBase;
+import org.apache.solr.client.solrj.impl.Http2SolrClient;
+import org.apache.solr.client.solrj.impl.HttpListenerFactory.RequestResponseListener;
+import org.apache.solr.client.solrj.request.SolrPing;
+import org.apache.solr.client.solrj.util.AsyncListener;
+import org.apache.solr.common.util.ExecutorUtil;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SolrNamedThreadFactory;
+import org.eclipse.jetty.client.api.Request;
+import org.eclipse.jetty.client.api.Result;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.slf4j.MDC;
+
+public class TestHttpRequestId extends SolrJettyTestBase {
Review Comment:
> You are basically saying that the improvements here are un-observable? I wonder what motivated this dubious improvement then -- something custom you have?
there might be a small misunderstanding here. It's not am improvement I am chasing here, it's a bug, a regression in Solr 9 compared to Solr 8. and I don't have anything custom, this is inside the http2 client as it is used by the HttpShardHandler.
I am trying to setup LogLevel + LogListener in a SolrCloudTestCase, see if I can get something better looking.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1410:
URL: https://github.com/apache/solr/pull/1410#discussion_r1125813213
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -1273,4 +1287,33 @@ static SslContextFactory.Client getDefaultSslContextFactory() {
return sslContextFactory;
}
+
+ private static class MDCCopyHelper extends RequestResponseListener {
Review Comment:
Okay then a comment to this affect would be helpful.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #1410:
URL: https://github.com/apache/solr/pull/1410#issuecomment-1457520914
I think all feedback was covered
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1410:
URL: https://github.com/apache/solr/pull/1410#discussion_r1127202620
##########
solr/core/src/test/org/apache/solr/handler/TestHttpRequestId.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler;
+
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.SynchronousQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import org.apache.solr.SolrJettyTestBase;
+import org.apache.solr.client.solrj.impl.Http2SolrClient;
+import org.apache.solr.client.solrj.impl.HttpListenerFactory.RequestResponseListener;
+import org.apache.solr.client.solrj.request.SolrPing;
+import org.apache.solr.client.solrj.util.AsyncListener;
+import org.apache.solr.common.util.ExecutorUtil;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SolrNamedThreadFactory;
+import org.eclipse.jetty.client.api.Request;
+import org.eclipse.jetty.client.api.Result;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.slf4j.MDC;
+
+public class TestHttpRequestId extends SolrJettyTestBase {
Review Comment:
in the end it was SolrJettyTestBase not the SolrCloudTestCase as I mentioned before. please take a look at this version of the test: it uses logs to register context and verifies existence of MDC context.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1410:
URL: https://github.com/apache/solr/pull/1410#discussion_r1127202872
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -469,6 +474,43 @@ public void testGetById() throws Exception {
}
}
+ @Test
+ public void testGetByIdArrayBlockingQueue() throws Exception {
+ DebugServlet.clear();
+
+ // client setup needs to be same as HttpShardHandlerFactory
+
Review Comment:
ended up removing, to reduce the span of changes
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1410:
URL: https://github.com/apache/solr/pull/1410#discussion_r1127332650
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -447,21 +450,42 @@ public Cancellable asyncRequest(
@Override
public void onHeaders(Response response) {
super.onHeaders(response);
+ if (log.isDebugEnabled()) {
+ log.debug("response processing started");
+ }
InputStreamResponseListener listener = this;
+ MDCCopyHelper mdcCopyHelper = new MDCCopyHelper();
executor.execute(
() -> {
InputStream is = listener.getInputStream();
assert ObjectReleaseTracker.track(is);
try {
NamedList<Object> body =
processErrorsAndResponse(solrRequest, parser, response, is);
+ mdcCopyHelper.onBegin(null);
+ if (log.isDebugEnabled()) {
Review Comment:
I was actually following the 'local style' of the class
```
if (log.isDebugEnabled()) {
log.debug("Create Http2SolrClient with HTTP/1.1 transport");
}
```
but sure, I will remove shortly
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1410:
URL: https://github.com/apache/solr/pull/1410#discussion_r1125191417
##########
solr/core/src/test/org/apache/solr/handler/TestHttpRequestId.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler;
+
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.SynchronousQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import org.apache.solr.SolrJettyTestBase;
+import org.apache.solr.client.solrj.impl.Http2SolrClient;
+import org.apache.solr.client.solrj.impl.HttpListenerFactory.RequestResponseListener;
+import org.apache.solr.client.solrj.request.SolrPing;
+import org.apache.solr.client.solrj.util.AsyncListener;
+import org.apache.solr.common.util.ExecutorUtil;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SolrNamedThreadFactory;
+import org.eclipse.jetty.client.api.Request;
+import org.eclipse.jetty.client.api.Result;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.slf4j.MDC;
+
+public class TestHttpRequestId extends SolrJettyTestBase {
Review Comment:
I don't think your example does covers this scenario out of the box. this bug is about MDC being present across the threads running the request. there are no logs at this level, unless I add some specifically for this test, something trace level, but that feels awkward.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #1410:
URL: https://github.com/apache/solr/pull/1410#issuecomment-1452711234
trying the `ShardHandlerFactory#sizeOfQueue` setting on a Solr 9 definitely shows some weirdness, I'm unable to create a collection:
```
2023-03-02 15:15:40.028 INFO (qtp2048013503-26) [] o.a.s.s.HttpSolrCall [admin] webapp=null path=/admin/collections params={replicationFactor=1&collection.configName=_default&name=test&router.name=compositeId&action=CREATE&numShards=5&wt=json&_=1677798725094} status=500 QTime=180057
2023-03-02 15:15:40.029 ERROR (qtp2048013503-26) [] o.a.s.s.HttpSolrCall org.apache.solr.common.SolrException: create the collection time out:180s => org.apache.solr.common.SolrException: create the collection time out:180s
at org.apache.solr.handler.admin.CollectionsHandler.submitCollectionApiCommand(CollectionsHandler.java:433)
org.apache.solr.common.SolrException: create the collection time out:180s
at org.apache.solr.handler.admin.CollectionsHandler.submitCollectionApiCommand(CollectionsHandler.java:433) ~[?:?]
at org.apache.solr.handler.admin.CollectionsHandler.invokeAction(CollectionsHandler.java:345) ~[?:?]
at org.apache.solr.handler.admin.CollectionsHandler.handleRequestBody(CollectionsHandler.java:297) ~[?:?]
at org.apache.solr.handler.RequestHandlerBase.handleRequest(RequestHandlerBase.java:224) ~[?:?]
at org.apache.solr.servlet.HttpSolrCall.handleAdmin(HttpSolrCall.java:941) ~[?:?]
at org.apache.solr.servlet.HttpSolrCall.handleAdminRequest(HttpSolrCall.java:893) ~[?:?]
at org.apache.solr.servlet.HttpSolrCall.call(HttpSolrCall.java:584) ~[?:?]
at org.apache.solr.servlet.SolrDispatchFilter.dispatch(SolrDispatchFilter.java:250) ~[?:?]
```
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #1410:
URL: https://github.com/apache/solr/pull/1410#issuecomment-1452676506
@dsmiley good news and bad news :) I finally managed to add tests (and cover failure scenarios too) thanks for the pointers, I was tempted to not go the extra mile.
now the not so great news. I believe there might be a deadlock on the Http2SolrClient when configured with an ArrayBlockingQueue. I was not able to find tests for this config, but it's definitely possible to have this enabled via the HttpShardHandlerFactory configs. please take a look at https://github.com/apache/solr/pull/1410/commits/a9f9af819ed094a504c60b17da8607e66afc6a67 and possibly tell me what I did wrong.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1410:
URL: https://github.com/apache/solr/pull/1410#discussion_r1127330844
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -447,21 +450,42 @@ public Cancellable asyncRequest(
@Override
public void onHeaders(Response response) {
super.onHeaders(response);
+ if (log.isDebugEnabled()) {
+ log.debug("response processing started");
+ }
InputStreamResponseListener listener = this;
+ MDCCopyHelper mdcCopyHelper = new MDCCopyHelper();
executor.execute(
() -> {
InputStream is = listener.getInputStream();
assert ObjectReleaseTracker.track(is);
try {
NamedList<Object> body =
processErrorsAndResponse(solrRequest, parser, response, is);
+ mdcCopyHelper.onBegin(null);
+ if (log.isDebugEnabled()) {
Review Comment:
Can you please remove all the needless "log.isDebugEnabled" checks? It's needlessly verbose. I suspect you are doing this because you are seeing it in many places but we only do this when there are interpolated arguments `{}` with non-simply references to objects.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1410: SOLR-16676 Http2SolrClient loss of MDC context
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1410:
URL: https://github.com/apache/solr/pull/1410#discussion_r1124950966
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -1273,4 +1287,33 @@ static SslContextFactory.Client getDefaultSslContextFactory() {
return sslContextFactory;
}
+
+ private static class MDCCopyHelper extends RequestResponseListener {
Review Comment:
Why implement RequestResponseListener?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -469,6 +474,43 @@ public void testGetById() throws Exception {
}
}
+ @Test
+ public void testGetByIdArrayBlockingQueue() throws Exception {
+ DebugServlet.clear();
+
+ // client setup needs to be same as HttpShardHandlerFactory
+
Review Comment:
ah..... okay, so you are seeing an existing problem. I would have recommended modifying an existing distributed-search test. Your new test is fine though.
##########
solr/core/src/test/org/apache/solr/handler/TestHttpRequestId.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler;
+
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.SynchronousQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import org.apache.solr.SolrJettyTestBase;
+import org.apache.solr.client.solrj.impl.Http2SolrClient;
+import org.apache.solr.client.solrj.impl.HttpListenerFactory.RequestResponseListener;
+import org.apache.solr.client.solrj.request.SolrPing;
+import org.apache.solr.client.solrj.util.AsyncListener;
+import org.apache.solr.common.util.ExecutorUtil;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SolrNamedThreadFactory;
+import org.eclipse.jetty.client.api.Request;
+import org.eclipse.jetty.client.api.Result;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.slf4j.MDC;
+
+public class TestHttpRequestId extends SolrJettyTestBase {
Review Comment:
Your test approach is a bit invasive... uses the internals of Http2 client. It's okay but I was hoping you would have more of an integration test that like TestRequestId, which is why I referred you to that test. It does not depend on internal details of SolrClients etc. I love integration tests as it's more realistic, doesn't assume internals, and also makes it easier to change internals at some point. Since I now understand you observed specifically an issue with distributed search because of HttpShardHandler's use of that, maybe you could even use a simple query that uses that under the hood?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org