[GitHub] [solr] gerlowskija opened a new pull request, #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

gerlowskija opened a new pull request, #1053:
URL: https://github.com/apache/solr/pull/1053

   # Description
   
   The Solr community is engaged in an ongoing effort to make Solr's v2 API more REST-ful and intuitive for users so that it can eventually serve as a viable v1 replacement. This PR in particular focuses on Solr's "delete-replica-prop" API, aligning it with the API design described [here](https://docs.google.com/spreadsheets/d/1HAoBBFPpSiT8mJmgNZKkZAPwfCfPvlc08m5jz3fQBpA/edit?usp=sharing).
   
   In particular this changes the v2 API from:
   
   ```
   POST /collections/collName {"delete-replica-property" : {...}}
   ```
   
   to:
   
   ```
   DELETE /collections/<coll>/shards/<shard>/replicas/<replica>/properties/<prop>
   ```
   
   This is a breaking change for users of the v2 API, but one that is allowed due to v2's "experimental" designation.
   
   # Solution
   
   This PR modifies delete-replica-prop in the way described, but it also refactors the API class to use the newly added JAX-RS framework in the process. This framework-switchover, while conceptually separate from the API realignment in this case, makes sense as both tasks were coming in the short term and touch identical chunks of code.
   
   As a result, this PR not only changes the cosmetics of the delete-replica-prop endpoint, it also adds it to Solr's growing OpenAPI spec.
   
   # Tests
   
   See DeleteReplicaPropertyAPITest for new unit tests.
   
   # Checklist
   
   Please review the following and check all that apply:
   
   - [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
   - [x] I have created a Jira issue and added the issue ID to my pull request title.
   - [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
   - [x] I have developed this patch against the `main` branch.
   - [ ] I have run `./gradlew check`.
   - [x] I have added tests for my changes.
   - [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
   


-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] gerlowskija commented on pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

gerlowskija commented on PR #1053:
URL: https://github.com/apache/solr/pull/1053#issuecomment-1310750866

   Alright, the tests on this should finally be ready.  Will plan to merge on Monday if there's no remaining objections!


-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] epugh commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

epugh commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1010962959


##########
solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java:
##########
@@ -1298,19 +1298,25 @@ public Map<String, Object> execute(
           V2ApiUtils.squashIntoSolrResponseWithoutHeader(rsp, addReplicaPropResponse);
           return null;
         }),
-    // XXX should this command support followAliases?
     DELETEREPLICAPROP_OP(
         DELETEREPLICAPROP,
         (req, rsp, h) -> {
-          Map<String, Object> map =
-              copy(
-                  req.getParams().required(),
-                  null,
-                  COLLECTION_PROP,
-                  PROPERTY_PROP,
-                  SHARD_ID_PROP,
-                  REPLICA_PROP);
-          return copy(req.getParams(), map, PROPERTY_PROP);
+          final RequiredSolrParams requiredParams = req.getParams().required();
+          final String propNameToDelete = requiredParams.get(PROPERTY_PROP);
+          final String trimmedPropNameToDelete =
+              propNameToDelete.startsWith(PROPERTY_PREFIX)
+                  ? propNameToDelete.substring(PROPERTY_PREFIX.length())
+                  : propNameToDelete;
+          final DeleteReplicaPropertyAPI deleteReplicaPropertyAPI =

Review Comment:
   I don't think I'm the person to introduce new Java constructs to the Solr code base ;-)  I think if the community is up for it, great...   I might advocate that we do a big sweep through so that all the places `var` makes sense, we put that in, so it's not "a bit one way here, a bit the other way there"  And maybe we even have a way of checking 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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] dsmiley commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

dsmiley commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1014014058


##########
solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java:
##########
@@ -1298,19 +1298,25 @@ public Map<String, Object> execute(
           V2ApiUtils.squashIntoSolrResponseWithoutHeader(rsp, addReplicaPropResponse);
           return null;
         }),
-    // XXX should this command support followAliases?
     DELETEREPLICAPROP_OP(
         DELETEREPLICAPROP,
         (req, rsp, h) -> {
-          Map<String, Object> map =
-              copy(
-                  req.getParams().required(),
-                  null,
-                  COLLECTION_PROP,
-                  PROPERTY_PROP,
-                  SHARD_ID_PROP,
-                  REPLICA_PROP);
-          return copy(req.getParams(), map, PROPERTY_PROP);
+          final RequiredSolrParams requiredParams = req.getParams().required();
+          final String propNameToDelete = requiredParams.get(PROPERTY_PROP);
+          final String trimmedPropNameToDelete =
+              propNameToDelete.startsWith(PROPERTY_PREFIX)
+                  ? propNameToDelete.substring(PROPERTY_PREFIX.length())
+                  : propNameToDelete;
+          final DeleteReplicaPropertyAPI deleteReplicaPropertyAPI =

Review Comment:
   We already have code using "var" albeit not much.  Nobody needed permission :-). It was explicitly forbidden by us by technical controls (validate source patterns gradle) but once 9.0 was released (or soon previously?), this control was intentionally lifted so we can use Java 11 stuff.  For some time, we didn't want to use Java 11 stuff on main (which became 9) to ease back ports to 8x.  If hypothetically we wanted to enable Java 17 on main today, we _might_ similarly want to avoid constructs that would make back-ports harder.



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] gerlowskija commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

gerlowskija commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1016747436


##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPITest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.apache.solr.cloud.api.collections.CollectionHandlingUtils.SHARD_UNIQUE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkNodeProps;
+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.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for {@link DeleteReplicaPropertyAPI} */
+public class DeleteReplicaPropertyAPITest extends SolrTestCaseJ4 {
+
+  private CoreContainer mockCoreContainer;
+  private DistributedCollectionConfigSetCommandRunner mockCommandRunner;
+  private SolrQueryRequest mockQueryRequest;
+  private SolrQueryResponse queryResponse;
+  private ArgumentCaptor<ZkNodeProps> messageCapturer;
+
+  private DeleteReplicaPropertyAPI deleteReplicaPropApi;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    mockCoreContainer = mock(CoreContainer.class);
+    mockCommandRunner = mock(DistributedCollectionConfigSetCommandRunner.class);
+    when(mockCoreContainer.getDistributedCollectionCommandRunner())
+        .thenReturn(Optional.of(mockCommandRunner));
+    when(mockCommandRunner.runCollectionCommand(any(), any(), anyLong()))
+        .thenReturn(new OverseerSolrResponse(new NamedList<>()));
+    mockQueryRequest = mock(SolrQueryRequest.class);
+    when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);
+    queryResponse = new SolrQueryResponse();
+    messageCapturer = ArgumentCaptor.forClass(ZkNodeProps.class);
+
+    deleteReplicaPropApi =
+        new DeleteReplicaPropertyAPI(mockCoreContainer, mockQueryRequest, queryResponse);
+  }
+
+  @Test
+  public void testReportsErrorWhenCalledInStandaloneMode() {
+    when(mockCoreContainer.isZooKeeperAware()).thenReturn(false);
+
+    final SolrException e =
+        expectThrows(
+            SolrException.class,
+            () -> {
+              deleteReplicaPropApi.deleteReplicaProperty(
+                  "someColl", "someShard", "someReplica", "somePropName");
+            });
+    assertEquals(400, e.code());
+    assertTrue(
+        "Exception message differed from expected: " + e.getMessage(),
+        e.getMessage().contains("not running in SolrCloud mode"));
+  }
+
+  @Test
+  public void testCreatesValidOverseerMessage() throws Exception {

Review Comment:
   OK, I don't super agree but I feel like I get the distinction.  I appreciate you taking the time to explain where you're coming from on this.  I know it's a lot of talk for a small thing, but I'm hoping being thorough here will save time on reviews going forward.
   
   I'll try this as a compromise:
   
   - I still feel like the message-construction code is important to unit test for now, because it's where the bulk of the "API" logic is as the code stands today.  But it doesn't have to mention the overseer or even live in the API class itself. 
    I'll refactor the message-creation code out, and that'll let us unit test it without mocks **and** it'll separate the tests from the API class itself so that they can just be nuked if we move away from the overseer (rather than playing some "cementing" role).
   - We can refactor the v1 code that calls into v2 in such a way that it's testable (maybe using mocks, but hopefully not?) to ensure that the v1 codepath uses the v2 API class properly.
   
   Is that something you could live with? @dsmiley 



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] epugh commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

epugh commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1019510916


##########
solr/core/src/java/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPI.java:
##########
@@ -17,49 +17,113 @@
 
 package org.apache.solr.handler.admin.api;
 
-import static org.apache.solr.client.solrj.SolrRequest.METHOD.POST;
-import static org.apache.solr.common.params.CollectionAdminParams.COLLECTION;
-import static org.apache.solr.common.params.CommonParams.ACTION;
-import static org.apache.solr.handler.ClusterAPI.wrapParams;
-import static org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM;
+import static org.apache.solr.cloud.Overseer.QUEUE_OPERATION;
+import static org.apache.solr.common.cloud.ZkStateReader.COLLECTION_PROP;
+import static org.apache.solr.common.cloud.ZkStateReader.PROPERTY_PROP;
+import static org.apache.solr.common.cloud.ZkStateReader.REPLICA_PROP;
+import static org.apache.solr.common.cloud.ZkStateReader.SHARD_ID_PROP;
+import static org.apache.solr.common.params.CollectionAdminParams.PROPERTY_PREFIX;
+import static org.apache.solr.handler.admin.CollectionsHandler.DEFAULT_COLLECTION_OP_TIMEOUT;
 
