Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-05-08 Thread via GitHub


dsmiley merged PR #2276:
URL: https://github.com/apache/solr/pull/2276


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-05-08 Thread via GitHub


dsmiley commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-2101217845

   Unless I hear to the contrary, I'll commit this tonight like midnight EST.  
Just "main" for a bit.
   
   
   The CHANGES.txt is currently:
   >  SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty 
HTTP2
   But want to reword do
   >  SOLR-16505: Use Jetty HTTP2 for index replication and other "recovery" 
operations.
   
   That will be more meaningful to users.  CHANGES.txt needs to speak to users.


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-05-08 Thread via GitHub


dsmiley commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-2100741552

   The most important part of Http2SolrClient to be re-used (instead of 
re-created) is Jetty's HttpClient.  Creating another Http2SolrClient that uses 
an existing HttpClient isn't too bad but yeah it'd be nice if we didn't have to 
re-create one.  This PR is an improvement!  IndexFetcher.getLatestVersion, 
fetchFileList, getStream, and getDetails not only re-use the underlying 
HttpClient, as before, but now use the SolrClient wrapper, which is more 
succinct & simple in so doing.
   
   Some thoughts on the commit message summarizing this long one:
   
   - UpdateShardHandler
 - switch "recoveryOnlyHttpClient" to Http2SolrClient
   - RecoveryStrategy:
 - Use Http2SolrClient
 - Simplify cancelation of prep recovery command
   - IndexFetcher:
 - Use Http2SolrClient
 - Ensure the entire stream is consumed to avoid a connection reset 
   - Http2SolrClient:
 - make HttpListenerFactory configurable (used for internal purposes) 
   
   It's unclear if there was any change with respect to gzip / 
"useExternalCompression" -- this part of the diff is confusing.


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-05-08 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1594018250


##
solr/core/src/test-files/solr/collection1/conf/solrconfig-follower-auth.xml:
##


Review Comment:
   I won't push this point further because your ...WithAuth test uses 
`ReplicationTestHelper.SolrInstance` which has sone conveniences and I'm not 
sure how compatible it is with my suggestion.



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-05-07 Thread via GitHub


iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-2099713438

   Thinking out loud! The new logic, however bit ugly it is because it gives 
freedom to manipulate the Http2SolrClient directly instead of creating it 
through Builder patter -- _anti-pattern_, to copy the listeners from older 
Http2SolrClient to the new Http2SolrClient is added only because of the custom 
timeouts configuration for replication logic. 
   
   ### Is there any way to avoid recreating the Http2SolrClient?
   
   The default socketTimeout and connectionTimeout for UpdateShardHandler http 
client is 60 secs for both, and for IndexFetcher Recovery client It is 120 and 
60 seconds respectively. Can we deprecate or basically remove the older way of 
setting these timeouts at replication Handler level and use the 
UpdateShardHandler config timeouts i.e. 60 seconds for both socket and 
connection? 


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-05-07 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1593008731


