gerlowskija commented on code in PR #4046:
URL: https://github.com/apache/solr/pull/4046#discussion_r2695688568
##########
solr/core/src/java/org/apache/solr/handler/admin/api/AddReplicaProperty.java:
##########
@@ -62,7 +58,7 @@ public AddReplicaProperty(
@Override
@PermissionName(COLL_EDIT_PERM)
- public SolrJerseyResponse addReplicaProperty(
+ public SubResponseAccumulatingJerseyResponse addReplicaProperty(
Review Comment:
[+1] Good catch
##########
solr/core/src/java/org/apache/solr/handler/admin/api/InstallShardData.java:
##########
@@ -81,27 +78,17 @@ public AsyncJerseyResponse installShardData(
// Only install data to shards which belong to a collection in read-only
mode
final DocCollection dc =
coreContainer.getZkController().getZkStateReader().getCollection(collName);
- if (!dc.isReadOnly()) {
+ if (dc.getSlice(shardName).getReplicas().size() > 1 && !dc.isReadOnly()) {
Review Comment:
[Q] Is there a connection I'm missing to the main thrust of this PR (i.e.
introducing AdminCmdContext), or is this unrelated as it appears?
I don't ask that to be nit-picky about making every little thing it's own PR
- it seems small enough that I'm not too worried about that in this case. But
it's probably worth calling out with its own changelog entry or something if it
is unrelated to the AdminCmdContext stuff.
##########
solr/core/src/java/org/apache/solr/handler/admin/api/CreateAlias.java:
##########
@@ -83,9 +78,9 @@ public CreateAlias(
@Override
@PermissionName(COLL_EDIT_PERM)
- public SolrJerseyResponse createAlias(CreateAliasRequestBody requestBody)
throws Exception {
- final SubResponseAccumulatingJerseyResponse response =
- instantiateJerseyResponse(SubResponseAccumulatingJerseyResponse.class);
+ public SubResponseAccumulatingJerseyResponse
createAlias(CreateAliasRequestBody requestBody)
Review Comment:
[-0] Idk why L88 was instantiating a "SubResponseAccumulatingJerseyResponse"
prior to this PR, maybe a copy-paste error on my part?
But in reality, this API never sets the "successes" or "failures" fields
that are the main reason for `SubResponse...` to exist. So rather than
switching the return type here to SubResponse, we should probably make it
"SolrJerseyResponse" in both places? Or maybe "AsyncJerseyResponse" if we want
to support asynchronous alias creation?
##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteAliasAPITest.java:
##########
@@ -17,28 +17,109 @@
package org.apache.solr.handler.admin.api;
-import static org.apache.solr.cloud.Overseer.QUEUE_OPERATION;
-import static org.apache.solr.common.params.CommonAdminParams.ASYNC;
-import static org.apache.solr.common.params.CoreAdminParams.NAME;
-
-import java.util.Map;
-import org.apache.solr.SolrTestCaseJ4;
+import java.lang.invoke.MethodHandles;
+import java.time.Duration;
+import org.apache.solr.client.api.model.SubResponseAccumulatingJerseyResponse;
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.impl.ZkClientClusterStateProvider;
+import org.apache.solr.client.solrj.request.AliasesApi;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.response.RequestStatusState;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.junit.BeforeClass;
import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** Integration tests for {@link DeleteAlias} using the V2 API */
+public class DeleteAliasAPITest extends SolrCloudTestCase {
Review Comment:
[Q] Why does this API get an "integration" test when most other APIs in this
PR so far all rely on mock-based testing?
Not suggesting it's a sub-par approach, just trying to think through in my
own mind what's going to be sustainable for us as we move v2 forward and get it
better tested.
##########
solr/core/src/test/org/apache/solr/handler/admin/api/RestoreCollectionAPITest.java:
##########
@@ -110,45 +111,49 @@ public void
testReportsErrorIfProvidedCollectionNameIsInvalid() {
requestBody.collection = "invalid$collection@name";
final SolrException thrown =
expectThrows(
- SolrException.class,
- () -> {
- restoreCollectionAPI.restoreCollection("someBackupName",
requestBody);
- });
+ SolrException.class, () -> api.restoreCollection("someBackupName",
requestBody));
assertEquals(400, thrown.code());
assertThat(
thrown.getMessage(), containsString("Invalid collection:
[invalid$collection@name]"));
}
@Test
- public void testCreatesValidRemoteMessageForExistingCollectionRestore() {
+ public void testCreatesValidRemoteMessageForExistingCollectionRestore()
throws Exception {
final var requestBody = new RestoreCollectionRequestBody();
requestBody.collection = "someCollectionName";
- requestBody.location = "/some/location/path";
+ requestBody.location = "someLocation";
requestBody.backupId = 123;
- requestBody.repository = "someRepositoryName";
+ requestBody.repository = "someRepository";
requestBody.async = "someAsyncId";
- final var remoteMessage =
- restoreCollectionAPI.createRemoteMessage("someBackupName",
requestBody).getProperties();
+
when(mockClusterState.hasCollection(eq("someCollectionName"))).thenReturn(true);
+ api.restoreCollection("someBackupName", requestBody);
+ verify(mockCommandRunner)
+ .runCollectionCommand(contextCapturer.capture(),
messageCapturer.capture(), anyLong());
+
+ final ZkNodeProps createdMessage = messageCapturer.getValue();
+ final Map<String, Object> remoteMessage = createdMessage.getProperties();
- assertEquals(7, remoteMessage.size());
- assertEquals("restore", remoteMessage.get(QUEUE_OPERATION));
+ assertEquals(5, remoteMessage.size());
assertEquals("someCollectionName", remoteMessage.get(COLLECTION));
- assertEquals("/some/location/path", remoteMessage.get(BACKUP_LOCATION));
+ assertEquals("someLocation", remoteMessage.get(BACKUP_LOCATION));
assertEquals(Integer.valueOf(123), remoteMessage.get(BACKUP_ID));
- assertEquals("someRepositoryName", remoteMessage.get(BACKUP_REPOSITORY));
- assertEquals("someAsyncId", remoteMessage.get(ASYNC));
+ assertEquals("someRepository", remoteMessage.get(BACKUP_REPOSITORY));
assertEquals("someBackupName", remoteMessage.get(NAME));
+
+ AdminCmdContext context = contextCapturer.getValue();
Review Comment:
[0] Most (all?) of these mock-based v2 API tests have some version of this
little snippet where we're grabbing the `AdminCmdContext` out of a capturer and
asserting the action and async ID. Just flagging it as there might be a way to
pull it into a helper method in MockAPITest.
##########
solr/core/src/java/org/apache/solr/handler/admin/api/CreateCollection.java:
##########
@@ -109,26 +105,12 @@ public SubResponseAccumulatingJerseyResponse
createCollection(
// Populate any 'null' creation parameters that support COLLECTIONPROP
defaults.
populateDefaultsIfNecessary(coreContainer, requestBody);
- final ZkNodeProps remoteMessage = createRemoteMessage(requestBody);
final SolrResponse remoteResponse =
- CollectionsHandler.submitCollectionApiCommand(
- coreContainer.getZkController(),
- remoteMessage,
+ submitRemoteMessageAndHandleResponse(
+ response,
CollectionParams.CollectionAction.CREATE,
- DEFAULT_COLLECTION_OP_TIMEOUT);
- if (remoteResponse.getException() != null) {
- throw remoteResponse.getException();
- }
-
- if (requestBody.async != null) {
Review Comment:
[+1] Such a huge improvement that each individual API doesn't need to check
for or copy over these fields anymore!
##########
solr/solrj/src/java/org/apache/solr/client/solrj/response/RequestStatusState.java:
##########
@@ -54,6 +56,11 @@ public String getKey() {
return key;
}
+ /** Returns whether the request status should be refreshed given the current
state. */
+ public boolean shouldRefresh() {
Review Comment:
[0] Idk if "refresh" means anything to most users. Maybe
`isOperationFinished` or something similar? Or maybe ignore me and "refresh"
is the best option as anything else would risk causing confusion with the
"COMPLETED" enum value.
Ignore me, just thinking aloud I guess...
##########
solr/core/src/test/org/apache/solr/handler/admin/api/BalanceShardUniqueAPITest.java:
##########
@@ -68,32 +74,35 @@ public void testReportsErrorIfPropertyToBalanceIsMissing() {
final var requestBody = new BalanceShardUniqueRequestBody();
final SolrException thrown =
expectThrows(
- SolrException.class,
- () -> {
- final var api = new BalanceShardUnique(null, null, null);
- api.balanceShardUnique("someCollName", requestBody);
- });
+ SolrException.class, () -> api.balanceShardUnique("someCollName",
requestBody));
assertEquals(400, thrown.code());
assertEquals("Missing required parameter: property", thrown.getMessage());
}
@Test
- public void testCreateRemoteMessageAllProperties() {
+ public void testCreateRemoteMessageAllProperties() throws Exception {
Review Comment:
[Q] Does this test name make sense, now that the test no longer calls
"BalanceShardUnique.createRemoteMessage"?
##########
solr/core/src/java/org/apache/solr/handler/admin/api/AliasProperty.java:
##########
@@ -128,63 +124,60 @@ public SolrJerseyResponse createOrUpdateAliasProperty(
recordCollectionForLogAndTracing(null, solrQueryRequest);
- SolrJerseyResponse response =
instantiateJerseyResponse(SolrJerseyResponse.class);
- modifyAliasProperty(aliasName, propName, requestBody.value);
+ var response =
instantiateJerseyResponse(SubResponseAccumulatingJerseyResponse.class);
+ modifyAliasProperty(response, aliasName, propName, requestBody.value);
return response;
}
@Override
@PermissionName(COLL_EDIT_PERM)
- public SolrJerseyResponse deleteAliasProperty(String aliasName, String
propName)
- throws Exception {
+ public SubResponseAccumulatingJerseyResponse deleteAliasProperty(
Review Comment:
[-0] All of the "update" methods in this class shouldn't return
SubResponseAccumulatingJerseyResponse, afaict, because the API response never
uses the "successes" or "failures" fields that are the main reason for
SubResponse... to exist.
If we aim of this change is to support async alias-prop modification, we can
probably use AsyncJerseyResponse instead?
##########
solr/core/src/java/org/apache/solr/handler/admin/api/DeleteAlias.java:
##########
@@ -49,34 +42,16 @@ public DeleteAlias(
@Override
@PermissionName(COLL_EDIT_PERM)
- public SolrJerseyResponse deleteAlias(String aliasName, String asyncId)
throws Exception {
- final AsyncJerseyResponse response =
instantiateJerseyResponse(AsyncJerseyResponse.class);
- final CoreContainer coreContainer =
fetchAndValidateZooKeeperAwareCoreContainer();
-
- final ZkNodeProps remoteMessage = createRemoteMessage(aliasName, asyncId);
- final SolrResponse remoteResponse =
- CollectionsHandler.submitCollectionApiCommand(
- coreContainer.getZkController(),
- remoteMessage,
- CollectionParams.CollectionAction.DELETEALIAS,
- DEFAULT_COLLECTION_OP_TIMEOUT);
- if (remoteResponse.getException() != null) {
- throw remoteResponse.getException();
- }
-
- if (asyncId != null) {
- response.requestId = asyncId;
- }
-
+ public SubResponseAccumulatingJerseyResponse deleteAlias(String aliasName,
String asyncId)
Review Comment:
[-0] Another place, afaict, where the original "SolrJerseyResponse" better
reflects the actual response content that we expect.
##########
solr/core/src/test/org/apache/solr/handler/admin/api/MockAPITest.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.admin.api;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import io.opentelemetry.api.trace.Span;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.ZkController;
+import org.apache.solr.cloud.api.collections.AdminCmdContext;
+import
org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.cloud.Aliases;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.common.cloud.SolrZkClient;
+import org.apache.solr.common.cloud.ZkNodeProps;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.mockito.ArgumentCaptor;
+
+/**
+ * Abstract test class to setup shared mocks for unit testing v2 API calls
that go to the Overseer
+ * or the DistributedCollectionConfigSetCommandRunner.
+ */
+public abstract class MockAPITest extends SolrTestCaseJ4 {
Review Comment:
[Q] Should the test name have something in it to indicate it's "v2"-ness,
and that it's not just for any old API you want to mock?
##########
solr/core/src/test/org/apache/solr/handler/admin/api/CreateAliasAPITest.java:
##########
@@ -325,7 +358,7 @@ public void
testConvertsV1ParamsForMultiDimensionalAliasToV2RequestBody() {
v1Params.add("name", "someAliasName");
v1Params.add("router.name", "Dimensional[time,category]");
v1Params.add("router.0.field", "someField");
- v1Params.add("router.0.start", "NOW");
+ v1Params.add("router.0.start", "NOW/HOUR");
Review Comment:
[Q] Just curious: any particular reason why this changed?
##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteNodeAPITest.java:
##########
@@ -16,59 +16,171 @@
*/
package org.apache.solr.handler.admin.api;
-import static org.mockito.Mockito.mock;
-
-import java.util.Map;
-import org.apache.solr.SolrTestCaseJ4;
-import org.apache.solr.client.api.model.DeleteNodeRequestBody;
-import org.apache.solr.common.SolrException;
-import org.apache.solr.common.cloud.ZkNodeProps;
-import org.apache.solr.common.params.ModifiableSolrParams;
+import java.lang.invoke.MethodHandles;
+import java.time.Duration;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import org.apache.solr.client.api.model.SubResponseAccumulatingJerseyResponse;
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.NodeApi;
+import org.apache.solr.client.solrj.response.RequestStatusState;
+import org.apache.solr.cloud.SolrCloudTestCase;
+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.StrUtils;
+import org.junit.After;
import org.junit.BeforeClass;
import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
-/** Unit tests for {@link DeleteNode} */
-public class DeleteNodeAPITest extends SolrTestCaseJ4 {
+/** Integration tests for {@link DeleteNode} using the V2 API */
+public class DeleteNodeAPITest extends SolrCloudTestCase {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
@BeforeClass
- public static void ensureWorkingMockito() {
- assumeWorkingMockito();
+ public static void setupCluster() throws Exception {
+ configureCluster(6)
+ .addConfig(
+ "conf1",
TEST_PATH().resolve("configsets").resolve("cloud-dynamic").resolve("conf"))
+ .configure();
}
- @Test
- public void testV1InvocationThrowsErrorsIfRequiredParametersMissing() {
- final var api = mock(DeleteNode.class);
- final SolrException e =
- expectThrows(
- SolrException.class,
- () -> {
- DeleteNode.invokeUsingV1Inputs(api, new ModifiableSolrParams());
- });
- assertEquals("Missing required parameter: node", e.getMessage());
+ @After
+ public void clearCollections() throws Exception {
+ cluster.deleteAllCollections();
}
@Test
- public void testValidOverseerMessageIsCreated() {
- final var requestBody = new DeleteNodeRequestBody();
- requestBody.async = "async";
- final ZkNodeProps createdMessage =
- DeleteNode.createRemoteMessage("nodeNameToDelete", requestBody);
- final Map<String, Object> createdMessageProps =
createdMessage.getProperties();
- assertEquals(3, createdMessageProps.size());
- assertEquals("nodeNameToDelete", createdMessageProps.get("node"));
- assertEquals("async", createdMessageProps.get("async"));
- assertEquals("deletenode", createdMessageProps.get("operation"));
+ public void testDeleteNode() throws Exception {
+ CloudSolrClient cloudClient = cluster.getSolrClient();
+ String coll = "deletenodetest_coll";
+ Set<String> liveNodes =
cloudClient.getClusterStateProvider().getLiveNodes();
+ ArrayList<String> l = new ArrayList<>(liveNodes);
+ Collections.shuffle(l, random());
+ CollectionAdminRequest.Create create =
+ pickRandom(
Review Comment:
[+1] This is a cool little pattern 👍
##########
solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java:
##########
@@ -415,6 +418,18 @@ protected static CoreStatusResponse.SingleCoreData
getCoreStatus(Replica replica
}
}
+ protected CollectionAdminRequest.RequestStatusResponse
waitForAsyncClusterRequest(
Review Comment:
[0] Is there anything SolrCloudTestCase-specific about this utility? Not
all of our SolrCloud tests use SolrCloudTestCase, so it might be nice to put it
in some place where it can be used by any SolrCloud-relevant test 🤷
##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteCollectionBackupAPITest.java:
##########
@@ -117,39 +136,63 @@ public void
testMultiVersionDeletionReportsErrorIfRetainParamMissing() {
// The message created in this test isn't valid in practice, since it
contains mutually-exclusive
// parameters, but that doesn't matter for the purposes of this test.
@Test
- public void testCreateRemoteMessageAllParams() {
- final var remoteMessage =
- DeleteCollectionBackup.createRemoteMessage(
- "someBackupName",
- "someBackupId",
- 123,
- true,
- "someLocation",
- "someRepository",
- "someAsyncId")
- .getProperties();
-
- assertEquals(8, remoteMessage.size());
- assertEquals("deletebackup", remoteMessage.get(QUEUE_OPERATION));
+ public void testCreateRemoteMessageSingle() throws Exception {
+ api.deleteSingleBackupById(
+ "someBackupName", "someBackupId", "someLocation", "someRepository",
"someAsyncId");
+ verify(mockCommandRunner)
+ .runCollectionCommand(contextCapturer.capture(),
messageCapturer.capture(), anyLong());
+
+ final ZkNodeProps createdMessage = messageCapturer.getValue();
+ final Map<String, Object> remoteMessage = createdMessage.getProperties();
+ assertEquals(4, remoteMessage.size());
assertEquals("someBackupName", remoteMessage.get(NAME));
assertEquals("someBackupId", remoteMessage.get(BACKUP_ID));
- assertEquals(Integer.valueOf(123),
remoteMessage.get(MAX_NUM_BACKUP_POINTS));
- assertEquals(Boolean.TRUE, remoteMessage.get(BACKUP_PURGE_UNUSED));
assertEquals("someLocation", remoteMessage.get(BACKUP_LOCATION));
assertEquals("someRepository", remoteMessage.get(BACKUP_REPOSITORY));
- assertEquals("someAsyncId", remoteMessage.get(ASYNC));
+
+ final AdminCmdContext context = contextCapturer.getValue();
+ assertEquals(CollectionParams.CollectionAction.DELETEBACKUP,
context.getAction());
+ assertEquals("someAsyncId", context.getAsyncId());
}
@Test
- public void testCreateRemoteMessageOnlyRequiredParams() {
- final var remoteMessage =
- DeleteCollectionBackup.createRemoteMessage(
- "someBackupName", "someBackupId", null, null, null, null, null)
- .getProperties();
-
- assertEquals(3, remoteMessage.size());
- assertEquals("deletebackup", remoteMessage.get(QUEUE_OPERATION));
+ public void testCreateRemoteMessageMultiple() throws Exception {
+ api.deleteMultipleBackupsByRecency("someBackupName", 2, "someLocation",
"someRepository", null);
+ verify(mockCommandRunner)
+ .runCollectionCommand(contextCapturer.capture(),
messageCapturer.capture(), anyLong());
+
+ final ZkNodeProps createdMessage = messageCapturer.getValue();
+ final Map<String, Object> remoteMessage = createdMessage.getProperties();
+ assertEquals(4, remoteMessage.size());
assertEquals("someBackupName", remoteMessage.get(NAME));
- assertEquals("someBackupId", remoteMessage.get(BACKUP_ID));
+ assertEquals(2, remoteMessage.get(MAX_NUM_BACKUP_POINTS));
+ assertEquals("someLocation", remoteMessage.get(BACKUP_LOCATION));
+ assertEquals("someRepository", remoteMessage.get(BACKUP_REPOSITORY));
+
+ final AdminCmdContext context = contextCapturer.getValue();
+ assertEquals(CollectionParams.CollectionAction.DELETEBACKUP,
context.getAction());
+ assertNull(context.getAsyncId());
+ }
+
+ @Test
+ public void testCreateRemoteMessageGarbageCollect() throws Exception {
Review Comment:
[+1] Thanks for improving the coverage here!
--
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]