-import java.util.HashMap;
+import io.swagger.v3.oas.annotations.Parameter;
 import java.util.Map;
-import org.apache.solr.api.Command;
-import org.apache.solr.api.EndPoint;
-import org.apache.solr.api.PayloadObj;
-import org.apache.solr.client.solrj.request.beans.DeleteReplicaPropertyPayload;
+import javax.inject.Inject;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import org.apache.solr.client.solrj.SolrResponse;
+import org.apache.solr.common.cloud.ZkNodeProps;
 import org.apache.solr.common.params.CollectionParams;
+import org.apache.solr.common.params.RequiredSolrParams;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.core.CoreContainer;
 import org.apache.solr.handler.admin.CollectionsHandler;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
 
 /**
  * V2 API for removing a property from a collection replica
  *
- * <p>This API (POST /v2/collections/collectionName {'delete-replica-property': {...}}) is analogous
- * to the v1 /admin/collections?action=DELETEREPLICAPROP command.
- *
- * @see DeleteReplicaPropertyPayload
+ * <p>This API is analogous to the v1 /admin/collections?action=DELETEREPLICAPROP command.
  */
-@EndPoint(
-    path = {"/c/{collection}", "/collections/{collection}"},
-    method = POST,
-    permission = COLL_EDIT_PERM)
-public class DeleteReplicaPropertyAPI {
-  private static final String V2_DELETE_REPLICA_PROPERTY_CMD = "delete-replica-property";
+@Path("/collections/{collName}/shards/{shardName}/replicas/{replicaName}/properties/{propName}")
+public class DeleteReplicaPropertyAPI extends AdminAPIBase {
+
+  @Inject
+  public DeleteReplicaPropertyAPI(
+      CoreContainer coreContainer,
+      SolrQueryRequest solrQueryRequest,
+      SolrQueryResponse solrQueryResponse) {
+    super(coreContainer, solrQueryRequest, solrQueryResponse);
+  }
 
-  private final CollectionsHandler collectionsHandler;
+  public SolrJerseyResponse deleteReplicaProperty(
+      @Parameter(
+              description = "The name of the collection the replica belongs to.",
+              required = true)
+          @PathParam("collName")
+          String collName,
+      @Parameter(description = "The name of the shard the replica belongs to.", required = true)
+          @PathParam("shardName")
+          String shardName,
+      @Parameter(description = "The replica, e.g., `core_node1`.", required = true)
+          @PathParam("replicaName")
+          String replicaName,
+      @Parameter(description = "The name of the property to delete.", required = true)
+          @PathParam("propName")

Review Comment:
   `propertyName` ?   I know, it's four more characters...



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] gerlowskija merged pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

gerlowskija merged PR #1053:
URL: https://github.com/apache/solr/pull/1053


-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] dsmiley commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

dsmiley commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1018294106


##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPITest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.apache.solr.cloud.api.collections.CollectionHandlingUtils.SHARD_UNIQUE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkNodeProps;
+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.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for {@link DeleteReplicaPropertyAPI} */
+public class DeleteReplicaPropertyAPITest extends SolrTestCaseJ4 {
+
+  private CoreContainer mockCoreContainer;
+  private DistributedCollectionConfigSetCommandRunner mockCommandRunner;
+  private SolrQueryRequest mockQueryRequest;
+  private SolrQueryResponse queryResponse;
+  private ArgumentCaptor<ZkNodeProps> messageCapturer;
+
+  private DeleteReplicaPropertyAPI deleteReplicaPropApi;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    mockCoreContainer = mock(CoreContainer.class);
+    mockCommandRunner = mock(DistributedCollectionConfigSetCommandRunner.class);
+    when(mockCoreContainer.getDistributedCollectionCommandRunner())
+        .thenReturn(Optional.of(mockCommandRunner));
+    when(mockCommandRunner.runCollectionCommand(any(), any(), anyLong()))
+        .thenReturn(new OverseerSolrResponse(new NamedList<>()));
+    mockQueryRequest = mock(SolrQueryRequest.class);
+    when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);
+    queryResponse = new SolrQueryResponse();
+    messageCapturer = ArgumentCaptor.forClass(ZkNodeProps.class);
+
+    deleteReplicaPropApi =
+        new DeleteReplicaPropertyAPI(mockCoreContainer, mockQueryRequest, queryResponse);
+  }
+
+  @Test
+  public void testReportsErrorWhenCalledInStandaloneMode() {
+    when(mockCoreContainer.isZooKeeperAware()).thenReturn(false);
+
+    final SolrException e =
+        expectThrows(
+            SolrException.class,
+            () -> {
+              deleteReplicaPropApi.deleteReplicaProperty(
+                  "someColl", "someShard", "someReplica", "somePropName");
+            });
+    assertEquals(400, e.code());
+    assertTrue(
+        "Exception message differed from expected: " + e.getMessage(),
+        e.getMessage().contains("not running in SolrCloud mode"));
+  }
+
+  @Test
+  public void testCreatesValidOverseerMessage() throws Exception {

Review Comment:
   That's a reasonable compromise; thanks.



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] gerlowskija commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

gerlowskija commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1006130505


