dsmiley commented on code in PR #2741:
URL: https://github.com/apache/solr/pull/2741#discussion_r1849072274
##########
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
##########
@@ -484,14 +504,24 @@ public List<FileDetails> list(String path,
Predicate<String> predicate) {
public void delete(String path) {
deleteLocal(path);
List<String> nodes =
FileStoreUtils.fetchAndShuffleRemoteLiveNodes(coreContainer);
- HttpClient client =
coreContainer.getUpdateShardHandler().getDefaultHttpClient();
+
+ final var solrParams = new ModifiableSolrParams();
+ solrParams.add("localDelete", "true");
+ final var solrRequest = new GenericSolrRequest(DELETE, "/cluster/files" +
path, solrParams);
+
for (String node : nodes) {
String baseUrl =
coreContainer.getZkController().getZkStateReader().getBaseUrlV2ForNodeName(node);
- String url = baseUrl + "/cluster/files" + path + "?localDelete=true";
- HttpDelete del = new HttpDelete(url);
- // invoke delete command on all nodes asynchronously
- coreContainer.runAsync(() -> Utils.executeHttpMethod(client, url, null,
del));
+ try {
+ var solrClient = coreContainer.getDefaultHttpSolrClient();
+ // invoke delete command on all nodes asynchronously
+ solrClient.requestWithBaseUrl(baseUrl, client ->
client.requestAsync(solrRequest));
Review Comment:
if submitted asynchronously, then we won't actually get the exception (if it
happens remotely). requestAsync returns immediately.
solrClient.requestWithBaseUrl takes a lambda and declares that it throws (hence
forcing you to catch something) but it has no clue that, in this case, nothing
will actually ever be thrown.
Maybe simply add a code comment to say that we don't expect this will happen
due to requestAsync. This is not the only instance of this scenario;
@gerlowskija recently encountered it (with my review).
##########
solr/modules/cross-dc/src/java/org/apache/solr/crossdc/update/processor/MirroringUpdateProcessor.java:
##########
@@ -233,7 +223,9 @@ public void processDelete(final DeleteUpdateCommand cmd)
throws IOException {
boolean done = false;
while (!done) {
q.set(CursorMarkParams.CURSOR_MARK_PARAM, cursorMark);
- QueryResponse rsp = client.query(collection, q);
+ var baseUrl =
cmd.getReq().getCore().getCoreContainer().getZkController().getBaseUrl();
+ var client =
cmd.getReq().getCoreContainer().getDefaultHttpSolrClient();
+ QueryResponse rsp = client.requestWithBaseUrl(baseUrl, c ->
c.query(collection, q));
Review Comment:
I believe getDefaulthttpSolrClient, will already have its URL as that of
getZkController.getBaseUrl as it's the same node, the current node. Thus can
issue a standard request without `requestWithBaseUrl`.
##########
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
##########
@@ -180,23 +184,35 @@ private boolean fetchFileFromNodeAndPersist(String
fromNode) {
ByteBuffer metadata = null;
Map<?, ?> m = null;
+
+ InputStream is = null;
+ var solrClient = coreContainer.getDefaultHttpSolrClient();
+
try {
+ GenericSolrRequest request = new GenericSolrRequest(GET, "/node/files"
+ getMetaPath());
+ request.setResponseParser(new InputStreamResponseParser(null));
+ var response = solrClient.requestWithBaseUrl(baseUrl,
request::process).getResponse();
+ is = (InputStream) response.get("stream");
metadata =
- Utils.executeGET(
- coreContainer.getUpdateShardHandler().getDefaultHttpClient(),
- baseUrl + "/node/files" + getMetaPath(),
- Utils.newBytesConsumer((int) MAX_PKG_SIZE));
+ Utils.newBytesConsumer((int) MAX_PKG_SIZE).accept((InputStream)
response.get("stream"));
m = (Map<?, ?>) Utils.fromJSON(metadata.array(),
metadata.arrayOffset(), metadata.limit());
Review Comment:
@iamsanjay ping.
BTW, updating what I said, if we must have a Map and not a NamedList then
NamedList.toMap(10) would be reasonable.
##########
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
##########
@@ -221,17 +242,18 @@ boolean fetchFromAnyNode() {
try {
String baseUrl =
coreContainer.getZkController().getZkStateReader().getBaseUrlV2ForNodeName(liveNode);
- String reqUrl = baseUrl + "/node/files" + path +
"?meta=true&wt=javabin&omitHeader=true";
+ final var solrParams = new ModifiableSolrParams();
+ solrParams.add("meta", "true");
+ solrParams.add("omitHeader", "true");
+
+ final var request = new GenericSolrRequest(GET, "/node/files" +
path, solrParams);
boolean nodeHasBlob = false;
- Object nl =
- Utils.executeGET(
- coreContainer.getUpdateShardHandler().getDefaultHttpClient(),
- reqUrl,
- Utils.JAVABINCONSUMER);
- if (Utils.getObjectByPath(nl, false, Arrays.asList("files", path))
!= null) {
+ var solrClient = coreContainer.getDefaultHttpSolrClient();
+ var resp = solrClient.requestWithBaseUrl(baseUrl,
request::process).getResponse();
+
+ if (Utils.getObjectByPath(resp, false, Arrays.asList("files", path))
!= null) {
Review Comment:
ping
--
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
