SOLR-16692: Let users set the CookieStore implementation in Http2SolrClient #1445

tflobbe · 2023-03-09T23:12:17Z

No description provided.

tflobbe · 2023-03-09T23:17:59Z

solr/benchmark/src/java/org/apache/solr/bench/MiniClusterState.java

@@ -273,7 +273,7 @@ public void startMiniCluster(int nodeCount) {
        nodes.add(runner.getBaseUrl().toString());
      }

-      client = new Http2SolrClient.Builder().build();
+      client = new Http2SolrClient.Builder().useHttp1_1(true).build();


http2 would just break for me, not sure about other people running these benchmarks

any details on "break" means here? what did you run specifically to run the benchmark? I'll see if I can reproduce.

I didn't dive into it, but here is the exception:

org.apache.solr.client.solrj.SolrServerException: IOException occurred when talking to server at: null at org.apache.solr.client.solrj.impl.Http2SolrClient.request(Http2SolrClient.java:530) at org.apache.solr.bench.search.SimpleSearch.query(SimpleSearch.java:75) at org.apache.solr.bench.search.jmh_generated.SimpleSearch_query_jmhTest.query_thrpt_jmhStub(SimpleSearch_query_jmhTest.java:274) at org.apache.solr.bench.search.jmh_generated.SimpleSearch_query_jmhTest.query_Throughput(SimpleSearch_query_jmhTest.java:127) at jdk.internal.reflect.GeneratedMethodAccessor15.invoke(Unknown Source) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:566) at org.openjdk.jmh.runner.BenchmarkHandler$BenchmarkTask.call(BenchmarkHandler.java:475) at org.openjdk.jmh.runner.BenchmarkHandler$BenchmarkTask.call(BenchmarkHandler.java:458) at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515) at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) at java.base/java.lang.Thread.run(Thread.java:829) Caused by: java.io.IOException: cancel_stream_error at org.eclipse.jetty.http2.client.http.HttpReceiverOverHTTP2.onReset(HttpReceiverOverHTTP2.java:202) at org.eclipse.jetty.http2.client.http.HttpChannelOverHTTP2$Listener.onReset(HttpChannelOverHTTP2.java:206) at org.eclipse.jetty.http2.api.Stream$Listener.onReset(Stream.java:271) at org.eclipse.jetty.http2.HTTP2Stream.notifyReset(HTTP2Stream.java:853) at org.eclipse.jetty.http2.HTTP2Stream.onReset(HTTP2Stream.java:563) at org.eclipse.jetty.http2.HTTP2Stream.process(HTTP2Stream.java:392) at org.eclipse.jetty.http2.HTTP2Session.onReset(HTTP2Session.java:320) at org.eclipse.jetty.http2.parser.Parser$Listener$Wrapper.onReset(Parser.java:367) at org.eclipse.jetty.http2.parser.BodyParser.notifyReset(BodyParser.java:139) at org.eclipse.jetty.http2.parser.ResetBodyParser.onReset(ResetBodyParser.java:92) at org.eclipse.jetty.http2.parser.ResetBodyParser.parse(ResetBodyParser.java:61) at org.eclipse.jetty.http2.parser.Parser.parseBody(Parser.java:193) at org.eclipse.jetty.http2.parser.Parser.parse(Parser.java:122) at org.eclipse.jetty.http2.HTTP2Connection$HTTP2Producer.produce(HTTP2Connection.java:278) at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.produceTask(AdaptiveExecutionStrategy.java:450) at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:243) at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.produce(AdaptiveExecutionStrategy.java:194) at org.eclipse.jetty.http2.HTTP2Connection.produce(HTTP2Connection.java:208) at org.eclipse.jetty.http2.HTTP2Connection.onFillable(HTTP2Connection.java:155) at org.eclipse.jetty.http2.HTTP2Connection$FillableCallback.succeeded(HTTP2Connection.java:378) at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:100) at org.eclipse.jetty.io.SelectableChannelEndPoint$1.run(SelectableChannelEndPoint.java:53) at org.eclipse.jetty.util.thread.Invocable.invokeNonBlocking(Invocable.java:151) at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.invokeAsNonBlocking(AdaptiveExecutionStrategy.java:433) at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.consumeTask(AdaptiveExecutionStrategy.java:375) at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:272) at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.produce(AdaptiveExecutionStrategy.java:194) at org.apache.solr.common.util.ExecutorUtil$MDCAwareThreadPoolExecutor.lambda$execute$0(ExecutorUtil.java:289) ... 3 more

what did you run specifically to run the benchmark? I'll see if I can reproduce.

Nothing special, just: ./jmh.sh search.SimpleSearch