##########
solr/core/src/test/org/apache/solr/handler/admin/TestCollectionAPIs.java:
##########
@@ -199,13 +199,6 @@ public void testCommands() throws Exception {
     //        "Value of enum must be one of"
     //    );
 
-    compareOutput(

Review Comment:
   Because TestCollectionAPIs only works with the "traditional" v2 framework.  This commit moves delete-replica-prop over to the JAX-RS framework, so it can't be covered anymore by TestCollectionAPIs.  You could think of it as the assertion here moving over to the DeleteReplicaPropertyAPITest class added in this 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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] gerlowskija commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

gerlowskija commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1013243883


##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPITest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.apache.solr.cloud.api.collections.CollectionHandlingUtils.SHARD_UNIQUE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkNodeProps;
+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.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for {@link DeleteReplicaPropertyAPI} */
+public class DeleteReplicaPropertyAPITest extends SolrTestCaseJ4 {
+
+  private CoreContainer mockCoreContainer;
+  private DistributedCollectionConfigSetCommandRunner mockCommandRunner;
+  private SolrQueryRequest mockQueryRequest;
+  private SolrQueryResponse queryResponse;
+  private ArgumentCaptor<ZkNodeProps> messageCapturer;
+
+  private DeleteReplicaPropertyAPI deleteReplicaPropApi;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    mockCoreContainer = mock(CoreContainer.class);
+    mockCommandRunner = mock(DistributedCollectionConfigSetCommandRunner.class);
+    when(mockCoreContainer.getDistributedCollectionCommandRunner())
+        .thenReturn(Optional.of(mockCommandRunner));
+    when(mockCommandRunner.runCollectionCommand(any(), any(), anyLong()))
+        .thenReturn(new OverseerSolrResponse(new NamedList<>()));
+    mockQueryRequest = mock(SolrQueryRequest.class);
+    when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);
+    queryResponse = new SolrQueryResponse();
+    messageCapturer = ArgumentCaptor.forClass(ZkNodeProps.class);
+
+    deleteReplicaPropApi =
+        new DeleteReplicaPropertyAPI(mockCoreContainer, mockQueryRequest, queryResponse);
+  }
+
+  @Test
+  public void testReportsErrorWhenCalledInStandaloneMode() {
+    when(mockCoreContainer.isZooKeeperAware()).thenReturn(false);
+
+    final SolrException e =
+        expectThrows(
+            SolrException.class,
+            () -> {
+              deleteReplicaPropApi.deleteReplicaProperty(
+                  "someColl", "someShard", "someReplica", "somePropName");
+            });
+    assertEquals(400, e.code());
+    assertTrue(
+        "Exception message differed from expected: " + e.getMessage(),
+        e.getMessage().contains("not running in SolrCloud mode"));
+  }
+
+  @Test
+  public void testCreatesValidOverseerMessage() throws Exception {

Review Comment:
   > a unit test that only tests the V1 to V2 mapping without implementation details of how V2 does its job
   
   We can absolutely do that, and if you want me to I can.  But I still feel like I'm missing some nuance here.  Isn't CollectionHandler's use of DeleteReplicaPropertyAPI under-the-hood just as much of an "implementation detail" as DeleteReplicaPropertyAPI's use of the overseer?  Why is one OK to test but not the other?  If anything I'd consider the overseer as less of an internal detail as it's doing message passing to some other system!
   
   Ideally I'd write both of those tests: one that validates CollectionHandler's usage of DRPA, and one that validates DRPA's message-crafting.



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] dsmiley commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

dsmiley commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1010941513


##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPITest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.apache.solr.cloud.api.collections.CollectionHandlingUtils.SHARD_UNIQUE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkNodeProps;
+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.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for {@link DeleteReplicaPropertyAPI} */
+public class DeleteReplicaPropertyAPITest extends SolrTestCaseJ4 {
+
+  private CoreContainer mockCoreContainer;
+  private DistributedCollectionConfigSetCommandRunner mockCommandRunner;
+  private SolrQueryRequest mockQueryRequest;
+  private SolrQueryResponse queryResponse;
+  private ArgumentCaptor<ZkNodeProps> messageCapturer;
+
+  private DeleteReplicaPropertyAPI deleteReplicaPropApi;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    mockCoreContainer = mock(CoreContainer.class);
+    mockCommandRunner = mock(DistributedCollectionConfigSetCommandRunner.class);
+    when(mockCoreContainer.getDistributedCollectionCommandRunner())
+        .thenReturn(Optional.of(mockCommandRunner));
+    when(mockCommandRunner.runCollectionCommand(any(), any(), anyLong()))
+        .thenReturn(new OverseerSolrResponse(new NamedList<>()));
+    mockQueryRequest = mock(SolrQueryRequest.class);
+    when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);
+    queryResponse = new SolrQueryResponse();
+    messageCapturer = ArgumentCaptor.forClass(ZkNodeProps.class);
+
+    deleteReplicaPropApi =
+        new DeleteReplicaPropertyAPI(mockCoreContainer, mockQueryRequest, queryResponse);
+  }
+
+  @Test
+  public void testReportsErrorWhenCalledInStandaloneMode() {
+    when(mockCoreContainer.isZooKeeperAware()).thenReturn(false);
+
+    final SolrException e =
+        expectThrows(
+            SolrException.class,
+            () -> {
+              deleteReplicaPropApi.deleteReplicaProperty(
+                  "someColl", "someShard", "someReplica", "somePropName");
+            });
+    assertEquals(400, e.code());
+    assertTrue(
+        "Exception message differed from expected: " + e.getMessage(),
+        e.getMessage().contains("not running in SolrCloud mode"));
+  }
+
+  @Test
+  public void testCreatesValidOverseerMessage() throws Exception {

Review Comment:
   > Yep, that's exactly what this PR does! 
   
   Can you substantiate that please?  I'm talking about a unit test that only tests the V1 to V2 mapping without implementation details of how V2 does its job.  I could imagine mocking (yes me, endorsing mocking) to capture what parameters are passed to DeleteReplicaPropertyAPI.deleteReplicaProperty() to ensure they align with a V1 call.  Mention of the Overseer would be an unwanted implementation detail.



##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPITest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.apache.solr.cloud.api.collections.CollectionHandlingUtils.SHARD_UNIQUE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkNodeProps;
+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.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for {@link DeleteReplicaPropertyAPI} */
+public class DeleteReplicaPropertyAPITest extends SolrTestCaseJ4 {
+
+  private CoreContainer mockCoreContainer;
+  private DistributedCollectionConfigSetCommandRunner mockCommandRunner;
+  private SolrQueryRequest mockQueryRequest;
+  private SolrQueryResponse queryResponse;
+  private ArgumentCaptor<ZkNodeProps> messageCapturer;
+
+  private DeleteReplicaPropertyAPI deleteReplicaPropApi;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    mockCoreContainer = mock(CoreContainer.class);
+    mockCommandRunner = mock(DistributedCollectionConfigSetCommandRunner.class);
+    when(mockCoreContainer.getDistributedCollectionCommandRunner())
+        .thenReturn(Optional.of(mockCommandRunner));
+    when(mockCommandRunner.runCollectionCommand(any(), any(), anyLong()))
+        .thenReturn(new OverseerSolrResponse(new NamedList<>()));
+    mockQueryRequest = mock(SolrQueryRequest.class);
+    when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);

Review Comment:
   > Mocking can be awkward for sure. You're not wrong. But IMO most of the time when mocking is ugly or when it has to reference obscure unrelated things - it's holding up a mirror to the design of the code-under-test.
   
   I agree.
   
   > e.g. We only have to muck about with NoopSpan here because the API logic itself in CollectionsHandler mucks about with spans (albeit indirectly, through o.a.s.util.tracing.TracingUtils). I'm sure there's good reason for it to be where it is, but If tracing stuff was off in a Jetty servlet filter (or where ever) this test probably would have no mention of it.
   
   I did much of Solr's tracing and can substantiate just about every detail.  Tracing at the servlet filter level would be generic; wouldn't have deep knowledge of Solr.  Consequently, its value/utility would be less.  If that's all we wanted out of tracing, we wouldn't even need special Solr code for it -- we could recommend users take off-the-shelf telemetry agents that can do this.  Solr has deep / integrated tracing.  Back to testing.... it's a shame that methods mocked return null even when the real implementation certainly wouldn't return null!  Maybe we could have a TestingCoreContainer?



##########
solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java:
##########
@@ -1298,19 +1298,25 @@ public Map<String, Object> execute(
           V2ApiUtils.squashIntoSolrResponseWithoutHeader(rsp, addReplicaPropResponse);
           return null;
         }),
-    // XXX should this command support followAliases?
     DELETEREPLICAPROP_OP(
         DELETEREPLICAPROP,
         (req, rsp, h) -> {
-          Map<String, Object> map =
-              copy(
-                  req.getParams().required(),
-                  null,
-                  COLLECTION_PROP,
-                  PROPERTY_PROP,
-                  SHARD_ID_PROP,
-                  REPLICA_PROP);
-          return copy(req.getParams(), map, PROPERTY_PROP);
+          final RequiredSolrParams requiredParams = req.getParams().required();
+          final String propNameToDelete = requiredParams.get(PROPERTY_PROP);
+          final String trimmedPropNameToDelete =
+              propNameToDelete.startsWith(PROPERTY_PREFIX)
+                  ? propNameToDelete.substring(PROPERTY_PREFIX.length())
+                  : propNameToDelete;
+          final DeleteReplicaPropertyAPI deleteReplicaPropertyAPI =

Review Comment:
   Don't mean to nit-pick, but I've been thinking of Java's new-ish `var` keyword and this line here is the poster-child for when `var` is appropriate.  The variable name `deleteReplicaPropertyAPI` is completely unambiguous.  The value is merely calling the constructor.  Thus saying `DeleteReplicaPropertyAPI` for the type is verbose for no benefit.  Java doesn't *have* to be verbose as much nowadays, even though it deservedly has had this reputation.  If I can't convince us to use `var` here, then we might as well forbid `var` :-)
   CC @epugh you've been thinking we need some bigger discussion on the dev list before we can use this Java language feature?  I can do so if you wish; I will certainly point to this very line in this PR if some discussion is necessary to get you to use it ;-)
   
   ditto for `RequiredSolrParams` above.
   



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] dsmiley commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

dsmiley commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r991437319


##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPITest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.apache.solr.cloud.api.collections.CollectionHandlingUtils.SHARD_UNIQUE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkNodeProps;
+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.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for {@link DeleteReplicaPropertyAPI} */
+public class DeleteReplicaPropertyAPITest extends SolrTestCaseJ4 {
+
+  private CoreContainer mockCoreContainer;
+  private DistributedCollectionConfigSetCommandRunner mockCommandRunner;
+  private SolrQueryRequest mockQueryRequest;
+  private SolrQueryResponse queryResponse;
+  private ArgumentCaptor<ZkNodeProps> messageCapturer;
+
+  private DeleteReplicaPropertyAPI deleteReplicaPropApi;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    mockCoreContainer = mock(CoreContainer.class);
+    mockCommandRunner = mock(DistributedCollectionConfigSetCommandRunner.class);
+    when(mockCoreContainer.getDistributedCollectionCommandRunner())
+        .thenReturn(Optional.of(mockCommandRunner));
+    when(mockCommandRunner.runCollectionCommand(any(), any(), anyLong()))
+        .thenReturn(new OverseerSolrResponse(new NamedList<>()));
+    mockQueryRequest = mock(SolrQueryRequest.class);
+    when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);
+    queryResponse = new SolrQueryResponse();
+    messageCapturer = ArgumentCaptor.forClass(ZkNodeProps.class);
+
+    deleteReplicaPropApi =
+        new DeleteReplicaPropertyAPI(mockCoreContainer, mockQueryRequest, queryResponse);
+  }
+
+  @Test
+  public void testReportsErrorWhenCalledInStandaloneMode() {
+    when(mockCoreContainer.isZooKeeperAware()).thenReturn(false);
+
+    final SolrException e =
+        expectThrows(
+            SolrException.class,
+            () -> {
+              deleteReplicaPropApi.deleteReplicaProperty(
+                  "someColl", "someShard", "someReplica", "somePropName");
+            });
+    assertEquals(400, e.code());
+    assertTrue(
+        "Exception message differed from expected: " + e.getMessage(),
+        e.getMessage().contains("not running in SolrCloud mode"));
+  }
+
+  @Test
+  public void testCreatesValidOverseerMessage() throws Exception {

Review Comment:
   For example, set the property and then get the property to see that we set it.  Don't test how it works.



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] gerlowskija commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

gerlowskija commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1006192774


##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPITest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.apache.solr.cloud.api.collections.CollectionHandlingUtils.SHARD_UNIQUE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkNodeProps;
+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.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for {@link DeleteReplicaPropertyAPI} */
+public class DeleteReplicaPropertyAPITest extends SolrTestCaseJ4 {
+
+  private CoreContainer mockCoreContainer;
+  private DistributedCollectionConfigSetCommandRunner mockCommandRunner;
+  private SolrQueryRequest mockQueryRequest;
+  private SolrQueryResponse queryResponse;
+  private ArgumentCaptor<ZkNodeProps> messageCapturer;
+
+  private DeleteReplicaPropertyAPI deleteReplicaPropApi;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    mockCoreContainer = mock(CoreContainer.class);
+    mockCommandRunner = mock(DistributedCollectionConfigSetCommandRunner.class);
+    when(mockCoreContainer.getDistributedCollectionCommandRunner())
+        .thenReturn(Optional.of(mockCommandRunner));
+    when(mockCommandRunner.runCollectionCommand(any(), any(), anyLong()))
+        .thenReturn(new OverseerSolrResponse(new NamedList<>()));
+    mockQueryRequest = mock(SolrQueryRequest.class);
+    when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);

