gerlowskija commented on code in PR #4046:
URL: https://github.com/apache/solr/pull/4046#discussion_r2695206685
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/DeleteAliasApi.java:
##########
@@ -32,7 +32,7 @@ public interface DeleteAliasApi {
@Operation(
summary = "Deletes an alias by its name",
tags = {"aliases"})
- SolrJerseyResponse deleteAlias(
+ SubResponseAccumulatingJerseyResponse deleteAlias(
Review Comment:
[Q] Why are we narrowing the response schema of this API?
Typically "SubResponseAccumulatingJerseyResponse" gets used for those admin
APIs where the overseer sends out requests to individual replicas, etc. and
includes each of those sub-responses in what gets returned to the original
caller. But looking at DeleteAliasCmd, it doesn't seem to be doing anything
like that?
There's a (minor?) downside to doing this in that our OpenAPI spec (and
everything that gets generated from it) will now look like the additional
fields will be present in the API response.
(Note: this question applies to a few of the files above as well, but just
leaving it in this one place to avoid duplication.)
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/DistributedCollectionConfigSetCommandRunner.java:
##########
@@ -439,21 +445,25 @@ public OverseerSolrResponse call() {
// hierarchy do no harm, and there shouldn't be too many of those.
lock.release();
} catch (SolrException se) {
- log.error(
- "Error when releasing collection locks for operation " +
action, se); // nowarn
+ if (log.isErrorEnabled()) {
+ log.error(
+ "Error when releasing collection locks for operation {}",
Review Comment:
[-0] This log line won't print out the full stack trace, exception message,
etc. the way it seems like you're assuming. (At least, unless slf4j has
changed since the last time I ran into this.)
The problem is the use of multiple arguments after the log string. Once you
have > 2 method args, you'll be calling the varargs version of this method
(i.e. `Logger.error(String, Object...)` which treats all arguments after the
log-string (including the exception) as values to be interpolated into the log
string. Since you only have one `{}` in your log string, the exception gets
ignored.
To fix, go back to using string concatenation 👍
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java:
##########
@@ -174,6 +174,11 @@ public void call(ClusterState clusterState, ZkNodeProps
message, NamedList<Objec
collectionParams,
ccc.getCoreContainer().getConfigSetService());
+ Map<String, Object> propMap = new HashMap<>();
Review Comment:
[0] I've seen a few little snippets like this one so far; might be worth
some kind of "cloneZkPropsWithOperation" utility method 🤷
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/MigrateCmd.java:
##########
@@ -312,17 +309,26 @@ private void migrateKey(
configName,
CollectionHandlingUtils.CREATE_NODE_SET,
sourceLeader.getNodeName());
- if (asyncId != null) {
- String internalAsyncId = asyncId + Math.abs(System.nanoTime());
- props.put(ASYNC, internalAsyncId);
+ String internalAsyncId = null;
+ if (adminCmdContext.getAsyncId() != null) {
Review Comment:
[Q] Wdyt of having some sort of `getInternalAsyncId()` or
`getSubrequestAsyncId()` method in AdminCmdContext that would encapsulate this
logic?
Seems like it's something a number of cmd implementations have to figure out
for themselves, and would definitely fit under the general umbrella of "admin
command context" conceptually.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
