gerlowskija commented on code in PR #1620:
URL: https://github.com/apache/solr/pull/1620#discussion_r1192313804
##########
solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java:
##########
@@ -798,6 +777,38 @@ private void getFileList(SolrParams solrParams,
SolrQueryResponse rsp) {
}
}
+ public CoreReplicationAPI.IndexVersionResponse getIndexVersionResponse()
throws IOException {
Review Comment:
[0] I know we're reliant on a few fields (`indexCommitPoint`,
`replicationEnabled`, `reserveCommitDuration`) in this class, but IMO we'd be
better off moving this method over to CoreReplicationAPI and creating getters
for any ReplicationHandler fields that we need.
From an abstraction perspective it leaks some ReplicationHandler details,
which isn't ideal. But that's a temporary situation as we move the remaining
api's/commands out of the RH here and into CoreReplicationAPI.
(This is largely opinion though - if you'd rather keep this here for now
that's also fine.)
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -813,6 +814,7 @@ public void load() {
registerV2ApiIfEnabled(packageLoader.getPackageAPI().readAPI);
registerV2ApiIfEnabled(ZookeeperReadAPI.class);
}
+ registerV2ApiIfEnabled(CoreReplicationAPI.class);
Review Comment:
[-1] Typically we only register "container-level" APIs (i.e. those APIs that
aren't specific to an individual core) here in CoreContainer. So we can
probably drop this line here.
For core-level APIs, it's sufficient to mention them in the
`getJerseyResources` method on the RequestHandler itself, as I can see you
already did below 👍
##########
solr/core/src/java/org/apache/solr/handler/replication/package-info.java:
##########
@@ -0,0 +1,19 @@
+/*
+ * 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.
+ */
+
+/** V2 API implementations for "replication" APIs. */
+package org.apache.solr.handler.replication;
Review Comment:
[0] If you want to create a package specifically for the APIs we're pulling
out of ReplicationHandler, that's fine by me. But:
1. Could you add 'api' to the package name somewhere? (e.g.
`o.a.s.handler.api.replication`). That's the convention I've tried to follow
elsewhere.
2. Do you envision each ReplicationHandler API/command its own class, or do
you imagine multiple of them going into CoreReplicationAPI? If the former,
maybe we should rename CoreReplicationAPI to be more specific (e.g.
`GetIndexVersionAPI`?). If the latter, maybe we don't need a separate package
for this code after all?
##########
solr/solr-ref-guide/modules/deployment-guide/pages/user-managed-index-replication.adoc:
##########
@@ -378,9 +378,26 @@
http://_leader_host:port_/solr/_core_name_/replication?command=disablereplicatio
`indexversion`::
Return the version of the latest replicatable index on the specified leader or
follower.
+
+====
+[.tab-label]*V1 API*
+
[source,bash]
+----
http://_host:port_/solr/_core_name_/replication?command=indexversion
+----
+====
++
+====
+[.tab-label]*V2 API*
+
+[source,bash]
+----
+http://_host:port_/api/_core_name_/replication/indexversion
Review Comment:
[-1] L396 should be
`http://_host:port_/api/cores/_core_name_/replication/indexversion` I think.
(The "/cores" segment is missing)
##########
solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java:
##########
@@ -1464,6 +1475,11 @@ public void inform(SolrCore core) {
log.info("Commits will be reserved for {} ms", reserveCommitDuration);
}
+ @Override
Review Comment:
[0] This method itself is great, but we also need to enable v2 APIs by
overriding `registerV2` to return true. (Currently, ReplicationHandler is
inheriting a `registerV2` implementation from `RequestHandlerBase` that returns
false.)
The method impl here should just be:
```
@Override
public Boolean registerV2() {
return Boolean.TRUE;
}
```
##########
solr/core/src/java/org/apache/solr/handler/replication/CoreReplicationAPI.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * 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.replication;
+
+import static
org.apache.solr.client.solrj.impl.BinaryResponseParser.BINARY_CONTENT_TYPE_V2;
+import static
org.apache.solr.security.PermissionNameProvider.Name.CORE_READ_PERM;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import io.swagger.v3.oas.annotations.Parameter;
+import java.io.IOException;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+@Path("/core/{coreName}/replication")
+public class CoreReplicationAPI extends ReplicationAPIBase {
+
+ @Inject
+ public CoreReplicationAPI(
+ CoreContainer coreContainer, SolrQueryRequest req, SolrQueryResponse
rsp) {
Review Comment:
[-1] We can inject the `SolrCore` directly in the constructor here, which
should save us from having to look it up from the CoreContainer later on. See
`SchemaNameAPI` as an example.
##########
solr/core/src/java/org/apache/solr/handler/replication/CoreReplicationAPI.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * 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.replication;
+
+import static
org.apache.solr.client.solrj.impl.BinaryResponseParser.BINARY_CONTENT_TYPE_V2;
+import static
org.apache.solr.security.PermissionNameProvider.Name.CORE_READ_PERM;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import io.swagger.v3.oas.annotations.Parameter;
+import java.io.IOException;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+@Path("/core/{coreName}/replication")
Review Comment:
[-1] The pattern we've established for collection/core APIs is
`/cores/{coreName}/....`. (Note that 'cores' in the first path segment is
plural.) Looks like we need an 's' at the end of the first path segment for us
to stick with that pattern.
##########
solr/core/src/test/org/apache/solr/handler/replication/CoreReplicationAPITest.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.replication;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import io.opentracing.noop.NoopSpan;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.handler.ReplicationHandler;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/** Unit tests for {@link CoreReplicationAPI} */
+public class CoreReplicationAPITest extends SolrTestCaseJ4 {
+
+ private CoreReplicationAPI coreReplicationAPI;
+ private CoreContainer mockCoreContainer;
+ private ReplicationHandler mockReplicationHandler;
+ private static final String coreName = "test";
+ private SolrQueryRequest mockQueryRequest;
+ private SolrQueryResponse queryResponse;
+
+ @BeforeClass
+ public static void ensureWorkingMockito() {
+ assumeWorkingMockito();
+ }
+
+ @Before
+ public void setUp() throws Exception {
+ super.setUp();
+ setUpMocks();
+ mockQueryRequest = mock(SolrQueryRequest.class);
+ when(mockQueryRequest.getSpan()).thenReturn(NoopSpan.INSTANCE);
+ queryResponse = new SolrQueryResponse();
+ coreReplicationAPI = new CoreReplicationAPI(mockCoreContainer,
mockQueryRequest, queryResponse);
+ }
+
+ @Test
+ public void testGetIndexVersion() throws Exception {
+ CoreReplicationAPI.IndexVersionResponse expected =
+ new CoreReplicationAPI.IndexVersionResponse(123L, 123L,
"testGeneration");
+
when(mockReplicationHandler.getIndexVersionResponse()).thenReturn(expected);
+
+ CoreReplicationAPI.IndexVersionResponse response =
+ coreReplicationAPI.IndexVersionResponse(coreName);
+ assertEquals(response.indexVersion, expected.indexVersion);
+ assertEquals(response.generation, expected.generation);
+ assertEquals(response.status, expected.status);
Review Comment:
[0] Tiny nit, but I think the "expected" value is supposed to be the first
argument to `assertEquals`. (Otherwise the error message that gets produced on
assertion-failure has the actual/expected values backwards.)
##########
solr/core/src/java/org/apache/solr/handler/replication/ReplicationAPIBase.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.replication;
+
+import java.io.IOException;
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.ReplicationHandler;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+/** A common parent for "replication" (i.e. replication-level) APIs. */
+public abstract class ReplicationAPIBase extends JerseyResource {
+
+ protected final CoreContainer coreContainer;
+ protected final SolrQueryRequest solrQueryRequest;
+ protected final SolrQueryResponse solrQueryResponse;
+
+ public ReplicationAPIBase(
+ CoreContainer coreContainer,
+ SolrQueryRequest solrQueryRequest,
+ SolrQueryResponse solrQueryResponse) {
+ this.coreContainer = coreContainer;
+ this.solrQueryRequest = solrQueryRequest;
+ this.solrQueryResponse = solrQueryResponse;
+ }
+
+ protected CoreReplicationAPI.GetIndexResponse fetchIndexVersion(String
coreName)
+ throws IOException {
+
+ if (coreContainer.getCore(coreName) == null) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST, "Solr core " + coreName + "
does not exist");
+ }
+
+ ReplicationHandler replicationHandler =
+ (ReplicationHandler)
+
coreContainer.getCore(coreName).getRequestHandler(ReplicationHandler.PATH);
Review Comment:
I couldn't find a viable alternative here to the approach you chose, so LGTM
for now.
Longer term I think we can refactor some of the logic out of
ReplicationHandler so that we can make do here without looking it up, but
that'll take more time and isn't something we should target for 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