Review Comment:
   Sorry for the delay replying here:
   
   Mocking can be awkward for sure.  You're not wrong.  But IMO most of the time when mocking is ugly or when it has to reference obscure unrelated things - it's holding up a mirror to the design of the code-under-test.
   
   e.g. We only have to muck about with NoopSpan here because the API logic itself in CollectionsHandler mucks about with spans (albeit indirectly, through `o.a.s.util.tracing.TracingUtils`).  I'm sure there's good reason for it to be where it is, but If tracing stuff was off in a Jetty servlet filter (or where ever) this test probably _would_ have no mention of it.
   
   I guess I'm not claiming that mocks are ideal, but I see them as a huge improvement (or at least, a badly needed supplement) to some of our other testing patterns.  We're going on a decade now of serious flakiness and speed concerns in our test suite!  And I think our communal aversion to anything that's _not_ an end-to-end test plays a big role there.  Spinning up full SolrCloud+ZK clusters is normal!  Our "light" option still spins up 1 or more full Jetty servers!  Talk about bringing in obscure, unrelated things!
   
   > If we need mocking, could we have test helpers/shims/subclasses such that the Mocking we need are pertinent to what it is being tested
   
   I can try abstracting some of this out into a helper or subclass, but my guess is that it might only have limited applicability.  v2 APIs that we pull out of CollectionsHandler will be able to use it, sure, but does the logic in e.g. ConfigSetsHandler interact with tracing in quite the same way?  Idk.
   
   Another alternative, if you still find the mocks repellent, would be to extract the "message creation" piece into a class that can be separately unit-tested without needing mocks.  That's essentially what these tests are validating, that the RPC message triggered by a given API call looks the way it should.  Or we could go a different route and do something that extends `JerseyTest`, though if I remember right you had other reasons for disliking that approach?
   
   So I guess there's 3 options there: (1) hide the mocks behind utils/parent-classes, (2) mockless unit testing on the message-creation logic only, or (3) something JerseyTest-based with hopefully fewer mocks.  Any of those jump out at you as preferable @dsmiley ?



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] gerlowskija commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

gerlowskija commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1016750781


##########
solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java:
##########
@@ -1298,19 +1298,25 @@ public Map<String, Object> execute(
           V2ApiUtils.squashIntoSolrResponseWithoutHeader(rsp, addReplicaPropResponse);
           return null;
         }),
-    // XXX should this command support followAliases?
     DELETEREPLICAPROP_OP(
         DELETEREPLICAPROP,
         (req, rsp, h) -> {
-          Map<String, Object> map =
-              copy(
-                  req.getParams().required(),
-                  null,
-                  COLLECTION_PROP,
-                  PROPERTY_PROP,
-                  SHARD_ID_PROP,
-                  REPLICA_PROP);
-          return copy(req.getParams(), map, PROPERTY_PROP);
+          final RequiredSolrParams requiredParams = req.getParams().required();
+          final String propNameToDelete = requiredParams.get(PROPERTY_PROP);
+          final String trimmedPropNameToDelete =
+              propNameToDelete.startsWith(PROPERTY_PREFIX)
+                  ? propNameToDelete.substring(PROPERTY_PREFIX.length())
+                  : propNameToDelete;
+          final DeleteReplicaPropertyAPI deleteReplicaPropertyAPI =

Review Comment:
   Ah, ok.  So the "ban" has already been lifted in a sense.  I'll give it a shot 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: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] gerlowskija commented on pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

gerlowskija commented on PR #1053:
URL: https://github.com/apache/solr/pull/1053#issuecomment-1310599921

   bq. Maybe the code in CollectionsHandler for a given V1 API should actually move to the *API class
   
   I don't totally love it, but it would have a really nice side-effect: it would make it much much easier to unit test the v1 code's invocation of the v2 code.  I'll give this a go here and see what it looks like.


-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] gerlowskija commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

gerlowskija commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1013268226


##########
solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java:
##########
@@ -1298,19 +1298,25 @@ public Map<String, Object> execute(
           V2ApiUtils.squashIntoSolrResponseWithoutHeader(rsp, addReplicaPropResponse);
           return null;
         }),
