Re: [PR] SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 [solr]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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, + *