##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##
@@ -115,6 +115,11 @@ public class Http2SolrClient extends HttpSolrClientBase {
   private SSLConfig sslConfig;
 
   private List listenerFactory = new ArrayList<>();
+
+  public List getListenerFactory() {

Review Comment:
   Minor, but this getter is very misplaced.  It shouldn't be placed amidst the 
fields.



##
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##
@@ -587,5 +587,27 @@ public void testBuilder() {
 }
   }
 
+  @Test
+  public void testIdleTimeoutWithHttpClient() {

Review Comment:
   nice!



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-05-07 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1592994488


##
solr/core/src/test/org/apache/solr/handler/TestUserManagedReplicationWithAuth.java:
##
@@ -0,0 +1,267 @@
+/*
+ * 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 static org.apache.solr.common.params.CommonParams.JAVABIN;
+import static org.apache.solr.handler.ReplicationHandler.CMD_DISABLE_POLL;
+import static org.apache.solr.handler.ReplicationHandler.CMD_FETCH_INDEX;
+import static org.apache.solr.handler.ReplicationHandler.COMMAND;
+import static 
org.apache.solr.handler.ReplicationTestHelper.createAndStartJetty;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.SolrTestCaseJ4.SuppressSSL;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrQuery;
+import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.SolrResponse;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.request.HealthCheckRequest;
+import org.apache.solr.client.solrj.request.QueryRequest;
+import org.apache.solr.client.solrj.request.UpdateRequest;
+import org.apache.solr.client.solrj.response.QueryResponse;
+import org.apache.solr.common.SolrInputDocument;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+@SuppressSSL
+public class TestUserManagedReplicationWithAuth extends SolrTestCaseJ4 {
+  JettySolrRunner leaderJetty, followerJetty, followerJettyWithAuth;
+  SolrClient leaderClient, followerClient, followerClientWithAuth;
+  ReplicationTestHelper.SolrInstance leader = null, follower = null, 
followerWithAuth = null;
+
+  private static String user = "solr";
+  private static String pass = "SolrRocks";
+  private static String securityJson =
+  "{\n"
+  + "\"authentication\":{ \n"
+  + "   \"blockUnknown\": true, \n"
+  + "   \"class\":\"solr.BasicAuthPlugin\",\n"
+  + "   
\"credentials\":{\"solr\":\"IV0EHq1OnNrj6gvRCwvFwTrZ1+z1oBbnQdiVC3otuq0= 
Ndd7LKvVBAaZIF0QAVi1ekCfAJXr1GGfLtRUXhgrF8c=\"}, \n"
+  + "   \"realm\":\"My Solr users\", \n"
+  + "   \"forwardCredentials\": false \n"
+  + "},\n"
+  + "\"authorization\":{\n"
+  + "   \"class\":\"solr.RuleBasedAuthorizationPlugin\",\n"
+  + "   \"permissions\":[{\"name\":\"security-edit\",\n"
+  + "  \"role\":\"admin\"}],\n"
+  + "   \"user-role\":{\"solr\":\"admin\"}\n"
+  + "}}";
+
+  @Override
+  @Before
+  public void setUp() throws Exception {
+super.setUp();
+systemSetPropertySolrDisableUrlAllowList("true");
+// leader with Basic auth enabled via security.json
+leader =
+new ReplicationTestHelper.SolrInstance(
+createTempDir("solr-instance").toFile(), "leader", null);
+leader.setUp();
+// Configuring basic auth for Leader
+Path solrLeaderHome = Path.of(leader.getHomeDir());
+Files.write(
+solrLeaderHome.resolve("security.json"), 
securityJson.getBytes(StandardCharsets.UTF_8));
+leaderJetty = ReplicationTestHelper.createAndStartJetty(leader);
+leaderClient =
+ReplicationTestHelper.createNewSolrClient(
+buildUrl(leaderJetty.getLocalPort()), DEFAULT_TEST_CORENAME);
+
+// follower with no basic auth credentials for leader configured.
+follower =
+new ReplicationTestHelper.SolrInstance(
+createTempDir("solr-instance").toFile(), "follower", 
leaderJetty.getLocalPort());
+follower.setUp();
+followerJetty = createAndStartJetty(follower);
+followerClient =
+ReplicationTestHelper.createNewSolrClient(
+buildUrl(followerJetty.getLocalPort()), DEFAULT_TEST_CORENAME);
+
+// follower with basic auth credentials for leader configured in 
solrconfig.xml.
+followerWithAuth =
+   

Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-05-07 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1592989945


##
solr/core/src/test/org/apache/solr/handler/TestUserManagedReplicationWithAuth.java:
##
@@ -0,0 +1,267 @@
+/*
+ * 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 static org.apache.solr.common.params.CommonParams.JAVABIN;
+import static org.apache.solr.handler.ReplicationHandler.CMD_DISABLE_POLL;
+import static org.apache.solr.handler.ReplicationHandler.CMD_FETCH_INDEX;
+import static org.apache.solr.handler.ReplicationHandler.COMMAND;
+import static 
org.apache.solr.handler.ReplicationTestHelper.createAndStartJetty;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.SolrTestCaseJ4.SuppressSSL;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrQuery;
+import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.SolrResponse;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.request.HealthCheckRequest;
+import org.apache.solr.client.solrj.request.QueryRequest;
+import org.apache.solr.client.solrj.request.UpdateRequest;
+import org.apache.solr.client.solrj.response.QueryResponse;
+import org.apache.solr.common.SolrInputDocument;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+@SuppressSSL
+public class TestUserManagedReplicationWithAuth extends SolrTestCaseJ4 {
+  JettySolrRunner leaderJetty, followerJetty, followerJettyWithAuth;
+  SolrClient leaderClient, followerClient, followerClientWithAuth;
+  ReplicationTestHelper.SolrInstance leader = null, follower = null, 
followerWithAuth = null;
+
+  private static String user = "solr";
+  private static String pass = "SolrRocks";
+  private static String securityJson =
+  "{\n"
+  + "\"authentication\":{ \n"
+  + "   \"blockUnknown\": true, \n"
+  + "   \"class\":\"solr.BasicAuthPlugin\",\n"
+  + "   
\"credentials\":{\"solr\":\"IV0EHq1OnNrj6gvRCwvFwTrZ1+z1oBbnQdiVC3otuq0= 
Ndd7LKvVBAaZIF0QAVi1ekCfAJXr1GGfLtRUXhgrF8c=\"}, \n"
+  + "   \"realm\":\"My Solr users\", \n"
+  + "   \"forwardCredentials\": false \n"
+  + "},\n"
+  + "\"authorization\":{\n"
+  + "   \"class\":\"solr.RuleBasedAuthorizationPlugin\",\n"
+  + "   \"permissions\":[{\"name\":\"security-edit\",\n"
+  + "  \"role\":\"admin\"}],\n"
+  + "   \"user-role\":{\"solr\":\"admin\"}\n"
+  + "}}";
+
+  @Override
+  @Before
+  public void setUp() throws Exception {
+super.setUp();
+systemSetPropertySolrDisableUrlAllowList("true");
+// leader with Basic auth enabled via security.json
+leader =
+new ReplicationTestHelper.SolrInstance(
+createTempDir("solr-instance").toFile(), "leader", null);
+leader.setUp();
+// Configuring basic auth for Leader
+Path solrLeaderHome = Path.of(leader.getHomeDir());
+Files.write(
+solrLeaderHome.resolve("security.json"), 
securityJson.getBytes(StandardCharsets.UTF_8));
+leaderJetty = ReplicationTestHelper.createAndStartJetty(leader);
+leaderClient =
+ReplicationTestHelper.createNewSolrClient(
+buildUrl(leaderJetty.getLocalPort()), DEFAULT_TEST_CORENAME);
+
+// follower with no basic auth credentials for leader configured.
+follower =
+new ReplicationTestHelper.SolrInstance(
+createTempDir("solr-instance").toFile(), "follower", 
leaderJetty.getLocalPort());
+follower.setUp();
+followerJetty = createAndStartJetty(follower);
+followerClient =
+ReplicationTestHelper.createNewSolrClient(
+buildUrl(followerJetty.getLocalPort()), DEFAULT_TEST_CORENAME);
+
+// follower with basic auth credentials for leader configured in 
solrconfig.xml.
+followerWithAuth =
+   

Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-05-07 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1592980298


##
solr/core/src/java/org/apache/solr/update/UpdateShardHandler.java:
##
@@ -133,16 +135,25 @@ public UpdateShardHandler(UpdateShardHandlerConfig cfg) {
 DistributedUpdateProcessor.DISTRIB_FROM,
 DistributingUpdateProcessorFactory.DISTRIB_UPDATE_PARAM);
 Http2SolrClient.Builder updateOnlyClientBuilder = new 
Http2SolrClient.Builder();
+Http2SolrClient.Builder recoveryOnlyClientBuilder = new 
Http2SolrClient.Builder();

Review Comment:
   @markrmiller do you recall why there are separate clients for "updateOnly" 
vs "recoveryOnly"?



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-05-01 Thread via GitHub


dsmiley commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-2089008592

   I looked at the [test 
history](https://ge.apache.org/scans/tests?search.relativeStartTime=P90D=solr-root=America%2FNew_York=org.apache.solr.search.TestCoordinatorRole)
 -- seems flaky.  I found [a JIRA 
issue](https://issues.apache.org/jira/browse/SOLR-16890) where you can relay 
these comments.
   
   Looking forward to the lingering small matters being addressed so we can get 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-05-01 Thread via GitHub


iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-2088106960

   Build failed due to one of the flaky test cases as test passing when ran 
again.
   
   > gradlew :solr:core:test --tests 
"org.apache.solr.search.TestCoordinatorRole.testNRTRestart" -Ptests.jvms=96 
"-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC 
-XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" 
-Ptests.seed=383FA9310A6D5B37 -Ptests.timeoutSuite=60! 
-Ptests.file.encoding=US-ASCII
   
   The error message that we are getting
   
   > initial error time threshold exceeded
   
   
https://github.com/apache/solr/blob/033be0fc1317b48992fb756aac7f8bd55731fc3b/solr/core/src/test/org/apache/solr/search/TestCoordinatorRole.java#L462-L483
   
   I will try to see If i can get more details on this. Should I open a Jira 
ticket for this? 
   
   
   


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-04-30 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1585244557


##
solr/core/src/test-files/solr/collection1/conf/solrconfig-follower-auth.xml:
##


Review Comment:
   This isn't used; right?



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-04-24 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1578527066


##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -1878,7 +1870,6 @@ private int fetchPackets(FastInputStream fis) throws 
Exception {
   log.debug("Fetched and wrote {} bytes of file: {}", bytesDownloaded, 
fileName);
   // errorCount is always set to zero after a successful packet
   errorCount = 0;
-  if (bytesDownloaded >= size) return 0;

Review Comment:
   That issue merely moved this code slightly; it didn't introduce it.  Anyway, 
I suppose this code potentially prematurely exited before reading the whole 
stream, and that's why you removed it?



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-04-22 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1575500892


##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##
@@ -845,6 +852,13 @@ public static class Builder
 
 protected Long keyStoreReloadIntervalSecs;
 
+public Http2SolrClient.Builder 
withListenerFactory(List listenerFactory) {

Review Comment:
   @epugh I know you worked on SolrClient immutability efforts.  Here, specific 
to Http2SolrClient, the "listenerFactory" list can now be set on the builder.  
The setter should probably also be marked Deprecated.  Do you think this should 
be separated to another PR?



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-04-22 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1575042664


##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -1833,18 +1819,24 @@ private int fetchPackets(FastInputStream fis) throws 
Exception {
   byte[] longbytes = new byte[8];
   try {
 while (true) {
+  if (fis.peek() == -1) {
+if (bytesDownloaded == 0) {
+  log.warn("No content received for file: {}", fileName);
+}
+return NO_CONTENT;
+  }
   if (stop) {
 stop = false;
 aborted = true;
 throw new ReplicationHandlerException("User aborted replication");
   }
   long checkSumServer = -1;
+
   fis.readFully(intbytes);
   // read the size of the packet
   int packetSize = readInt(intbytes);
   if (packetSize <= 0) {
-log.warn("No content received for file: {}", fileName);
-return NO_CONTENT;

Review Comment:
   at least an assert (or comment) communicating this is unexpected



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-04-22 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1575036574


##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -1878,7 +1870,6 @@ private int fetchPackets(FastInputStream fis) throws 
Exception {
   log.debug("Fetched and wrote {} bytes of file: {}", bytesDownloaded, 
fileName);
   // errorCount is always set to zero after a successful packet
   errorCount = 0;
-  if (bytesDownloaded >= size) return 0;

Review Comment:
   I did a git blame and found: SOLR-14299 IndexFetcher doesn't reset count to 
0 after the last packet is received



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-04-22 Thread via GitHub


iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-2070093200

   Below test failed!
   ```
   ./gradlew :solr:core:test --tests 
"org.apache.solr.cloud.OverseerStatusTest.test" -Ptests.jvms=96 
"-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC 
-XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" 
-Ptests.seed=5C4EADB21CB7FBF1 -Ptests.timeoutSuite=60! 
-Ptests.file.encoding=US-ASCII
   ```
   
   **Exception**
   
   ```
java.lang.NullPointerException
  > at 
__randomizedtesting.SeedInfo.seed([5C4EADB21CB7FBF1:D41A9268B24B9609]:0)
  > at 
org.apache.solr.cloud.OverseerStatusTest.test(OverseerStatusTest.java:78)
  > at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
  > at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
  > at 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
  > at java.base/java.lang.reflect.Method.invoke(Method.java:566)
  > at 
com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1758)
  > at 
com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:946)
  > at 
com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:982)
  > at 
com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:996)
  > at 
com.carrotsearch.randomizedtesting.rules.SystemPropertiesRestoreRule$1.evaluate(SystemPropertiesRestoreRule.java:80)
  > at org.junit.rules.RunRules.evaluate(RunRules.java:20)
  > at 
org.apache.lucene.tests.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:48)
  > at 
org.apache.lucene.tests.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
  > at 
org.apache.lucene.tests.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:45)
  > at 
org.apache.lucene.tests.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:60)
  > at 
org.apache.lucene.tests.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:44)
  > at org.junit.rules.RunRules.evaluate(RunRules.java:20)
   ```
   
   git bisect [61a67c0](https://github.com/apache/solr/pull/2230)
   
   CC @noblepaul 


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-04-21 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1573866976


##
solr/core/src/java/org/apache/solr/update/UpdateShardHandler.java:
##
@@ -133,16 +135,25 @@ public UpdateShardHandler(UpdateShardHandlerConfig cfg) {
 DistributedUpdateProcessor.DISTRIB_FROM,
 DistributingUpdateProcessorFactory.DISTRIB_UPDATE_PARAM);
 Http2SolrClient.Builder updateOnlyClientBuilder = new 
Http2SolrClient.Builder();
+Http2SolrClient.Builder recoveryOnlyClientBuilder = new 
Http2SolrClient.Builder();

Review Comment:
   CC @markrmiller 



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-04-21 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1573856139


##
solr/core/src/test-files/solr/collection1/conf/solrconfig-follower-auth.xml:
##


Review Comment:
   I don't see a reference to this file from the test; am I missing something?
   
   If this is actually used, then please consider instead modifying your test 
to manipulate the temp config files so that we don't have yet another test 
config file to maintain.  For an example of this technique, see 
org.apache.solr.search.TestCpuAllowedLimit#createConfigSet



##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -1867,7 +1866,6 @@ private int fetchPackets(FastInputStream fis) throws 
Exception {
   log.debug("Fetched and wrote {} bytes of file: {}", bytesDownloaded, 
fileName);
   // errorCount is always set to zero after a successful packet
   errorCount = 0;
-  if (bytesDownloaded >= size) return 0;

Review Comment:
   FWIW it's a bizarre check to me so I'm glad you are removing it.



##
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##
@@ -867,6 +869,8 @@ public static class Builder
 
 protected Long keyStoreReloadIntervalSecs;
 
+private List listenerFactory;

Review Comment:
   There's no setter for this.  Which is suspicious... like do we even need 
this in this PR?  If you add a setter for it, you can use the setter in 
UpdateHandler.



##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -1831,21 +1814,27 @@ private void fetch() throws Exception {
 private int fetchPackets(FastInputStream fis) throws Exception {
   byte[] intbytes = new byte[4];
   byte[] longbytes = new byte[8];
+  boolean isContentReceived = false;

Review Comment:
   We probably don't need this variable anymore; can rely on bytesDownloaded > 
0 condition.



##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -1831,21 +1814,27 @@ private void fetch() throws Exception {
 private int fetchPackets(FastInputStream fis) throws Exception {
   byte[] intbytes = new byte[4];
   byte[] longbytes = new byte[8];
+  boolean isContentReceived = false;
   try {
 while (true) {
+  if (fis.peek() == -1) {
+return NO_CONTENT;

Review Comment:
   Shouldn't we add this log here as seenbelow: if 
(!isContentReceived) log.warn("No content received for file: {}", fileName);
   



##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -1831,21 +1814,27 @@ private void fetch() throws Exception {
 private int fetchPackets(FastInputStream fis) throws Exception {
   byte[] intbytes = new byte[4];
   byte[] longbytes = new byte[8];
+  boolean isContentReceived = false;
   try {
 while (true) {
+  if (fis.peek() == -1) {
+return NO_CONTENT;
+  }
   if (stop) {
 stop = false;
 aborted = true;
 throw new ReplicationHandlerException("User aborted replication");
   }
   long checkSumServer = -1;
+
   fis.readFully(intbytes);
   // read the size of the packet
   int packetSize = readInt(intbytes);
   if (packetSize <= 0) {
-log.warn("No content received for file: {}", fileName);
-return NO_CONTENT;
+if (!isContentReceived) log.warn("No content received for file: 
{}", fileName);

Review Comment:
   Since we don't return here, this probably isn't where we should log this 
message.
   Nonetheless if we get to this spot, it's now highly unexpected since even if 
there was no content, the peek==-1 above would happen and we wouldn't get here. 
So what do we do?  Hmm.  If we think it's impossible (I think?), add an assert. 
 Could also log a warning.



##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -261,22 +260,23 @@ public String getMessage() {
 }
   }
 
-  private static HttpClient createHttpClient(
-  SolrCore core,
-  String httpBasicAuthUser,
-  String httpBasicAuthPassword,
-  boolean useCompression) {
+  private Http2SolrClient createHttpClient(
+  SolrCore core, String httpBasicAuthUser, String httpBasicAuthPassword, 
String leaderBaseUrl) {
 final ModifiableSolrParams httpClientParams = new ModifiableSolrParams();

Review Comment:
   Do you see my point that `httpClientParams` is now unused?  At a minimum, it 
needs to be removed now.
   
   With your insights on httpBasicAuthUser and httpBasicAuthPassword 
configuration, a comment explaining *why* we can't simply assume the PKI one 
would be helpful to whoever reads this in the future.



-- 
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 

Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-04-16 Thread via GitHub


iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-2058715129

   ### Can we remove the Legacy Auth mechanism? 
   
   No
   
   In User managed cluster, the only communication, IMO, happening is 
downloading of index from one node to another. The `PKIAuthenticationPlugin` 
only works in cloud environment and the condition that enclosed the PKI 
initialization logic checks whether it `isZookeeperAware()`.  Of course! that 
condition evaluates to false in UserManaged cluster and therefore the method 
which is responsible of setting auth headers never gets called.
   
   
https://github.com/apache/solr/blob/c3c83ffb8dba17dd79f78429df65869d1b7d87bb/solr/core/src/java/org/apache/solr/core/CoreContainer.java#L836-L855
   
   
https://github.com/apache/solr/blob/c3c83ffb8dba17dd79f78429df65869d1b7d87bb/solr/core/src/java/org/apache/solr/core/CoreContainer.java#L626-L631
   
   Will be adding a test case to test Legacy replication when Basic auth is 
enabled


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-04-13 Thread via GitHub


dsmiley commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-2053755426

   If there's anything I can do to help here, let me know.


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-31 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1545945449


##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -261,22 +260,23 @@ public String getMessage() {
 }
   }
 
-  private static HttpClient createHttpClient(
-  SolrCore core,
-  String httpBasicAuthUser,
-  String httpBasicAuthPassword,
-  boolean useCompression) {
+  private Http2SolrClient createHttpClient(
+  SolrCore core, String httpBasicAuthUser, String httpBasicAuthPassword, 
String leaderBaseUrl) {
 final ModifiableSolrParams httpClientParams = new ModifiableSolrParams();

Review Comment:
   (this is still pending action)



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-30 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1545301666


##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -1828,13 +1823,17 @@ private int fetchPackets(FastInputStream fis) throws 
Exception {
 throw new ReplicationHandlerException("User aborted replication");
   }
   long checkSumServer = -1;
+
   fis.readFully(intbytes);
+
   // read the size of the packet
   int packetSize = readInt(intbytes);
   if (packetSize <= 0) {
-log.warn("No content received for file: {}", fileName);
+fis.read(); // read till end-of-file

Review Comment:
   See `org.apache.solr.common.util.Utils#readFully` instead



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-30 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1545293836


##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -1828,13 +1823,17 @@ private int fetchPackets(FastInputStream fis) throws 
Exception {
 throw new ReplicationHandlerException("User aborted replication");
   }
   long checkSumServer = -1;
+
   fis.readFully(intbytes);
+
   // read the size of the packet
   int packetSize = readInt(intbytes);
   if (packetSize <= 0) {
-log.warn("No content received for file: {}", fileName);
+fis.read(); // read till end-of-file

Review Comment:
   This line does not do what the comment says.  Regardless, shouldn't we 
*already* be at EOF?



##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -1984,7 +1982,7 @@ private FastInputStream getStream() throws IOException {
 QueryRequest req = new QueryRequest(params);
 req.setResponseParser(new InputStreamResponseParser(FILE_STREAM));
 req.setBasePath(leaderBaseUrl);
-if (useExternalCompression) req.addHeader("Accept-Encoding", "gzip, 
deflate");
+if (useExternalCompression) req.addHeader("Accept-Encoding", "gzip");

Review Comment:
   why?



##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -1867,7 +1866,6 @@ private int fetchPackets(FastInputStream fis) throws 
Exception {
   log.debug("Fetched and wrote {} bytes of file: {}", bytesDownloaded, 
fileName);
   // errorCount is always set to zero after a successful packet
   errorCount = 0;
-  if (bytesDownloaded >= size) return 0;

Review Comment:
   why this change?



##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -2111,7 +2109,6 @@ NamedList getDetails() throws IOException, 
SolrServerException {
 
 QueryRequest request = new QueryRequest(params);
 request.setBasePath(leaderBaseUrl);
-if (useExternalCompression) request.addHeader("Accept-Encoding", "gzip, 
deflate");

Review Comment:
   why?  I suspect temporary for your debugging



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-30 Thread via GitHub


iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-2027942595

   Fix for RST_STREAM! 
   
   Upon enabling logs at "org.eclipse.jetty" level. It was found that 
IndexFetcher was closing the InputStream prematurely, causing the client to 
send RST_FRAME and flooding the server with RESET Frame and eventually hitting 
the ceiling.
   
   The solution involved modifying the IndexFetcher to continue reading from 
the InputStream until it receives a zero-length packet before closing the 
stream.
   
   PS :- Our friends at Jetty helped. [Thank you 
@sbordet!](https://github.com/jetty/jetty.project/issues/11595#issuecomment-2027522789)


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-28 Thread via GitHub


iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-2025117803

   Used **LogLevel** annotation to generate the DEBUG logs from Jetty. However, 
the excessive logging reduce the likelihood of reproducing the failure. So I 
restrict the logging to one class.
   ```
   @SuppressSSL // Currently, unknown why SSL does not work with this test
   @LogLevel("org.eclipse.jetty.http2.HTTP2Connection=DEBUG")
   public class TestHealthCheckHandlerLegacyMode extends SolrTestCaseJ4 {
   ```
   
   Below is the new exception observed in the logs related to terminating the 
connection.
   
   > DEBUG (qtp803109855-19) [n: c: s: r: x: t:] o.e.j.h.HTTP2Connection 
Processing session failure on 
HTTP2ServerSession@1674feca{local:/127.0.0.1:50713,remote:/127.0.0.1:50719,sendWindow=938358,recvWindow=1048576,state=[streams=0,CLOSING,goAwayRecv=null,goAwaySent=GoAwayFrame@ca473bc{847/enhance_your_calm_error/invalid_rst_stream_frame_rate},failure=java.io.IOException:
 enhance_your_calm_error/invalid_rst_stream_frame_rate]}
   >   2>   => java.io.IOException: 
enhance_your_calm_error/invalid_rst_stream_frame_rate
   >   2>   at 
org.eclipse.jetty.http2.HTTP2Session.toFailure(HTTP2Session.java:633)
   >   2> java.io.IOException: 
enhance_your_calm_error/invalid_rst_stream_frame_rate
   >   2>   at 
org.eclipse.jetty.http2.HTTP2Session.toFailure(HTTP2Session.java:633) 
[http2-common-10.0.20.jar:10.0.20]
   >   2>   at 
org.eclipse.jetty.http2.HTTP2Session$StreamsState.onSessionFailure(HTTP2Session.java:2006)
 [http2-common-10.0.20.jar:10.0.20]
   >   2>   at 
org.eclipse.jetty.http2.HTTP2Session.onSessionFailure(HTTP2Session.java:578) 
[http2-common-10.0.20.jar:10.0.20]
   >   2>   at 
org.eclipse.jetty.http2.HTTP2Session.onConnectionFailure(HTTP2Session.java:573) 
[http2-common-10.0.20.jar:10.0.20]
   >   2>   at 
org.eclipse.jetty.http2.HTTP2Connection.onConnectionFailure(HTTP2Connection.java:303)
 [http2-common-10.0.20.jar:10.0.20]
   >   2>   at 
org.eclipse.jetty.http2.parser.BodyParser.notifyConnectionFailure(BodyParser.java:218)
 [http2-common-10.0.20.jar:10.0.20]
   >   2>   at 
org.eclipse.jetty.http2.parser.BodyParser.connectionFailure(BodyParser.java:210)
 [http2-common-10.0.20.jar:10.0.20]
   >   2>   **at 
org.eclipse.jetty.http2.parser.ResetBodyParser.onReset(ResetBodyParser.java:92) 
[http2-common-10.0.20.jar:10.0.20]**
   >   2>   at 
org.eclipse.jetty.http2.parser.ResetBodyParser.parse(ResetBodyParser.java:61) 
[http2-common-10.0.20.jar:10.0.20]
   >   2>   at 
org.eclipse.jetty.http2.parser.Parser.parseBody(Parser.java:240) 
[http2-common-10.0.20.jar:10.0.20]
   >   2>   at org.eclipse.jetty.http2.parser.Parser.parse(Parser.java:167) 
[http2-common-10.0.20.jar:10.0.20]
   >   2>   at 
org.eclipse.jetty.http2.parser.ServerParser.parse(ServerParser.java:126) 
[http2-common-10.0.20.jar:10.0.20]
   >   2>   at 
org.eclipse.jetty.http2.HTTP2Connection$HTTP2Producer.produce(HTTP2Connection.java:350)
 [http2-common-10.0.20.jar:10.0.20]
   >   2>   at 
org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.produceTask(AdaptiveExecutionStrategy.java:455)
 [jetty-util-10.0.20.jar:10.0.20]
   >   2>   at 
org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:248)
 [jetty-util-10.0.20.jar:10.0.20]
   >   2>   at 
org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.produce(AdaptiveExecutionStrategy.java:193)
 [jetty-util-10.0.20.jar:10.0.20]
   
   ## Error
   
   org.eclipse.jetty.io.EofException: Close enhance_your_calm_error/ 
(invalid_rst_stream_frame_rate) 
   
   As per RFC https://datatracker.ietf.org/doc/html/rfc9113#name-error-codes
   
   **ENHANCE_YOUR_CALM (0x0b):**
   The endpoint detected that its peer is exhibiting a behavior that might be 
generating excessive load.
   
   ### RST_STREAM
   
   The Client is sending RST_STREAM frame to terminate the connection. And on 
the server side there is a rateControl code to mitigate the HTTP/2 Rapid Reset 
attack
   
   
https://github.com/jetty/jetty.project/blob/89c41b2550ed367a25d1664da8843f5a4e1019da/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/parser/ResetBodyParser.java#L88-L92
   
   ```
   private boolean onReset(ByteBuffer buffer, int error)
   {
   ResetFrame frame = new ResetFrame(getStreamId(), error);
   if (!rateControlOnEvent(frame))
   return connectionFailure(buffer, 
ErrorCode.ENHANCE_YOUR_CALM_ERROR.code, "invalid_rst_stream_frame_rate");
   reset();
   notifyReset(frame);
   return true;
   }
   ```
   
   ### The HTTP/2 Rapid Reset attack
   
   This attack is called Rapid Reset because it relies on the ability for an 
endpoint to send a RST_STREAM frame immediately after sending a request frame, 
which makes the other endpoint start working and then rapidly resets 

Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-27 Thread via GitHub


iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-2022152280

   For now, I fixed other issue related to closing output stream in 
IndexFetcher. IndexFetcher is closing the outputstream in unwanted manner which 
fails the retry operation. And with that, even if server sends GO_AWAY then 
IndexFetcher would try 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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-27 Thread via GitHub


iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-2022150807

   **GO_AWAY** frame is used to terminate the connection. In our case, Jetty 
server is sending **GO_AWAY** to terminate the connection as we sending 
multiple "GetStream" requests to fetch all the files. 
   
   > Caused by: java.nio.channels.AsynchronousCloseException
   >at 
org.eclipse.jetty.http2.client.http.HttpConnectionOverHTTP2.close(HttpConnectionOverHTTP2.java:217)
 ~[http2-http-client-transport-10.0.20.jar:10.0.20]
   >at 
org.eclipse.jetty.http2.client.http.HttpClientTransportOverHTTP2.onClose(HttpClientTransportOverHTTP2.java:175)
 ~[http2-http-client-transport-10.0.20.jar:10.0.20]
   >at 
org.eclipse.jetty.http2.client.http.HttpClientTransportOverHTTP2$SessionListenerPromise.onClose(HttpClientTransportOverHTTP2.java:194)
 ~[http2-http-client-transport-10.0.20.jar:10.0.20]
   >at 
org.eclipse.jetty.http2.client.http.HTTPSessionListenerPromise.onClose(HTTPSessionListenerPromise.java:101)
 ~[http2-http-client-transport-10.0.20.jar:10.0.20]
   >at 
org.eclipse.jetty.http2.api.Session$Listener.onClose(Session.java:260) 
~[http2-common-10.0.20.jar:10.0.20]
   >at 
org.eclipse.jetty.http2.HTTP2Session.notifyClose(HTTP2Session.java:1197) 
~[http2-common-10.0.20.jar:10.0.20]
   >at 
org.eclipse.jetty.http2.HTTP2Session$StreamsState.terminate(HTTP2Session.java:2153)
 ~[http2-common-10.0.20.jar:10.0.20]
   >at 
org.eclipse.jetty.http2.HTTP2Session$StreamsState.lambda$sendGoAwayAndTerminate$16(HTTP2Session.java:2059)
 ~[http2-common-10.0.20.jar:10.0.20]
   
   @dsmiley Does jetty uses separate files for logging? If yes, can you please 
tell me How can I access that file, especially in test environment.


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-19 Thread via GitHub


iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-2007163874

   IndexFetcher getStream fails, seen it multiple times now. Blocker!
   
   > ERROR 
(recoveryExecutor-452-thread-1-processing-unloadcollection_shard1_replica4 
null-484 core_node5 127.0.0.1:42871_solr unloadcollection shard1) 
[n:127.0.0.1:42871_solr c:unloadcollection s:shard1 r:core_node5 
x:unloadcollection_shard1_replica4 t:null-484] o.a.s.h.IndexFetcher Error 
fetching file, doing one retry...
   >   2>   => org.apache.solr.common.SolrException: Unable to download 
_7_Lucene99_0.tip completely. Downloaded 0!=76
   >   2>   at 
org.apache.solr.handler.IndexFetcher$FileFetcher.cleanup(IndexFetcher.java:1938)
   >   2> org.apache.solr.common.SolrException: Unable to download 
_7_Lucene99_0.tip completely. Downloaded 0!=76
   >   2>   at 
org.apache.solr.handler.IndexFetcher$FileFetcher.cleanup(IndexFetcher.java:1938)
 ~[main/:?]
   >   2>   at 
org.apache.solr.handler.IndexFetcher$FileFetcher.fetch(IndexFetcher.java:1801) 
~[main/:?]
   >   2>   at 
org.apache.solr.handler.IndexFetcher$FileFetcher.fetchFile(IndexFetcher.java:1775)
 [main/:?]
   >   2>   at 
org.apache.solr.handler.IndexFetcher.downloadIndexFiles(IndexFetcher.java:1195) 
[main/:?]
   >   2>   at 
org.apache.solr.handler.IndexFetcher.fetchLatestIndex(IndexFetcher.java:682) 
[main/:?]
   >   2>   at 
org.apache.solr.handler.IndexFetcher.fetchLatestIndex(IndexFetcher.java:419) 
[main/:?]
   >   2>   at 
org.apache.solr.handler.ReplicationHandler.doFetch(ReplicationHandler.java:467) 
[main/:?]
   >   2>   at 
org.apache.solr.cloud.RecoveryStrategy.replicate(RecoveryStrategy.java:243) 
[main/:10.0.0-SNAPSHOT 9775e47757b251257633d12a58da38228932fbd4 [snapshot 
build, details omitted]]
   >   2>   at 
org.apache.solr.cloud.RecoveryStrategy.doSyncOrReplicateRecovery(RecoveryStrategy.java:701)
 [main/:10.0.0-SNAPSHOT 9775e47757b251257633d12a58da38228932fbd4 [snapshot 
build, details omitted]]
   >   2>   at 
org.apache.solr.cloud.RecoveryStrategy.doRecovery(RecoveryStrategy.java:333) 
[main/:10.0.0-SNAPSHOT 9775e47757b251257633d12a58da38228932fbd4 [snapshot 
build, details omitted]]
   >   2>   at 
org.apache.solr.cloud.RecoveryStrategy.run(RecoveryStrategy.java:309) 
[main/:10.0.0-SNAPSHOT 9775e47757b251257633d12a58da38228932fbd4 [snapshot 
build, details omitted]]
   >   2>   at 
com.codahale.metrics.InstrumentedExecutorService$InstrumentedRunnable.run(InstrumentedExecutorService.java:212)
 [metrics-core-4.2.25.jar:4.2.25]


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-14 Thread via GitHub


dsmiley commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-1998300785

   Glad that you did the compression testing manually!
   I think at this point the PR just needs a few method/field renames to 
reflect to not confuse a SolrClient with an HttpClient.  And there's the 
IndexFetcher basic authentication that's obsolete.


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-14 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1525398141


##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -261,22 +260,23 @@ public String getMessage() {
 }
   }
 
-  private static HttpClient createHttpClient(
-  SolrCore core,
-  String httpBasicAuthUser,
-  String httpBasicAuthPassword,
-  boolean useCompression) {
+  private Http2SolrClient createHttpClient(
+  SolrCore core, String httpBasicAuthUser, String httpBasicAuthPassword, 
String leaderBaseUrl) {
 final ModifiableSolrParams httpClientParams = new ModifiableSolrParams();

Review Comment:
   I did some digging here.  IndexFetcher can be configured with 
httpBasicAuthUser and httpBasicAuthPassword in its solrconfig.xml based 
configuration however this isn't document, and seems rather ancient / legacy to 
me.  Let's just drop this.  Not sure it's even worth mentioning in upgrade 
notes.



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-12 Thread via GitHub


iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-1991993862

   So far we were only testing internalCompression but not the 
externalComrpession. So in one of the class, ReplicationTestHelper, I found a 
way to enable the external Compression, and now whenever it's on that would 
start sending ACCEPT-ENCODING header. 
   
   I still have not found a viable way to test the external Compression. As in 
I can confirm that header being send when externalCompression is enabled but 
how it affects the stream is not tested yet. Through Wireshark I try to trace 
these calls but not able to trace Http2 calls, only http1. 


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-07 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1516376085


##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -261,22 +260,23 @@ public String getMessage() {
 }
   }
 
-  private static HttpClient createHttpClient(
-  SolrCore core,
-  String httpBasicAuthUser,
-  String httpBasicAuthPassword,
-  boolean useCompression) {
+  private Http2SolrClient createHttpClient(

Review Comment:
   rename to "createSolrClient" so we don't confuse a lower level httpClient 
with a solrClient.  Whenever I see a method/field/param name saying 
"httpClient" but the type is actually "SolrClient"; it irks me.  httpSolrClient 
is fine if you prefer.



##
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##
@@ -261,22 +260,23 @@ public String getMessage() {
 }
   }
 
-  private static HttpClient createHttpClient(
-  SolrCore core,
-  String httpBasicAuthUser,
-  String httpBasicAuthPassword,
-  boolean useCompression) {
+  private Http2SolrClient createHttpClient(
+  SolrCore core, String httpBasicAuthUser, String httpBasicAuthPassword, 
String leaderBaseUrl) {
 final ModifiableSolrParams httpClientParams = new ModifiableSolrParams();

Review Comment:
   This is now unused.  I'd hope that authentication stuff would be handled in 
some other way (e.g. via configuration / initialization in UpdateShardHandler 
creating the recoveryOnlyHttpClient) so that internal Solr code here can merely 
get this pre-configured client.  That client should also already have an 
InstrumentedHttpListenerFactory too; no?



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-07 Thread via GitHub


iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-1983485002

   Test case which is failing. 
   > ./gradlew :solr:core:test --tests 
"org.apache.solr.handler.TestHealthCheckHandlerLegacyMode.doTestHealthCheckWithReplication"
 -Ptests.jvms=2 "-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC 
-XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" 
-Ptests.seed=6C9BE6E7222DDA0B -Ptests.file.encoding=US-ASCII 
-Ptests.verbose=true
   
   And If I remove the code which is removing the ACCEPT_ENCODING header in 
Http2SolrClient then it works fine! 
   `req.headers(headers -> headers.remove(HttpHeader.ACCEPT_ENCODING));`
   


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-07 Thread via GitHub


iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-1983413759

   There will be no need to add the extra code at SolrRequest level. I can just 
call add header for SolrRequest to add "ACCEPT_ENCODING:gzip"
   


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-07 Thread via GitHub


iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-1983401595

   ### Allow Compression
   HttpSolrClient provides feature to enable external compression that would 
add "ACCEPT_ENCODING: gzip" to the request headers. And we are using this 
feature in `IndexFetcher.class` while creating HttpSolrClient.
   
https://github.com/apache/solr/blob/a1104bd04a799ba99cba652922bfded51d0c2d72/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpClientUtil.java#L428-L433
   
   Now Jetty Http client by default add "ACCEPT_ENCODING: gzip" header to all 
the request.
   
   However, in `Http2SolrClient2.java` we have removed that header while 
decorating request.
   
   
https://github.com/apache/solr/blob/a1104bd04a799ba99cba652922bfded51d0c2d72/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java#L632-L634
   
   Added new commit that would allow users to enable external compression at 
request level. Now I only enabled for one request in IndexFetcher, I believe It 
should be added for each request. Or, we can add this functionality to 
Http2SolrClient which would add for all the request automatically. 


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-04 Thread via GitHub


dsmiley commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-1978009974

   >On a side note, How about we rethink the design decorating the request. 
Right now, we recreate the Http2SolrClient to override socketTimeout and 
connTimeout.
   
   Which is a fine approach, IMO.  They are light.
   
   >  Instead of recreating Http2SolrClient, we introduced another method which 
would override Timeouts.
   
   I was going to mention something like this but I'm opposed to another 
request() method like you propose.  Instead, SolrRequest could have the 
timeouts.  It already has other methods of a low-ish level like headers, URL, 
preferred nodes.  Just keep in mind we have 2 clients and another on the way so 
2-3 places to update to consider this timeout.


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-04 Thread via GitHub


iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-1977998958

   TestPullReplicaWithAuth.testPKIAuthWorksForPullReplication is the one.
   
   On a side note, How about we rethink the design decorating the request. 
Right now, we recreate the Http2SolrClient to override socketTimeout and 
connTimeout. Instead of recreating Http2SolrClient, we introduced another 
method which would override Timeouts.
   
   `request(SolrRequest solrRequest, String collection, int socketTimeout, 
int connTimeout)`


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-04 Thread via GitHub


dsmiley commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-1977700604

   Thanks for digging into the authentication issue further; this is shining a 
light on an issue I was vaguely aware of but now I can see it directly.  And 
I'm so glad there are tests; I thought it wasn't tested.  Can you name one 
please?
   
   One thing will help; stop excessive creation of new SolrClient instances 
when we can re-use the same one that is configured with this auth listener 
thing.  Thus don't have fields on classes that hold an HttpClient (we'll deem 
this bad/questionable practice); instead hold an Http2SolrClient.  If due to 
customized timeouts we need a new Http2SolrClient, we could ensure that the 
builder withHttpClient(http2SolrClient) copies listeners (and anything else 
like an executor; why not).  That could be its own JIRA.


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-03 Thread via GitHub


iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-1975233648

   @dsmiley  I was working on IndexFetcher to change their client to 
Http2SolrClient. Suddenly all the request started failing due to authentication 
issue. Further inspection revealed that the listener required for Auth is not 
registered for  recoveryOnlyClient. Interestingly, no calls in RecoveryStrategy 
requires Auth, and therefore no request failed there.
   
   Below is the code, that called on updateOnlyClient to register the Auth 
listener.
   
   
https://github.com/apache/solr/blob/1c6798b0a012f47ab88ed091b9015462016dc8e8/solr/core/src/java/org/apache/solr/update/UpdateShardHandler.java#L317-L319
   
   However, only calling this method for recoveryOnlyClient won't work. As 
recreating Http2SolrClient using withHttpClient won't pass on listeners. 
Basically same issue that we have faced with Executors.


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-02 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1510020415


##
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##
@@ -915,18 +907,15 @@ private final void sendPrepRecoveryCmd(String 
leaderBaseUrl, String leaderCoreNa
 int readTimeout =
 conflictWaitMs
 + 
Integer.parseInt(System.getProperty("prepRecoveryReadTimeoutExtraWait", 
"8000"));

Review Comment:
   ooh, okay, here we have a special timeout option unique to this method.



##
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##
@@ -175,24 +175,21 @@ public final void setRecoveringAfterStartup(boolean 
recoveringAfterStartup) {
 this.recoveringAfterStartup = recoveringAfterStartup;
   }
 
-  /** Builds a new HttpSolrClient for use in recovery. Caller must close */
-  private HttpSolrClient.Builder recoverySolrClientBuilder(String baseUrl, 
String leaderCoreName) {
+  private Http2SolrClient.Builder recoverySolrClientBuilder(String baseUrl, 
String leaderCoreName) {
 // workaround for SOLR-13605: get the configured timeouts & set them 
directly
 // (even though getRecoveryOnlyHttpClient() already has them set)
 final UpdateShardHandlerConfig cfg = 
cc.getConfig().getUpdateShardHandlerConfig();
-return (new HttpSolrClient.Builder(baseUrl)
+return new Http2SolrClient.Builder(baseUrl)
 .withDefaultCollection(leaderCoreName)
-.withConnectionTimeout(cfg.getDistributedConnectionTimeout(), 
TimeUnit.MILLISECONDS)
-.withSocketTimeout(cfg.getDistributedSocketTimeout(), 
TimeUnit.MILLISECONDS)
-
.withHttpClient(cc.getUpdateShardHandler().getRecoveryOnlyHttpClient()));
+
.withHttpClient(cc.getUpdateShardHandler().getRecoveryOnlyHttpClient());

Review Comment:
   FYI when re-using a common SolrClient for requests that may need to go to 
various URLs, use `solrRequest.setBasePath(leaderUrl)` to have the request 
indicate where it needs to go, for example.



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-02 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1510019464


##
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##
@@ -175,24 +175,21 @@ public final void setRecoveringAfterStartup(boolean 
recoveringAfterStartup) {
 this.recoveringAfterStartup = recoveringAfterStartup;
   }
 
-  /** Builds a new HttpSolrClient for use in recovery. Caller must close */
-  private HttpSolrClient.Builder recoverySolrClientBuilder(String baseUrl, 
String leaderCoreName) {
+  private Http2SolrClient.Builder recoverySolrClientBuilder(String baseUrl, 
String leaderCoreName) {
 // workaround for SOLR-13605: get the configured timeouts & set them 
directly
 // (even though getRecoveryOnlyHttpClient() already has them set)
 final UpdateShardHandlerConfig cfg = 
cc.getConfig().getUpdateShardHandlerConfig();
-return (new HttpSolrClient.Builder(baseUrl)
+return new Http2SolrClient.Builder(baseUrl)
 .withDefaultCollection(leaderCoreName)
-.withConnectionTimeout(cfg.getDistributedConnectionTimeout(), 
TimeUnit.MILLISECONDS)
-.withSocketTimeout(cfg.getDistributedSocketTimeout(), 
TimeUnit.MILLISECONDS)
-
.withHttpClient(cc.getUpdateShardHandler().getRecoveryOnlyHttpClient()));
+
.withHttpClient(cc.getUpdateShardHandler().getRecoveryOnlyHttpClient());
   }
 
   // make sure any threads stop retrying
   @Override
   public final void close() {
 close = true;
 if (prevSendPreRecoveryHttpUriRequest != null) {
-  prevSendPreRecoveryHttpUriRequest.abort();
+  prevSendPreRecoveryHttpUriRequest.cancel(true);

Review Comment:
   ofNullable here too? 



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-03-02 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1510019364


##
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##
@@ -175,24 +175,21 @@ public final void setRecoveringAfterStartup(boolean 
recoveringAfterStartup) {
 this.recoveringAfterStartup = recoveringAfterStartup;
   }
 
-  /** Builds a new HttpSolrClient for use in recovery. Caller must close */
-  private HttpSolrClient.Builder recoverySolrClientBuilder(String baseUrl, 
String leaderCoreName) {
+  private Http2SolrClient.Builder recoverySolrClientBuilder(String baseUrl, 
String leaderCoreName) {
 // workaround for SOLR-13605: get the configured timeouts & set them 
directly
 // (even though getRecoveryOnlyHttpClient() already has them set)
 final UpdateShardHandlerConfig cfg = 
cc.getConfig().getUpdateShardHandlerConfig();
-return (new HttpSolrClient.Builder(baseUrl)
+return new Http2SolrClient.Builder(baseUrl)
 .withDefaultCollection(leaderCoreName)
-.withConnectionTimeout(cfg.getDistributedConnectionTimeout(), 
TimeUnit.MILLISECONDS)
-.withSocketTimeout(cfg.getDistributedSocketTimeout(), 
TimeUnit.MILLISECONDS)
-
.withHttpClient(cc.getUpdateShardHandler().getRecoveryOnlyHttpClient()));
+
.withHttpClient(cc.getUpdateShardHandler().getRecoveryOnlyHttpClient());

Review Comment:
   Ah; now I see we can remove the obsolete comment, because you removed the 
explicit calls for timeout configuration that shouldn't be necessary (because 
they should be set on recoveryOnlyHttpClient).  Moreover, to my point earlier, 
we shouldn't need to be creating a new Http2SolrClient at all though!  Thus 
`recoverySolrClientBuilder` could disappear.



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-02-29 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1508138037


##
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##
@@ -175,8 +175,6 @@ public final void setRecoveringAfterStartup(boolean 
recoveringAfterStartup) {
   }
 
   private Http2SolrClient.Builder recoverySolrClientBuilder(String baseUrl, 
String leaderCoreName) {
-// workaround for SOLR-13605: get the configured timeouts & set them 
directly
-// (even though getRecoveryOnlyHttpClient() already has them set)

Review Comment:
   Removing the comment isn't fully what I was getting at because the code is 
still here pertinent to a work-around that isn't necessary.



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-02-29 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1508136097


##
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##
@@ -630,11 +628,8 @@ public final void doSyncOrReplicateRecovery(SolrCore core) 
throws Exception {
 .getCollection(cloudDesc.getCollectionName())
 .getSlice(cloudDesc.getShardId());
 
-try {
+if (prevSendPreRecoveryHttpUriRequest != null) {

Review Comment:
   I see this is correct because prevSendPreRecoveryHttpUriRequest never 
becomes null once it is non-null.  Otherwise a concurrent set to null would 
cause an NPE here if timed just right.  But this is super subtle!  It'd be 
clearer, even if slightly more code, to save the field access to a local 
variable where you check it then -- a typical pattern when a field can be 
accessed concurrently.  Perhaps there's a one-liner approach using 
`Optional.ofNullable(field).ifPresent(f -> f.cancel(true))`



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-02-28 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1507039357


##
solr/CHANGES.txt:
##
@@ -111,6 +111,8 @@ Improvements
 
 * SOLR-16138: Throw a exception when issuing a streaming expression and all 
cores are down instead of returning 0 documents. (Antoine Bursaux via Eric Pugh)
 
+* SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty 
HTTP2 (Sanjay Dutt, David Smiley)

Review Comment:
   IMO this better fits in "Other Changes" as it's more of a refactoring than 
any kind of improvement that would be noticed.  I suggest using words a user 
might slightly more likely understand like "Switch internal replica recovery 
commands to Jetty HTTP2".



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-02-28 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1506994591


##
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##
@@ -633,9 +631,10 @@ public final void doSyncOrReplicateRecovery(SolrCore core) 
throws Exception {
 .getSlice(cloudDesc.getShardId());
 
 try {
-  prevSendPreRecoveryHttpUriRequest.cancel();
+  prevSendPreRecoveryHttpUriRequest.cancel(true);
 } catch (NullPointerException e) {
   // okay
+  log.info("Failed to abort the Prep Recovery command as it has not 
been sent yet.");

Review Comment:
   No need to log; this is very normal and non-interesting.
   
   IMO even better to either ensure this is non-null (use an initial dummy 
value), or to check null before even calling cancel.  It's kinda sloppy to 
catch NPE.  Okay if you prefer to defer this!



##
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##
@@ -175,24 +174,21 @@ public final void setRecoveringAfterStartup(boolean 
recoveringAfterStartup) {
 this.recoveringAfterStartup = recoveringAfterStartup;
   }
 
-  /** Builds a new HttpSolrClient for use in recovery. Caller must close */
-  private HttpSolrClient.Builder recoverySolrClientBuilder(String baseUrl, 
String leaderCoreName) {
+  private Http2SolrClient.Builder recoverySolrClientBuilder(String baseUrl, 
String leaderCoreName) {

Review Comment:
   I observe that you changed UpdateShardHandler to have a SolrClient instead 
of an HttpClient for the recovery ops.  Nice; note that this is a higher level 
abstraction.  A ramification of this is that RecoveryStrategy shouldn't need to 
create brand new SolrClient instances because now we have one already pre-made! 
 Thus don't need to close them either.  I assume they all need the same 
configuration?  Also, I looked at the comment below RE SOLR-13605 and it's 
obsolete -- SOLR-13605 is closed yet this comment pre-dated it.



##
solr/CHANGES.txt:
##
@@ -111,6 +111,8 @@ Improvements
 
 * SOLR-16138: Throw a exception when issuing a streaming expression and all 
cores are down instead of returning 0 documents. (Antoine Bursaux via Eric Pugh)
 
+* SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty 
HTTP2 (Sanjay Dutt, David Smiley)

Review Comment:
   IMO this better fits in "Other Changes" as it's more of a refactoring than 
any kind of improvement that would be noticed.  I suggest using words a user 
might slightly more likely understand like "Switch replica recovery 
PREPRECOVERY command to Jetty HTTP2".



##
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##
@@ -915,18 +911,15 @@ private final void sendPrepRecoveryCmd(String 
leaderBaseUrl, String leaderCoreNa
 int readTimeout =
 conflictWaitMs
 + 
Integer.parseInt(System.getProperty("prepRecoveryReadTimeoutExtraWait", 
"8000"));
-try (HttpSolrClient client =
+try (Http2SolrClient client =

Review Comment:
   minor: please broaden the type to just SolrClient so we aren't overly 
specific (e.g. List vs ArrayList)



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-02-26 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1498431204


##
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##
@@ -946,7 +939,6 @@ public void onFailure(Throwable throwable) {
 }
   });
 } finally {
-  recoveryExec.shutdown();

Review Comment:
   By removing this, I see nothing here that will wait for this request to 
complete.  But that was the semantics before, with the `future.get()` call.  It 
kinda makes me wonder why we're even bothering with "async" request in the 
first place; couldn't we do a standard synchronous HTTP request?  One little 
thing I did see was the "abort" call.  But that could be done via using a 
FutureTask and calling `cancel(true)` on it.  Basically, the field 
`prevSendPreRecoveryHttpUriRequest` because a FutureTask.  Set it and call 
run() in `sendPrepRecoveryCmd`.  I think it's sloppy/ugly to catch NPE in some 
places in this file, yuck; we could use a dummy initial FutureTask value.
   
   I think this might end up deferring any "executor" matters in this PR since 
we then wouldn't need an executor; right?  It'd be a separate PR to improve 
Http2SolrClient executor initialization.



##
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##
@@ -911,18 +911,39 @@ private final void sendPrepRecoveryCmd(String 
leaderBaseUrl, String leaderCoreNa
 int readTimeout =
 conflictWaitMs
 + 
Integer.parseInt(System.getProperty("prepRecoveryReadTimeoutExtraWait", 
"8000"));
-try (HttpSolrClient client =
+var recoveryExec =
+ExecutorUtil.newMDCAwareFixedThreadPool(
+1, new SolrNamedThreadFactory("sendPrepRecoveryCmd"));
+try (Http2SolrClient client =
 recoverySolrClientBuilder(
 leaderBaseUrl,
 null) // leader core omitted since client only used for 
'admin' request
-.withSocketTimeout(readTimeout, TimeUnit.MILLISECONDS)
+.withIdleTimeout(readTimeout, TimeUnit.MILLISECONDS)
+.withExecutor(recoveryExec)
 .build()) {
-  HttpUriRequestResponse mrr = client.httpUriRequest(prepCmd);
-  prevSendPreRecoveryHttpUriRequest = mrr.httpUriRequest;
-
   log.info("Sending prep recovery command to [{}]; [{}]", leaderBaseUrl, 
prepCmd);
-
-  mrr.future.get();
+  MDC.put("HttpSolrClient.url", baseUrl);

Review Comment:
   Aaaah, you are imitating 
`org.apache.solr.client.solrj.impl.HttpSolrClient#httpUriRequest(org.apache.solr.client.solrj.SolrRequest,
 org.apache.solr.client.solrj.ResponseParser)`.  But there's no need because 
the Http2Solrclient natively supports an async method, which should be the 
place where any MDC/logging is specified.
   
   Note that below I question if we even need async logic in the first place!



-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-02-20 Thread via GitHub


iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-1955856284

   I was thinking about the executor in the Http2SolrClient class. 
   
   **What is the issue?**
   1. Http2SolrClent calls `createHttpClient` to create a Jetty HTTP client. By 
default, If executor is not provided then a executor will be created for it to 
initialize the Jetty HTTP.
   ```
  private HttpClient createHttpClient(Builder builder) {
   HttpClient httpClient;
   
   executor = builder.executor;
   if (executor == null) {
 BlockingArrayQueue queue = new BlockingArrayQueue<>(256, 
256);
 this.executor =
 new ExecutorUtil.MDCAwareThreadPoolExecutor(
 32, 256, 60, TimeUnit.SECONDS, queue, new 
SolrNamedThreadFactory("h2sc"));
 shutdownExecutor = true;
   } else {
 shutdownExecutor = false;
   }
   ```
   2. However, If someone creates Http2SolrClient.Builder with existing Jetty 
HTTP client by calling `withHttpClient` there will be no executor available to 
them, even if `withExecutor` method is called. Therefore, all the asyncRequest 
will result in NullPointerException. Because only `createHttpClient` is the 
only method which handles the initialization of executor and If new builder is 
created is using existing Jetty client then no call is made to 
`createHttpClient` and hence no executor.
   
   **Solution**
   There are two ways that I am thinking as of now to handle it, in case If new 
builder is created is using the existing Jetty HTTP client.
   
   1.  In Http2SolrClient constructor, below snipped of code is added to check 
whether executor is null or not. And then re-initlialize the executor at 
Http2SolrClient level. At this point, we have two executors: one for 
Http2SolrClient and another for Jetty Http client. And Http2SolrClient is only 
responsible for closing the ones they making from inside, and If you are 
providing any Executor from outside, then user will be responsible for closing 
it. 
   
   ```
   if (this.executor == null) {
   this.executor = http2SolrClient.executor;
 }
   ```
   
   2. Second, As we recreate builder using existing Jetty Client by calling 
`withHttpClient`, there we can also add code that would also set executor(one 
which we created calling `createHttpClient`) for Http2SolrClient as well among 
with other properties. But that means that Every builder recreated using the 
existing Jetty client will have access to executor without them knowing about 
it. Below is that executor. Something I am not much comfortable about, I 
believe If user creating Http2SolrClient to send asyncRequest then they should 
provide the implementation for executor. 
   
   Also In this option there will be code to check wh
   
   ```
BlockingArrayQueue queue = new BlockingArrayQueue<>(256, 256);
 this.executor =
 new ExecutorUtil.MDCAwareThreadPoolExecutor(
 32, 256, 60, TimeUnit.SECONDS, queue, new 
SolrNamedThreadFactory("h2sc"));
   ```
   
   @dsmiley wdyt? I have changed the code to implement the second option for 
now.
   
   @stillalex I see that we transferred some properties from older 
Http2SolrClient as we call `withHttpClient` to create new Http2SolrClient but 
the exisitng executor was never used for the new builder. Was there any reason 
for that?


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-02-20 Thread via GitHub


epugh commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-1954855217

   Thank you @dsmiley  for reviewing this PR...   I'm excited for the work 
@iamsanjay  has been doing, and I do NOT feel comfortable reviewing/merging 
code in this area ;-)


-- 
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-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org



Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]

2024-02-19 Thread via GitHub


dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1495075326


##
solr/core/src/test/org/apache/solr/cloud/RecoveryStrategyStressTest.java:
##
@@ -0,0 +1,135 @@
+/*
+ * 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.cloud;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrQuery;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.impl.CloudLegacySolrClient;
+import org.apache.solr.client.solrj.impl.HttpSolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.UpdateRequest;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.Replica;
+import org.apache.solr.common.cloud.Slice;
+import org.apache.solr.common.util.SolrNamedThreadFactory;
+import org.apache.solr.embedded.JettySolrRunner;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class RecoveryStrategyStressTest extends SolrCloudTestCase {
+
+  @BeforeClass
+  public static void setupCluster() throws Exception {
+cluster = configureCluster(4).addConfig("conf", 
configset("cloud-minimal")).configure();
+  }
+
+  @Test
+  public void stressTestRecovery() throws Exception {
+final String collection = "recoveryStressTest";
+CollectionAdminRequest.createCollection(collection, "conf", 1, 4)
+.process(cluster.getSolrClient());
+waitForState(
+"Expected a collection with one shard and two replicas", collection, 
clusterShape(1, 4));
+
+SolrClient solrClient =

Review Comment:
   why not declare this in the `try`?



##
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##
@@ -911,18 +911,39 @@ private final void sendPrepRecoveryCmd(String 
leaderBaseUrl, String leaderCoreNa
 int readTimeout =
 conflictWaitMs
 + 
Integer.parseInt(System.getProperty("prepRecoveryReadTimeoutExtraWait", 
"8000"));
-try (HttpSolrClient client =
+var recoveryExec =
+ExecutorUtil.newMDCAwareFixedThreadPool(
+1, new SolrNamedThreadFactory("sendPrepRecoveryCmd"));
+try (Http2SolrClient client =
 recoverySolrClientBuilder(
 leaderBaseUrl,
 null) // leader core omitted since client only used for 
'admin' request
-.withSocketTimeout(readTimeout, TimeUnit.MILLISECONDS)
+.withIdleTimeout(readTimeout, TimeUnit.MILLISECONDS)
+.withExecutor(recoveryExec)
 .build()) {
-  HttpUriRequestResponse mrr = client.httpUriRequest(prepCmd);
-  prevSendPreRecoveryHttpUriRequest = mrr.httpUriRequest;
-
   log.info("Sending prep recovery command to [{}]; [{}]", leaderBaseUrl, 
prepCmd);
-
-  mrr.future.get();
+  MDC.put("HttpSolrClient.url", baseUrl);
+  prevSendPreRecoveryHttpUriRequest =
+  client.asyncRequest(
+  prepCmd,
+  null,
+  new AsyncListener<>() {
+@Override
+public void onSuccess(NamedList entries) {
+  log.info(
+  "Prep recovery command successfully send to [{}]; [{}]",

Review Comment:
   send -> sent



##
solr/core/src/test/org/apache/solr/cloud/RecoveryStrategyStressTest.java:
##
@@ -0,0 +1,135 @@
+/*
+ * 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,
+ *