-    // XXX should this command support followAliases?
     DELETEREPLICAPROP_OP(
         DELETEREPLICAPROP,
         (req, rsp, h) -> {
-          Map<String, Object> map =
-              copy(
-                  req.getParams().required(),
-                  null,
-                  COLLECTION_PROP,
-                  PROPERTY_PROP,
-                  SHARD_ID_PROP,
-                  REPLICA_PROP);
-          return copy(req.getParams(), map, PROPERTY_PROP);
+          final RequiredSolrParams requiredParams = req.getParams().required();
+          final String propNameToDelete = requiredParams.get(PROPERTY_PROP);
+          final String trimmedPropNameToDelete =
+              propNameToDelete.startsWith(PROPERTY_PREFIX)
+                  ? propNameToDelete.substring(PROPERTY_PREFIX.length())
+                  : propNameToDelete;
+          final DeleteReplicaPropertyAPI deleteReplicaPropertyAPI =

Review Comment:
   I have no issue with `var`.  It's not in my muscle-memory so to speak, but no time like the present to start if there's community agreement.
   
   Do we specify a language-level in our gradle build or anything that'd prevent us from using `var` without build changes?



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] dsmiley commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

dsmiley commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r991422003


##########
solr/core/src/test/org/apache/solr/handler/admin/TestCollectionAPIs.java:
##########
@@ -199,13 +199,6 @@ public void testCommands() throws Exception {
     //        "Value of enum must be one of"
     //    );
 
-    compareOutput(

Review Comment:
   why is this test/comparison being deleted?



##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPITest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.apache.solr.cloud.api.collections.CollectionHandlingUtils.SHARD_UNIQUE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkNodeProps;
+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.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for {@link DeleteReplicaPropertyAPI} */
+public class DeleteReplicaPropertyAPITest extends SolrTestCaseJ4 {
+
+  private CoreContainer mockCoreContainer;
+  private DistributedCollectionConfigSetCommandRunner mockCommandRunner;
+  private SolrQueryRequest mockQueryRequest;
+  private SolrQueryResponse queryResponse;
+  private ArgumentCaptor<ZkNodeProps> messageCapturer;
+
+  private DeleteReplicaPropertyAPI deleteReplicaPropApi;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    mockCoreContainer = mock(CoreContainer.class);
+    mockCommandRunner = mock(DistributedCollectionConfigSetCommandRunner.class);
+    when(mockCoreContainer.getDistributedCollectionCommandRunner())
+        .thenReturn(Optional.of(mockCommandRunner));
+    when(mockCommandRunner.runCollectionCommand(any(), any(), anyLong()))
+        .thenReturn(new OverseerSolrResponse(new NamedList<>()));
+    mockQueryRequest = mock(SolrQueryRequest.class);
+    when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);
+    queryResponse = new SolrQueryResponse();
+    messageCapturer = ArgumentCaptor.forClass(ZkNodeProps.class);
+
+    deleteReplicaPropApi =
+        new DeleteReplicaPropertyAPI(mockCoreContainer, mockQueryRequest, queryResponse);
+  }
+
+  @Test
+  public void testReportsErrorWhenCalledInStandaloneMode() {
+    when(mockCoreContainer.isZooKeeperAware()).thenReturn(false);
+
+    final SolrException e =
+        expectThrows(
+            SolrException.class,
+            () -> {
+              deleteReplicaPropApi.deleteReplicaProperty(
+                  "someColl", "someShard", "someReplica", "somePropName");
+            });
+    assertEquals(400, e.code());
+    assertTrue(
+        "Exception message differed from expected: " + e.getMessage(),
+        e.getMessage().contains("not running in SolrCloud mode"));
+  }
+
+  @Test
+  public void testCreatesValidOverseerMessage() throws Exception {

Review Comment:
   Maybe this is too internal?  I like to think we don't need the Overseer for this -- remember distributed state updates.  FWIW I prefer that we test functionally and not internal details subject to change that make internal refactoring harder.



##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPITest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.apache.solr.cloud.api.collections.CollectionHandlingUtils.SHARD_UNIQUE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkNodeProps;
+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.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for {@link DeleteReplicaPropertyAPI} */
+public class DeleteReplicaPropertyAPITest extends SolrTestCaseJ4 {
+
+  private CoreContainer mockCoreContainer;
+  private DistributedCollectionConfigSetCommandRunner mockCommandRunner;
+  private SolrQueryRequest mockQueryRequest;
+  private SolrQueryResponse queryResponse;
+  private ArgumentCaptor<ZkNodeProps> messageCapturer;
+
+  private DeleteReplicaPropertyAPI deleteReplicaPropApi;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    mockCoreContainer = mock(CoreContainer.class);
+    mockCommandRunner = mock(DistributedCollectionConfigSetCommandRunner.class);
+    when(mockCoreContainer.getDistributedCollectionCommandRunner())
+        .thenReturn(Optional.of(mockCommandRunner));
+    when(mockCommandRunner.runCollectionCommand(any(), any(), anyLong()))
+        .thenReturn(new OverseerSolrResponse(new NamedList<>()));
+    mockQueryRequest = mock(SolrQueryRequest.class);
+    when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);

Review Comment:
   Mocking is so awkward; to be honest, I absolutely hate mocking.  The NoopSpan thing is exemplary of this; this test should make no mention of tracing whatsoever; not even one line.  If we need mocking, could we have test helpers/shims/subclasses such that the Mocking we need are pertinent to what it is being tested without referencing obscure unrelated things?



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] sonatype-lift[bot] commented on pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

sonatype-lift[bot] commented on PR #1053:
URL: https://github.com/apache/solr/pull/1053#issuecomment-1264743021

   :warning: **314 God Classes** were detected by Lift in this project. [Visit the Lift web console](https://lift.sonatype.com/results/github.com/apache/solr/01GED8XX72E6ZGK75713GJ01EA?tab=technical-debt&utm_source=github.com&utm_campaign=lift-comment&utm_content=apache\%20solr) for more details.


-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] gerlowskija commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

gerlowskija commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1019586251


##########
solr/core/src/java/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPI.java:
##########
@@ -17,49 +17,113 @@
 
 package org.apache.solr.handler.admin.api;
 
-import static org.apache.solr.client.solrj.SolrRequest.METHOD.POST;
-import static org.apache.solr.common.params.CollectionAdminParams.COLLECTION;
-import static org.apache.solr.common.params.CommonParams.ACTION;
-import static org.apache.solr.handler.ClusterAPI.wrapParams;
-import static org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM;
+import static org.apache.solr.cloud.Overseer.QUEUE_OPERATION;
+import static org.apache.solr.common.cloud.ZkStateReader.COLLECTION_PROP;
+import static org.apache.solr.common.cloud.ZkStateReader.PROPERTY_PROP;
+import static org.apache.solr.common.cloud.ZkStateReader.REPLICA_PROP;
+import static org.apache.solr.common.cloud.ZkStateReader.SHARD_ID_PROP;
+import static org.apache.solr.common.params.CollectionAdminParams.PROPERTY_PREFIX;
+import static org.apache.solr.handler.admin.CollectionsHandler.DEFAULT_COLLECTION_OP_TIMEOUT;
 
-import java.util.HashMap;
+import io.swagger.v3.oas.annotations.Parameter;
 import java.util.Map;
-import org.apache.solr.api.Command;
-import org.apache.solr.api.EndPoint;
-import org.apache.solr.api.PayloadObj;
-import org.apache.solr.client.solrj.request.beans.DeleteReplicaPropertyPayload;
+import javax.inject.Inject;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import org.apache.solr.client.solrj.SolrResponse;
+import org.apache.solr.common.cloud.ZkNodeProps;
 import org.apache.solr.common.params.CollectionParams;
+import org.apache.solr.common.params.RequiredSolrParams;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.core.CoreContainer;
 import org.apache.solr.handler.admin.CollectionsHandler;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
 
 /**
  * V2 API for removing a property from a collection replica
  *
- * <p>This API (POST /v2/collections/collectionName {'delete-replica-property': {...}}) is analogous
- * to the v1 /admin/collections?action=DELETEREPLICAPROP command.
- *
- * @see DeleteReplicaPropertyPayload
+ * <p>This API is analogous to the v1 /admin/collections?action=DELETEREPLICAPROP command.
  */
-@EndPoint(
-    path = {"/c/{collection}", "/collections/{collection}"},
-    method = POST,
-    permission = COLL_EDIT_PERM)
-public class DeleteReplicaPropertyAPI {
-  private static final String V2_DELETE_REPLICA_PROPERTY_CMD = "delete-replica-property";
+@Path("/collections/{collName}/shards/{shardName}/replicas/{replicaName}/properties/{propName}")
+public class DeleteReplicaPropertyAPI extends AdminAPIBase {
+
+  @Inject
+  public DeleteReplicaPropertyAPI(
+      CoreContainer coreContainer,
+      SolrQueryRequest solrQueryRequest,
+      SolrQueryResponse solrQueryResponse) {
+    super(coreContainer, solrQueryRequest, solrQueryResponse);
+  }
 
-  private final CollectionsHandler collectionsHandler;
+  public SolrJerseyResponse deleteReplicaProperty(
+      @Parameter(
+              description = "The name of the collection the replica belongs to.",
+              required = true)
+          @PathParam("collName")
+          String collName,
+      @Parameter(description = "The name of the shard the replica belongs to.", required = true)
+          @PathParam("shardName")
+          String shardName,
+      @Parameter(description = "The replica, e.g., `core_node1`.", required = true)
+          @PathParam("replicaName")
+          String replicaName,
+      @Parameter(description = "The name of the property to delete.", required = true)
+          @PathParam("propName")

Review Comment:
   I would've been fine with this change, but I'll hold off since I'm getting feedback both ways 😛 
   
   Fwiw, I don't think "propName" suffers at all from the abbreviation - there's not much plausible ambiguity about what "prop" might stand for. 🤷 



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] gerlowskija commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

gerlowskija commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1016747436


##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPITest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.apache.solr.cloud.api.collections.CollectionHandlingUtils.SHARD_UNIQUE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkNodeProps;
+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.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for {@link DeleteReplicaPropertyAPI} */
+public class DeleteReplicaPropertyAPITest extends SolrTestCaseJ4 {
+
+  private CoreContainer mockCoreContainer;
+  private DistributedCollectionConfigSetCommandRunner mockCommandRunner;
+  private SolrQueryRequest mockQueryRequest;
+  private SolrQueryResponse queryResponse;
+  private ArgumentCaptor<ZkNodeProps> messageCapturer;
+
+  private DeleteReplicaPropertyAPI deleteReplicaPropApi;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    mockCoreContainer = mock(CoreContainer.class);
+    mockCommandRunner = mock(DistributedCollectionConfigSetCommandRunner.class);
+    when(mockCoreContainer.getDistributedCollectionCommandRunner())
+        .thenReturn(Optional.of(mockCommandRunner));
+    when(mockCommandRunner.runCollectionCommand(any(), any(), anyLong()))
+        .thenReturn(new OverseerSolrResponse(new NamedList<>()));
+    mockQueryRequest = mock(SolrQueryRequest.class);
+    when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);
+    queryResponse = new SolrQueryResponse();
+    messageCapturer = ArgumentCaptor.forClass(ZkNodeProps.class);
+
+    deleteReplicaPropApi =
+        new DeleteReplicaPropertyAPI(mockCoreContainer, mockQueryRequest, queryResponse);
+  }
+
+  @Test
+  public void testReportsErrorWhenCalledInStandaloneMode() {
+    when(mockCoreContainer.isZooKeeperAware()).thenReturn(false);
+
+    final SolrException e =
+        expectThrows(
+            SolrException.class,
+            () -> {
+              deleteReplicaPropApi.deleteReplicaProperty(
+                  "someColl", "someShard", "someReplica", "somePropName");
+            });
+    assertEquals(400, e.code());
+    assertTrue(
+        "Exception message differed from expected: " + e.getMessage(),
+        e.getMessage().contains("not running in SolrCloud mode"));
+  }
+
+  @Test
+  public void testCreatesValidOverseerMessage() throws Exception {

Review Comment:
   OK, I'm not sure if I agree, but I definitely see the distinction you're pointing to there.  I appreciate you taking the time to explain where you're coming from on this.  I know it's a lot of back and forth for a small thing, but I'm hoping being thorough here will save time on reviews going forward.
   
   I'll try this as a compromise:
   
   - I still feel like the message-construction code is important to unit test for now, because it's where the bulk of the "API" logic is as the code stands today.  But it doesn't have to mention the overseer or even live in the API class itself. 
    I'll refactor the message-creation code out, and that'll let us unit test it without mocks **and** it'll separate the tests from the API class itself so that they can just be nuked if we move away from the overseer (rather than playing some "cementing" role).
   - We can refactor the v1 code that calls into v2 in such a way that it's testable (maybe using mocks, but hopefully not?) to ensure that the v1 codepath uses the v2 API class properly.
   
   Is that something you could live with? @dsmiley 



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] dsmiley commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

dsmiley commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1014010860


##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPITest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.apache.solr.cloud.api.collections.CollectionHandlingUtils.SHARD_UNIQUE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkNodeProps;
+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.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for {@link DeleteReplicaPropertyAPI} */
+public class DeleteReplicaPropertyAPITest extends SolrTestCaseJ4 {
+
+  private CoreContainer mockCoreContainer;
+  private DistributedCollectionConfigSetCommandRunner mockCommandRunner;
+  private SolrQueryRequest mockQueryRequest;
+  private SolrQueryResponse queryResponse;
+  private ArgumentCaptor<ZkNodeProps> messageCapturer;
+
+  private DeleteReplicaPropertyAPI deleteReplicaPropApi;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    mockCoreContainer = mock(CoreContainer.class);
+    mockCommandRunner = mock(DistributedCollectionConfigSetCommandRunner.class);
+    when(mockCoreContainer.getDistributedCollectionCommandRunner())
+        .thenReturn(Optional.of(mockCommandRunner));
+    when(mockCommandRunner.runCollectionCommand(any(), any(), anyLong()))
+        .thenReturn(new OverseerSolrResponse(new NamedList<>()));
+    mockQueryRequest = mock(SolrQueryRequest.class);
+    when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);
+    queryResponse = new SolrQueryResponse();
+    messageCapturer = ArgumentCaptor.forClass(ZkNodeProps.class);
+
+    deleteReplicaPropApi =
+        new DeleteReplicaPropertyAPI(mockCoreContainer, mockQueryRequest, queryResponse);
+  }
+
+  @Test
+  public void testReportsErrorWhenCalledInStandaloneMode() {
+    when(mockCoreContainer.isZooKeeperAware()).thenReturn(false);
+
+    final SolrException e =
+        expectThrows(
+            SolrException.class,
+            () -> {
+              deleteReplicaPropApi.deleteReplicaProperty(
+                  "someColl", "someShard", "someReplica", "somePropName");
+            });
+    assertEquals(400, e.code());
+    assertTrue(
+        "Exception message differed from expected: " + e.getMessage(),
+        e.getMessage().contains("not running in SolrCloud mode"));
+  }
+
+  @Test
+  public void testCreatesValidOverseerMessage() throws Exception {

Review Comment:
   > We can absolutely do that, and if you want me to I can. But I still feel like I'm missing some nuance here. Isn't CollectionHandler's use of DeleteReplicaPropertyAPI under-the-hood just as much of an "implementation detail" as DeleteReplicaPropertyAPI's use of the overseer?
   
   I very much disagree.  The *API classes are *the* primary internal API going forward.  We want to test that some old V1 API thing will call it.  Whatever the *API class does internally is an implementation detail.  Today it talks to the Overseer, tomorrow, lets remove the Overseer :-). 
   
   I'd rather no test test Overseer message crafting because that cements the Overseer into Solr further.



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] dsmiley commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

dsmiley commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1008392932


##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPITest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.apache.solr.cloud.api.collections.CollectionHandlingUtils.SHARD_UNIQUE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkNodeProps;
+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.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for {@link DeleteReplicaPropertyAPI} */
+public class DeleteReplicaPropertyAPITest extends SolrTestCaseJ4 {
+
+  private CoreContainer mockCoreContainer;
+  private DistributedCollectionConfigSetCommandRunner mockCommandRunner;
+  private SolrQueryRequest mockQueryRequest;
+  private SolrQueryResponse queryResponse;
+  private ArgumentCaptor<ZkNodeProps> messageCapturer;
+
+  private DeleteReplicaPropertyAPI deleteReplicaPropApi;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    mockCoreContainer = mock(CoreContainer.class);
+    mockCommandRunner = mock(DistributedCollectionConfigSetCommandRunner.class);
+    when(mockCoreContainer.getDistributedCollectionCommandRunner())
+        .thenReturn(Optional.of(mockCommandRunner));
+    when(mockCommandRunner.runCollectionCommand(any(), any(), anyLong()))
+        .thenReturn(new OverseerSolrResponse(new NamedList<>()));
+    mockQueryRequest = mock(SolrQueryRequest.class);
+    when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);
+    queryResponse = new SolrQueryResponse();
+    messageCapturer = ArgumentCaptor.forClass(ZkNodeProps.class);
+
+    deleteReplicaPropApi =
+        new DeleteReplicaPropertyAPI(mockCoreContainer, mockQueryRequest, queryResponse);
+  }
+
+  @Test
+  public void testReportsErrorWhenCalledInStandaloneMode() {
+    when(mockCoreContainer.isZooKeeperAware()).thenReturn(false);
+
+    final SolrException e =
+        expectThrows(
+            SolrException.class,
+            () -> {
+              deleteReplicaPropApi.deleteReplicaProperty(
+                  "someColl", "someShard", "someReplica", "somePropName");
+            });
+    assertEquals(400, e.code());
+    assertTrue(
+        "Exception message differed from expected: " + e.getMessage(),
+        e.getMessage().contains("not running in SolrCloud mode"));
+  }
+
+  @Test
+  public void testCreatesValidOverseerMessage() throws Exception {

Review Comment:
   You're right that a SolrCloud API integration test would involve running Jetty & ZK.  That is certainly a subset of the API surface area though.  For those tests, I would hope that variations of a single API (e.g. tests all its input variations) could be in the same test class using a common running instance.  For non-SolrCloud tests, especially after a great test refactor to be based on SolrClients, they would run with EmbeddedSolrServer (not Jetty) but could be toggled to a Jetty mode on a nightly job, say; even SolrCloud.  To be clear, I'm talking about one test class that is able to run in different modes based on say a system property toggle.  I do this at work now and I've done it in the past to great effect.  Maybe that's only partly related to our discussion above (how to test the API); but I want to get your reaction any way.  Of course we have real unit tests for some low-level things too :-)
   
   If V1 is merely going to be calling V2 (yay!!), then I agree we can have real unit tests that only test that mechanism in isolation.
   
   Still, there's how you test the primary API (V2).  We already have tests for these in general, albeit using V1?  They can be changed to use V2.
   
   I may be talking past you in a sense... but I wanted to get this out there and maybe circle back to your ideas.



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] gerlowskija commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

