Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SOLR-16503: Replace default USH apache client with Http2SolrClient #2741

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open

SOLR-16503: Replace default USH apache client with Http2SolrClient #2741

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
3f3a75e
introducing new home for Default solr client
iamsanjay Sep 2, 2024
a4615d1
format code
iamsanjay Sep 2, 2024
8062a1e
HttpSolrClientProvider for default http client
iamsanjay Sep 4, 2024
0701cac
Merge branch 'main' into SOLR-16503_default_Http2SolrClient
iamsanjay Sep 4, 2024
f80d3d2
format code
iamsanjay Sep 4, 2024
82a6255
Deleted ServerSolrClientCache and related code
iamsanjay Sep 5, 2024
05630c4
Change return type to Http2SolrClient
iamsanjay Sep 6, 2024
a6489a7
Merge branch 'main' into SOLR-16503_default_Http2SolrClient
iamsanjay Sep 6, 2024
ec1d9b5
refector ReindexCollectionCmd
iamsanjay Sep 6, 2024
ee0e888
Added test cases
iamsanjay Sep 9, 2024
0da4466
Merge branch 'main' into SOLR-16503_default_Http2SolrClient
iamsanjay Sep 9, 2024
7729fd9
tidy code
iamsanjay Sep 9, 2024
f27a07f
Revert default usage to http2 for now
iamsanjay Sep 9, 2024
c7e8ed9
Added override anotation for setup
iamsanjay Sep 9, 2024
bebaf2d
Reverting TestCoreContainer
iamsanjay Sep 10, 2024
03386e4
Merge branch 'main' into SOLR-16503_default_Http2SolrClient
iamsanjay Sep 10, 2024
6db8ee6
Enabling option for maxUpdateConnectionsPerHost setting for default c…
iamsanjay Sep 10, 2024
212c65b
Change Closeable to AutoCloseable for broader resource management
iamsanjay Sep 10, 2024
59f700b
Merge branch 'main' into SOLR-16503_default_Http2SolrClient
iamsanjay Sep 26, 2024
1695d1e
Added doc, chnaged names and return new Http2SolrClient
iamsanjay Sep 26, 2024
c49b792
Merge branch 'main' into SOLR-16503_default_Http2SolrClient
iamsanjay Oct 4, 2024
f849ad0
Added doc and changed method signature
iamsanjay Oct 4, 2024
dffd95c
Replace usage of default apache http client
iamsanjay Oct 4, 2024
76e9284
tidy
iamsanjay Oct 4, 2024
04e17a3
Merge branch 'main' into replace_defaultClient_usage
iamsanjay Oct 5, 2024
7c160a5
Start using requestWithBaseUrl
iamsanjay Oct 5, 2024
786cb6d
makes some changes to test
iamsanjay Oct 7, 2024
cdf2775
Merge branch 'main' into replace_defaultClient_usage
iamsanjay Oct 7, 2024
9331bbf
Add another assertion to make sure idleTimeout not set to defaultIdle…
iamsanjay Oct 7, 2024
6a6f536
gradlew check fix
iamsanjay Oct 7, 2024
acecf82
replaced with method reference
iamsanjay Oct 7, 2024
6dd2383
Merge branch 'main' into replace_defaultClient_usage
iamsanjay Oct 17, 2024
3b432e6
removed creation of http2SolrClient instead uses requestWithBaseUrl
iamsanjay Oct 17, 2024
03bbe45
Merge branch 'main' into replace_defaultClient_usage
iamsanjay Oct 25, 2024
07e6bb4
MirroringUpdateProcessor switch to Http2SolrClient
iamsanjay Oct 25, 2024
1ba30bd
Removed httpclient dependency from cross-dc
iamsanjay Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions solr/core/src/java/org/apache/solr/cloud/Overseer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.cloud.SolrCloudManager;
import org.apache.solr.client.solrj.impl.CloudHttp2SolrClient;
import org.apache.solr.client.solrj.impl.CloudSolrClient;
import org.apache.solr.client.solrj.impl.Http2SolrClient;
import org.apache.solr.client.solrj.request.CollectionAdminRequest;
import org.apache.solr.client.solrj.response.CollectionAdminResponse;
import org.apache.solr.cloud.api.collections.CreateCollectionCmd;
Expand Down Expand Up @@ -857,13 +857,18 @@ private void doCompatCheck(BiConsumer<String, Object> consumer) {
return;
}