Other benchmarks also fail for this reason (main branch, didn't try others)

searching builds mailing list - I see two instances of cancel_stream_error also pop up so its not super common in normal tests. I'll see if this reproduces.

./jmh.sh search.JsonFaceting on main didn't reproduce for me. I'll checkout this PR and revert the use http 1 change.

so I pulled down your branch and am seeing the same exception you shared.

./jmh.sh -jvmArgsAppend '-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:5005' search.SimpleSearch

It looks like to me the exception is coming from HttpTransportOverHTTP2 line 375 - https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java#L375

I don't know why or how we get into this scenario but it seems like some sort of race condition or something since if you slow down the benchmark (ie: add a bunch of debug logging for Jetty) then things seem to be ok.

I noticed that the stream is remotely closed but not locally closed so isClosed() is false.

I can reproduce it in main using ./jmh.sh search.FilterCache if I bump the number of threads (I was running with 16)

Created https://issues.apache.org/jira/browse/SOLR-16702

risdenk · 2023-03-10T14:13:04Z

solr/benchmark/src/java/org/apache/solr/bench/MiniClusterState.java

@@ -273,7 +273,7 @@ public void startMiniCluster(int nodeCount) {
        nodes.add(runner.getBaseUrl().toString());
      }

-      client = new Http2SolrClient.Builder().build();
+      client = new Http2SolrClient.Builder().useHttp1_1(true).build();


any details on "break" means here? what did you run specifically to run the benchmark? I'll see if I can reproduce.

solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java

stillalex · 2023-03-14T20:01:59Z

the Http2SolrClient changes look good to me. the benchmark could move to it's own PR (similar to the discussion on #1447)

tflobbe · 2023-03-17T00:22:27Z

@risdenk I plan to add a CHANGES entry and merge soon, let me know if you have any concerns

This reverts commit c16cd06bf60625060e72254ddb3be9d9cb3dfed7.

…Client (#1445)

tflobbe commented Mar 9, 2023

View reviewed changes

risdenk reviewed Mar 10, 2023

View reviewed changes

tflobbe mentioned this pull request Mar 14, 2023

SOLR-16694: Shortcut Http2SolrClient mimeType check for default response parse #1447

Merged

tflobbe force-pushed the cookies branch from 91f3eb0 to 61ce3e3 Compare March 16, 2023 19:24

risdenk approved these changes Mar 17, 2023

View reviewed changes

tflobbe added 5 commits March 17, 2023 10:24

SOLR-16692: Allow setting a CookieStore in Http2SolrClient

cf15588

Add Benchmark results to .gitignore

c41bd67

Revert "Add Benchmark results to .gitignore"

300aacb

This reverts commit c16cd06bf60625060e72254ddb3be9d9cb3dfed7.

Add option to set the CookieStore in clients builder

45cc035

Add CHANGES entry

255f0ac

tflobbe force-pushed the cookies branch from 61ce3e3 to 255f0ac Compare March 17, 2023 17:32

risdenk approved these changes Mar 17, 2023

View reviewed changes

tflobbe changed the title ~~SOLR-16692: Use HttpCookieStore.Empty in Http2SolrClient~~ SOLR-16692: Let users set the CookieStore implementation in Http2SolrClient Mar 17, 2023

tflobbe merged commit cc35c1e into apache:main Mar 17, 2023
5 checks passed

tflobbe deleted the cookies branch March 17, 2023 18:13

tflobbe added a commit that referenced this pull request Mar 17, 2023

SOLR-16692: Let users set the CookieStore implementation in Http2Solr…

1ba732b

…Client (#1445)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-16692: Let users set the CookieStore implementation in Http2SolrClient #1445

SOLR-16692: Let users set the CookieStore implementation in Http2SolrClient #1445

tflobbe commented Mar 9, 2023

tflobbe Mar 9, 2023

risdenk Mar 10, 2023

tflobbe Mar 10, 2023

tflobbe Mar 10, 2023

risdenk Mar 13, 2023

risdenk Mar 13, 2023

risdenk Mar 13, 2023 •

edited

tflobbe Mar 13, 2023

tflobbe Mar 14, 2023

risdenk Mar 10, 2023

stillalex commented Mar 14, 2023

tflobbe commented Mar 17, 2023

SOLR-16692: Let users set the CookieStore implementation in Http2SolrClient #1445

SOLR-16692: Let users set the CookieStore implementation in Http2SolrClient #1445

Conversation

tflobbe commented Mar 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

risdenk Mar 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stillalex commented Mar 14, 2023

tflobbe commented Mar 17, 2023

risdenk Mar 13, 2023 •

edited