gerlowskija commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1006192774


##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPITest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.apache.solr.cloud.api.collections.CollectionHandlingUtils.SHARD_UNIQUE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkNodeProps;
+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.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for {@link DeleteReplicaPropertyAPI} */
+public class DeleteReplicaPropertyAPITest extends SolrTestCaseJ4 {
+
+  private CoreContainer mockCoreContainer;
+  private DistributedCollectionConfigSetCommandRunner mockCommandRunner;
+  private SolrQueryRequest mockQueryRequest;
+  private SolrQueryResponse queryResponse;
+  private ArgumentCaptor<ZkNodeProps> messageCapturer;
+
+  private DeleteReplicaPropertyAPI deleteReplicaPropApi;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    mockCoreContainer = mock(CoreContainer.class);
+    mockCommandRunner = mock(DistributedCollectionConfigSetCommandRunner.class);
+    when(mockCoreContainer.getDistributedCollectionCommandRunner())
+        .thenReturn(Optional.of(mockCommandRunner));
+    when(mockCommandRunner.runCollectionCommand(any(), any(), anyLong()))
+        .thenReturn(new OverseerSolrResponse(new NamedList<>()));
+    mockQueryRequest = mock(SolrQueryRequest.class);
+    when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);

Review Comment:
   Sorry for the delay replying here:
   
   Mocking can be awkward for sure.  You're not wrong.  But IMO most of the time when mocking is ugly or when it has to reference obscure unrelated things - it's holding up a mirror to the design of the code-under-test.
   
   e.g. We only have to muck about with NoopSpan here because the API logic itself in CollectionsHandler mucks about with spans (albeit indirectly, through `o.a.s.util.tracing.TracingUtils`).  I'm sure there's good reason for it to be where it is, but If tracing stuff was off in a Jetty servlet filter (or where ever) this test probably _would_ have no mention of it.
   
   I guess I'm not claiming that mocks are ideal, but I see them as a huge improvement (or at least, a badly needed supplement) to some of our other testing patterns.  We're going on a decade now of serious flakiness and speed concerns in our test suite!  And I think our communal aversion to anything that's _not_ an end-to-end test plays a big role there.  Spinning up full SolrCloud+ZK clusters is normal, and the "lighter" alternative still spins up full Jetty server(s)!  Talk about bringing in obscure, unrelated things!
   
   > If we need mocking, could we have test helpers/shims/subclasses such that the Mocking we need are pertinent to what it is being tested
   
   I can try abstracting some of this out into a helper or subclass, but my guess is that it might only have limited applicability.  v2 APIs that we pull out of CollectionsHandler will be able to use it, sure, but does the logic in e.g. ConfigSetsHandler interact with tracing in quite the same way?  Idk.
   
   Another alternative, if you still find the mocks repellent, would be to extract the "message creation" piece into a class that can be separately unit-tested without needing mocks.  That's essentially what these tests are validating, that the RPC message triggered by a given API call looks the way it should.  Or we could go a different route and do something that extends `JerseyTest`, though if I remember right you had other reasons for disliking that approach?
   
   So I guess there's 3 options there: (1) hide the mocks behind utils/parent-classes, (2) mockless unit testing on the message-creation logic only, or (3) something JerseyTest-based with hopefully fewer mocks.  Any of those jump out at you as preferable @dsmiley ?



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] gerlowskija commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

gerlowskija commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1006192774


##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPITest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.apache.solr.cloud.api.collections.CollectionHandlingUtils.SHARD_UNIQUE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkNodeProps;
+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.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for {@link DeleteReplicaPropertyAPI} */
+public class DeleteReplicaPropertyAPITest extends SolrTestCaseJ4 {
+
+  private CoreContainer mockCoreContainer;
+  private DistributedCollectionConfigSetCommandRunner mockCommandRunner;
+  private SolrQueryRequest mockQueryRequest;
+  private SolrQueryResponse queryResponse;
+  private ArgumentCaptor<ZkNodeProps> messageCapturer;
+
+  private DeleteReplicaPropertyAPI deleteReplicaPropApi;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    mockCoreContainer = mock(CoreContainer.class);
+    mockCommandRunner = mock(DistributedCollectionConfigSetCommandRunner.class);
+    when(mockCoreContainer.getDistributedCollectionCommandRunner())
+        .thenReturn(Optional.of(mockCommandRunner));
+    when(mockCommandRunner.runCollectionCommand(any(), any(), anyLong()))
+        .thenReturn(new OverseerSolrResponse(new NamedList<>()));
+    mockQueryRequest = mock(SolrQueryRequest.class);
+    when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);