try (CloudSolrClient client =
new CloudHttp2SolrClient.Builder(
Collections.singletonList(getZkController().getZkServerAddress()), Optional.empty())
.withZkClientTimeout(30000, TimeUnit.MILLISECONDS)
.withZkConnectTimeout(15000, TimeUnit.MILLISECONDS)
.withHttpClient(getCoreContainer().getDefaultHttpSolrClient())
.build()) {
try (var solrClient =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this client is embedded into the Cloud one, I think you should name this var maybe internalClient or something and don't refer to it in the block of code for the try-finally except for passing into the CloudSolrClient.

BTW; it really is painful to customize these particular timeouts as we're forced to do it at an inner layer. Ugh. @epugh not sure if you worked on some of the SolrClient building methods and saw this issue before. Maybe we should add the same methods to the CloudHttp2SolrClient.Builder

new Http2SolrClient.Builder()
.withHttpClient(getCoreContainer().getDefaultHttpSolrClient())
.withIdleTimeout(30000, TimeUnit.MILLISECONDS)
.withConnectionTimeout(15000, TimeUnit.MILLISECONDS)
.build();
var client =
new CloudHttp2SolrClient.Builder(
Collections.singletonList(getZkController().getZkServerAddress()),
Optional.empty())
.withHttpClient(solrClient)
.build()) {
CollectionAdminRequest.ColStatus req =
CollectionAdminRequest.collectionStatus(CollectionAdminParams.SYSTEM_COLL)
.setWithSegments(true)
Expand Down
28 changes: 10 additions & 18 deletions solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ private boolean fetchFileFromNodeAndPersist(String fromNode) {
try {
GenericSolrRequest request = new GenericSolrRequest(GET, "/node/files" + getMetaPath());
request.setResponseParser(new InputStreamResponseParser(null));
var response = solrClient.requestWithBaseUrl(baseUrl, client -> client.request(request));
var response = solrClient.requestWithBaseUrl(baseUrl, request::process).getResponse();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is rather nice. @gerlowskija, I think we can do away with the lambda-less requestWithBaseUrl I was suggesting. WDYT?

is = (InputStream) response.get("stream");
metadata =
Utils.newBytesConsumer((int) MAX_PKG_SIZE).accept((InputStream) response.get("stream"));
Expand All @@ -210,7 +210,7 @@ private boolean fetchFileFromNodeAndPersist(String fromNode) {
try {
GenericSolrRequest request = new GenericSolrRequest(GET, "/node/files" + path);
request.setResponseParser(new InputStreamResponseParser(null));
var response = solrClient.requestWithBaseUrl(baseUrl, client -> client.request(request));
var response = solrClient.requestWithBaseUrl(baseUrl, request::process).getResponse();
is = (InputStream) response.get("stream");
ByteBuffer filedata =
Utils.newBytesConsumer((int) MAX_PKG_SIZE).accept((InputStream) response.get("stream"));
Expand Down Expand Up @@ -249,18 +249,12 @@ boolean fetchFromAnyNode() {

final var request = new GenericSolrRequest(GET, "/node/files" + path, solrParams);
boolean nodeHasBlob = false;
var solrClient = coreContainer.getDefaultHttpSolrClient();
var resp = solrClient.requestWithBaseUrl(baseUrl, request::process).getResponse();

try (var solrClient =
new Http2SolrClient.Builder(baseUrl)
.withHttpClient(coreContainer.getDefaultHttpSolrClient())
.build()) {
var resp = solrClient.request(request);

if (Utils.getObjectByPath(resp, false, Arrays.asList("files", path)) != null) {
nodeHasBlob = true;
}
if (Utils.getObjectByPath(resp, false, Arrays.asList("files", path)) != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are convenience methods on NamedList avoiding the need to refer to this awkward utility method. Try findRecursive.

nodeHasBlob = true;
}

if (nodeHasBlob) {
boolean success = fetchFileFromNodeAndPersist(liveNode);
if (success) return true;
Expand Down Expand Up @@ -398,16 +392,14 @@ private void distribute(FileInfo info) {
// almost FETCHFROM_SRC other nodes may have it
getFrom += "*";
}
try (var solrClient =
new Http2SolrClient.Builder(baseUrl)
.withHttpClient(coreContainer.getDefaultHttpSolrClient())
.build()) {
try {
var solrClient = coreContainer.getDefaultHttpSolrClient();
var solrParams = new ModifiableSolrParams();
solrParams.set("getFrom", getFrom);

var solrRequest = new GenericSolrRequest(GET, "/node/files" + info.path, solrParams);
var request = new GenericSolrRequest(GET, "/node/files" + info.path, solrParams);
// fire and forget
solrClient.request(solrRequest);
solrClient.requestWithBaseUrl(baseUrl, request::process);
} catch (Exception e) {
log.info("Node: {} failed to respond for file fetch notification", node, e);
// ignore the exception
Expand Down
19 changes: 8 additions & 11 deletions solr/core/src/java/org/apache/solr/pkg/PackageAPI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
import org.apache.solr.client.solrj.SolrRequest;
import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.impl.BinaryResponseParser;
import org.apache.solr.client.solrj.impl.Http2SolrClient;
import org.apache.solr.client.solrj.request.GenericSolrRequest;
import org.apache.solr.client.solrj.request.beans.PackagePayload;
import org.apache.solr.common.SolrException;
Expand Down Expand Up @@ -260,18 +259,16 @@ public void refresh(PayloadObj<String> payload) {
solrParams.add("omitHeader", "true");
solrParams.add("refreshPackage", p);

final var solrRequest =
final var request =
new GenericSolrRequest(SolrRequest.METHOD.GET, "/cluster/package", solrParams);
solrRequest.setResponseParser(new BinaryResponseParser());
request.setResponseParser(new BinaryResponseParser());

for (String liveNode : FileStoreUtils.fetchAndShuffleRemoteLiveNodes(coreContainer)) {
final var baseUrl =
coreContainer.getZkController().zkStateReader.getBaseUrlV2ForNodeName(liveNode);
try (var solrClient =
new Http2SolrClient.Builder(baseUrl)
.withHttpClient(coreContainer.getDefaultHttpSolrClient())
.build()) {
solrClient.request(solrRequest);
try {
var solrClient = coreContainer.getDefaultHttpSolrClient();
solrClient.requestWithBaseUrl(baseUrl, request::process);
} catch (SolrServerException | IOException e) {
throw new SolrException(
SolrException.ErrorCode.SERVER_ERROR,
Expand Down Expand Up @@ -444,15 +441,15 @@ void notifyAllNodesToSync(int expected) {
solrParams.add("omitHeader", "true");
solrParams.add("expectedVersion", String.valueOf(expected));

final var solrRequest =
final var request =
new GenericSolrRequest(SolrRequest.METHOD.GET, "/cluster/package", solrParams);
solrRequest.setResponseParser(new BinaryResponseParser());
request.setResponseParser(new BinaryResponseParser());

for (String liveNode : FileStoreUtils.fetchAndShuffleRemoteLiveNodes(coreContainer)) {
var baseUrl = coreContainer.getZkController().zkStateReader.getBaseUrlV2ForNodeName(liveNode);
try {
var solrClient = coreContainer.getDefaultHttpSolrClient();
solrClient.requestWithBaseUrl(baseUrl, client -> client.request(solrRequest));
solrClient.requestWithBaseUrl(baseUrl, request::process);
} catch (SolrServerException | IOException e) {
throw new SolrException(
SolrException.ErrorCode.SERVER_ERROR,
Expand Down
3 changes: 1 addition & 2 deletions solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,8 +354,7 @@ PublicKey fetchPublicKeyFromRemote(String nodename) {
final var request = new GenericSolrRequest(GET, PublicKeyHandler.PATH, solrParams);
log.debug("Fetching fresh public key from: {}", url);
var solrClient = cores.getDefaultHttpSolrClient();
NamedList<Object> resp =
solrClient.requestWithBaseUrl(url, client -> client.request(request));
NamedList<Object> resp = solrClient.requestWithBaseUrl(url, request::process).getResponse();

String key = (String) resp.get("key");
if (key == null) {
Expand Down
7 changes: 3 additions & 4 deletions solr/core/src/test/org/apache/solr/core/TestHttpSolrClientProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,10 @@ public void test_when_updateShardHandler_cfg_is_null() {
@Test
public void test_when_updateShardHandler_cfg_is_not_null() {
var idleTimeout = 10000;
assertNotEquals(idleTimeout, UpdateShardHandlerConfig.DEFAULT.getDistributedSocketTimeout());
UpdateShardHandlerConfig cfg = new UpdateShardHandlerConfig(-1, -1, idleTimeout, -1, null, -1);
iamsanjay marked this conversation as resolved.
Show resolved Hide resolved
try (var httpSolrClientProvider = new HttpSolrClientProvider(cfg, parentSolrMetricCtx); ) {
assertNotEquals(
httpSolrClientProvider.getSolrClient().getIdleTimeout(),
UpdateShardHandlerConfig.DEFAULT.getDistributedSocketTimeout());
assertEquals(httpSolrClientProvider.getSolrClient().getIdleTimeout(), idleTimeout);
}
}

Expand All @@ -67,7 +66,7 @@ public void test_closing_solr_metric_context() {
Mockito.when(parentSolrMetricCtx.getChildContext(any(HttpSolrClientProvider.class)))
.thenReturn(childSolrMetricContext);
try (var httpSolrClientProvider = new HttpSolrClientProvider(null, parentSolrMetricCtx)) {
// Just to make sure we are closing solr metrics object
assertNotNull(httpSolrClientProvider.getSolrClient());
} finally {
verify(childSolrMetricContext, times(1)).unregister();
}
Expand Down
Loading