Review Comment:
   Sorry for the delay replying here:
   
   Mocking can be awkward for sure.  You're not wrong.  But IMO most of the time when mocking is ugly or when it has to reference obscure unrelated things - it's a sign that the code-under-test is designed poorly.
   
   e.g. We only have to muck about with NoopSpan here because the API logic itself in CollectionsHandler mucks about with spans (albeit indirectly, through `o.a.s.util.tracing.TracingUtils`).  I'm sure there's good reason for it to be where it is, but If tracing stuff was off in a Jetty servlet filter (or where ever) this test probably _would_ have no mention of it.
   
   I guess I'm not claiming that mocks are ideal, but I see them as a huge improvement (or at least, a badly needed supplement) to some of our other testing patterns.  We're going on a decade now of serious flakiness and speed concerns in our test suite!  And I think our communal aversion to anything that's _not_ an end-to-end test plays a big role there.  Spinning up full SolrCloud+ZK clusters is normal!  Our "light" option still spins up 1 or more full Jetty servers!  Talk about bringing in obscure, unrelated things!
   
   > If we need mocking, could we have test helpers/shims/subclasses such that the Mocking we need are pertinent to what it is being tested
   
   I can try abstracting some of this out into a helper or subclass, but my guess is that it might only have limited applicability.  v2 APIs that we pull out of CollectionsHandler will be able to use it, sure, but does the logic in e.g. ConfigSetsHandler interact with tracing in quite the same way?  Idk.
   
   Another alternative, if you still find the mocks repellent, would be to extract the "message creation" piece into a class that can be separately unit-tested without needing mocks.  That's essentially what these tests are validating, that the RPC message triggered by a given API call looks the way it should.  Or we could go a different route and do something that extends `JerseyTest`, though if I remember right you had other reasons for disliking that approach?
   
   So I guess there's 3 options there: (1) hide the mocks behind utils/parent-classes, (2) mockless unit testing on the message-creation logic only, or (3) something JerseyTest-based with hopefully fewer mocks.  Any of those jump out at you as preferable @dsmiley ?



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] gerlowskija commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

gerlowskija commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1006937868


##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPITest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.apache.solr.cloud.api.collections.CollectionHandlingUtils.SHARD_UNIQUE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkNodeProps;
+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.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for {@link DeleteReplicaPropertyAPI} */
+public class DeleteReplicaPropertyAPITest extends SolrTestCaseJ4 {
+
+  private CoreContainer mockCoreContainer;
+  private DistributedCollectionConfigSetCommandRunner mockCommandRunner;
+  private SolrQueryRequest mockQueryRequest;
+  private SolrQueryResponse queryResponse;
+  private ArgumentCaptor<ZkNodeProps> messageCapturer;
+
+  private DeleteReplicaPropertyAPI deleteReplicaPropApi;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    mockCoreContainer = mock(CoreContainer.class);
+    mockCommandRunner = mock(DistributedCollectionConfigSetCommandRunner.class);
+    when(mockCoreContainer.getDistributedCollectionCommandRunner())
+        .thenReturn(Optional.of(mockCommandRunner));
+    when(mockCommandRunner.runCollectionCommand(any(), any(), anyLong()))
+        .thenReturn(new OverseerSolrResponse(new NamedList<>()));
+    mockQueryRequest = mock(SolrQueryRequest.class);
+    when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);
+    queryResponse = new SolrQueryResponse();
+    messageCapturer = ArgumentCaptor.forClass(ZkNodeProps.class);
+
+    deleteReplicaPropApi =
+        new DeleteReplicaPropertyAPI(mockCoreContainer, mockQueryRequest, queryResponse);
+  }
+
+  @Test
+  public void testReportsErrorWhenCalledInStandaloneMode() {
+    when(mockCoreContainer.isZooKeeperAware()).thenReturn(false);
+
+    final SolrException e =
+        expectThrows(
+            SolrException.class,
+            () -> {
+              deleteReplicaPropApi.deleteReplicaProperty(
+                  "someColl", "someShard", "someReplica", "somePropName");
+            });
+    assertEquals(400, e.code());
+    assertTrue(
+        "Exception message differed from expected: " + e.getMessage(),
+        e.getMessage().contains("not running in SolrCloud mode"));
+  }
+
+  @Test
+  public void testCreatesValidOverseerMessage() throws Exception {

Review Comment:
   Tbh I'm at a bit of a loss as to how to proceed in testing this stuff.
   
   I want to address your concerns as best as I can, but I feel pretty strongly that we shouldn't be writing even more integration tests every time we modify a v2 API.  And if I'm not misunderstanding you, that kindof sounds like what you're suggesting here.  (i.e. "set the property and then get the property" requires (at a minimum) a ZK for the props to live in, if not a full Solr to submit the API to)
   
   **Why am I anti-integration test for these PRs?**
   
   Firstly, because it just wouldn't scale.  If I add an integration test for each v2 API as I refactor it, we'll be near doubling the size of our test suite!  (Or at least, something on that order of magnitude *general handwavy-ness*.) And then, down the road when individual SolrJ classes switch over to using v2, we'll have essentially duplicate tests everywhere!
   
   Secondly, because an integration test wouldn't add all that much coverage for the trouble - these refactor PRs change the v1 codepath to _use_ the v2 objects, so the existing v1 integration tests are already testing the v2 code, in a sense.  There's a bit of a gap around validating that the v2 endpoint itself looks the way we want it to.  A v2-specific integration test _would_ cover that, but there are lighter, quicker, less flaky ways to close that gap (e.g. a `JerseyTest`-based unit test).
   
   Lastly, I don't see messages as internal at all.  In fact, I see them as external, almost by definition.  They're sent/received/understood by multiple sub-systems, which makes them internal to none, right?  Unless you consider even these interfaces between sub-components to be "internal"?
   
   Ultimately if you have this thing that one part of your system throws over the wall to another, it seems helpful to have some low-level automated validation that you don't accidentally steer from that contract.  An integration test might catch that break, but it might not.  That's one of the advantages of unit tests here - they can quickly cover more variations than is usually feasible to integration test.
   
   **So, What Now?**
   
   I'm really hoping there's common ground to address both our concerns.  Lmk if you see some.  I mentioned some potential alternatives in the second half of my comment [here](https://github.com/apache/solr/pull/1053#discussion_r1006192774), so hopefully one of those will work?
   
   I do really want to get this sorted out; not for the sake of this PR specifically but because this is just 1 of many similar PRs we'll need to get v2 into line, and I want to find something we can both live with in all those going forward.
   
   I'm going to bring all the non-test stuff in this PR up to date, so that when we find a compromise on the test stuff, we'll be ready to go.



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] epugh commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

epugh commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1019509981


##########
solr/core/src/java/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPI.java:
##########
@@ -17,49 +17,113 @@
 
 package org.apache.solr.handler.admin.api;
 
-import static org.apache.solr.client.solrj.SolrRequest.METHOD.POST;
-import static org.apache.solr.common.params.CollectionAdminParams.COLLECTION;
-import static org.apache.solr.common.params.CommonParams.ACTION;
-import static org.apache.solr.handler.ClusterAPI.wrapParams;
-import static org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM;
+import static org.apache.solr.cloud.Overseer.QUEUE_OPERATION;
+import static org.apache.solr.common.cloud.ZkStateReader.COLLECTION_PROP;
+import static org.apache.solr.common.cloud.ZkStateReader.PROPERTY_PROP;
+import static org.apache.solr.common.cloud.ZkStateReader.REPLICA_PROP;
+import static org.apache.solr.common.cloud.ZkStateReader.SHARD_ID_PROP;
+import static org.apache.solr.common.params.CollectionAdminParams.PROPERTY_PREFIX;
+import static org.apache.solr.handler.admin.CollectionsHandler.DEFAULT_COLLECTION_OP_TIMEOUT;
 
-import java.util.HashMap;
+import io.swagger.v3.oas.annotations.Parameter;
 import java.util.Map;
-import org.apache.solr.api.Command;
-import org.apache.solr.api.EndPoint;
-import org.apache.solr.api.PayloadObj;
-import org.apache.solr.client.solrj.request.beans.DeleteReplicaPropertyPayload;
+import javax.inject.Inject;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import org.apache.solr.client.solrj.SolrResponse;
+import org.apache.solr.common.cloud.ZkNodeProps;
 import org.apache.solr.common.params.CollectionParams;
+import org.apache.solr.common.params.RequiredSolrParams;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.core.CoreContainer;
 import org.apache.solr.handler.admin.CollectionsHandler;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
 
 /**
  * V2 API for removing a property from a collection replica
  *
- * <p>This API (POST /v2/collections/collectionName {'delete-replica-property': {...}}) is analogous
- * to the v1 /admin/collections?action=DELETEREPLICAPROP command.
- *
- * @see DeleteReplicaPropertyPayload
+ * <p>This API is analogous to the v1 /admin/collections?action=DELETEREPLICAPROP command.
  */
-@EndPoint(
-    path = {"/c/{collection}", "/collections/{collection}"},
-    method = POST,
-    permission = COLL_EDIT_PERM)
-public class DeleteReplicaPropertyAPI {
-  private static final String V2_DELETE_REPLICA_PROPERTY_CMD = "delete-replica-property";
+@Path("/collections/{collName}/shards/{shardName}/replicas/{replicaName}/properties/{propName}")

Review Comment:
   so pretty!



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] dsmiley commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

dsmiley commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1019522570


##########
solr/core/src/java/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPI.java:
##########
@@ -17,49 +17,113 @@
 
 package org.apache.solr.handler.admin.api;
 
-import static org.apache.solr.client.solrj.SolrRequest.METHOD.POST;
-import static org.apache.solr.common.params.CollectionAdminParams.COLLECTION;
-import static org.apache.solr.common.params.CommonParams.ACTION;
-import static org.apache.solr.handler.ClusterAPI.wrapParams;
-import static org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM;
+import static org.apache.solr.cloud.Overseer.QUEUE_OPERATION;
+import static org.apache.solr.common.cloud.ZkStateReader.COLLECTION_PROP;
+import static org.apache.solr.common.cloud.ZkStateReader.PROPERTY_PROP;
+import static org.apache.solr.common.cloud.ZkStateReader.REPLICA_PROP;
+import static org.apache.solr.common.cloud.ZkStateReader.SHARD_ID_PROP;
+import static org.apache.solr.common.params.CollectionAdminParams.PROPERTY_PREFIX;
+import static org.apache.solr.handler.admin.CollectionsHandler.DEFAULT_COLLECTION_OP_TIMEOUT;
 
-import java.util.HashMap;
+import io.swagger.v3.oas.annotations.Parameter;
 import java.util.Map;
-import org.apache.solr.api.Command;
-import org.apache.solr.api.EndPoint;
-import org.apache.solr.api.PayloadObj;
-import org.apache.solr.client.solrj.request.beans.DeleteReplicaPropertyPayload;
+import javax.inject.Inject;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import org.apache.solr.client.solrj.SolrResponse;
+import org.apache.solr.common.cloud.ZkNodeProps;
 import org.apache.solr.common.params.CollectionParams;
+import org.apache.solr.common.params.RequiredSolrParams;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.core.CoreContainer;
 import org.apache.solr.handler.admin.CollectionsHandler;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
 
 /**
  * V2 API for removing a property from a collection replica
  *
- * <p>This API (POST /v2/collections/collectionName {'delete-replica-property': {...}}) is analogous
- * to the v1 /admin/collections?action=DELETEREPLICAPROP command.
- *
- * @see DeleteReplicaPropertyPayload
+ * <p>This API is analogous to the v1 /admin/collections?action=DELETEREPLICAPROP command.
  */
-@EndPoint(
-    path = {"/c/{collection}", "/collections/{collection}"},
-    method = POST,
-    permission = COLL_EDIT_PERM)
-public class DeleteReplicaPropertyAPI {
-  private static final String V2_DELETE_REPLICA_PROPERTY_CMD = "delete-replica-property";
+@Path("/collections/{collName}/shards/{shardName}/replicas/{replicaName}/properties/{propName}")
+public class DeleteReplicaPropertyAPI extends AdminAPIBase {
+
+  @Inject
+  public DeleteReplicaPropertyAPI(
+      CoreContainer coreContainer,
+      SolrQueryRequest solrQueryRequest,
+      SolrQueryResponse solrQueryResponse) {
+    super(coreContainer, solrQueryRequest, solrQueryResponse);
+  }
 
-  private final CollectionsHandler collectionsHandler;
+  public SolrJerseyResponse deleteReplicaProperty(
+      @Parameter(
+              description = "The name of the collection the replica belongs to.",
+              required = true)
+          @PathParam("collName")
+          String collName,
+      @Parameter(description = "The name of the shard the replica belongs to.", required = true)
+          @PathParam("shardName")
+          String shardName,
+      @Parameter(description = "The replica, e.g., `core_node1`.", required = true)
+          @PathParam("replicaName")
+          String replicaName,
+      @Parameter(description = "The name of the property to delete.", required = true)
+          @PathParam("propName")
+          String propertyName)
+      throws Exception {
+    final SolrJerseyResponse response = instantiateJerseyResponse(SolrJerseyResponse.class);
+    final CoreContainer coreContainer = fetchAndValidateZooKeeperAwareCoreContainer();
+    recordCollectionForLogAndTracing(collName, solrQueryRequest);
 
-  public DeleteReplicaPropertyAPI(CollectionsHandler collectionsHandler) {
-    this.collectionsHandler = collectionsHandler;
+    final ZkNodeProps remoteMessage =
+        createRemoteMessage(collName, shardName, replicaName, propertyName);
+    final SolrResponse remoteResponse =
+        CollectionsHandler.submitCollectionApiCommand(
+            coreContainer,
+            coreContainer.getDistributedCollectionCommandRunner(),
+            remoteMessage,
+            CollectionParams.CollectionAction.DELETEREPLICAPROP,
+            DEFAULT_COLLECTION_OP_TIMEOUT);
+    if (remoteResponse.getException() != null) {
+      throw remoteResponse.getException();
+    }
+
+    disableResponseCaching();

Review Comment:
   Irrelevant unless GET which this isn't.



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org

[GitHub] [solr] gerlowskija commented on a diff in pull request #1053: SOLR-16392: Refactor and update v2 DELETEREPLICAPROP API

[GitBox <gi...@apache.org>]

gerlowskija commented on code in PR #1053:
URL: https://github.com/apache/solr/pull/1053#discussion_r1009778323


##########
solr/core/src/test/org/apache/solr/handler/admin/api/DeleteReplicaPropertyAPITest.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.apache.solr.cloud.api.collections.CollectionHandlingUtils.SHARD_UNIQUE;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.OverseerSolrResponse;
+import org.apache.solr.cloud.api.collections.DistributedCollectionConfigSetCommandRunner;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkNodeProps;
+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.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+/** Unit tests for {@link DeleteReplicaPropertyAPI} */
+public class DeleteReplicaPropertyAPITest extends SolrTestCaseJ4 {
+
+  private CoreContainer mockCoreContainer;
+  private DistributedCollectionConfigSetCommandRunner mockCommandRunner;
+  private SolrQueryRequest mockQueryRequest;
+  private SolrQueryResponse queryResponse;
+  private ArgumentCaptor<ZkNodeProps> messageCapturer;
+
+  private DeleteReplicaPropertyAPI deleteReplicaPropApi;
+
+  @BeforeClass
+  public static void ensureWorkingMockito() {
+    assumeWorkingMockito();
+  }
+
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+
+    mockCoreContainer = mock(CoreContainer.class);
+    mockCommandRunner = mock(DistributedCollectionConfigSetCommandRunner.class);
+    when(mockCoreContainer.getDistributedCollectionCommandRunner())
+        .thenReturn(Optional.of(mockCommandRunner));
+    when(mockCommandRunner.runCollectionCommand(any(), any(), anyLong()))
+        .thenReturn(new OverseerSolrResponse(new NamedList<>()));
+    mockQueryRequest = mock(SolrQueryRequest.class);
+    when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);
+    queryResponse = new SolrQueryResponse();
+    messageCapturer = ArgumentCaptor.forClass(ZkNodeProps.class);
+
+    deleteReplicaPropApi =
+        new DeleteReplicaPropertyAPI(mockCoreContainer, mockQueryRequest, queryResponse);
+  }
+
+  @Test
+  public void testReportsErrorWhenCalledInStandaloneMode() {
+    when(mockCoreContainer.isZooKeeperAware()).thenReturn(false);
+
+    final SolrException e =
+        expectThrows(
+            SolrException.class,
+            () -> {
+              deleteReplicaPropApi.deleteReplicaProperty(
+                  "someColl", "someShard", "someReplica", "somePropName");
+            });
+    assertEquals(400, e.code());
+    assertTrue(
+        "Exception message differed from expected: " + e.getMessage(),
+        e.getMessage().contains("not running in SolrCloud mode"));
+  }
+
+  @Test
+  public void testCreatesValidOverseerMessage() throws Exception {

Review Comment:
   > Maybe that's only partly related to our discussion above (how to test the API); but I want to get your reaction any way.
   
   It seems like a reasonable way to handle things.  My main concern would be that the base class abstracts things well enough so that test code doesn't need to know/care how Solr is actually run.  That sounds hard - my off-the-top-of-the-head impression is that we have lots of tests that fetch the JettySolrRunner for some reason or other.  But if you're already doing something similar in your work context, and you feel confident in the approach, it seems solid to me. 
   
   > If V1 is merely going to be calling V2 (yay!!), then I agree we can have real unit tests that only test that mechanism in isolation.
   
   Yep, that's exactly what this PR does!  Of course, there's some plumbing/shim logic to convert the v1 params into the format v2 expects, etc.  But with this PR, the heart of the v1 delete-replica-prop logic is "just" a [call to DeleteReplicaPropertyAPI.deleteReplicaProperty](https://github.com/apache/solr/pull/1053/files#diff-582348d44491dcb0ce1dfb169fb544e9e95620b2d0448eb1a1744f4e8dd5a349R1313).
   
   This is all, interestingly enough, inspired by a comment you yourself made [here](https://lists.apache.org/thread/43xy1drq8wswg6vpo9b11rkvdqtqomlv).  So, thanks haha!
   
   > Still, there's how you test the primary API (V2). We already have tests for these in general, albeit using V1? They can be changed to use V2.
   
   They can be switched over to v2, and that is my plan, but I don't think we're there just yet.  I really want people using v2, but v1 is what people are on today, so I think it makes sense to keep the coverage there, at least up until we near v1 deprecation.  (And of course, the whole v1-as-a-shim-on-top plan allows v1 integration tests to cover the v2 codepath implicitly, so it's not like that's untested while we defer the switch).
   
   (I'm close to publishing a SIP that aims to paint a picture of what the path to v1-deprecation looks like.  I'm sure it's awhile away, but maybe waiting will be more bearable when we have a concrete path to the destination? idk)



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org