gerlowskija commented on code in PR #1119:
URL: https://github.com/apache/solr/pull/1119#discussion_r1004311073
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -452,6 +454,11 @@ void call() throws Exception {
}
}
+ @Override
Review Comment:
[Q] There are lots of backup APIs - maybe I just got confused about which
one you were working on - but I would've expected this new method to appear in
ReplicationHandler since that's the backup functionality you're intending to
address. What am I missing?
##########
solr/core/src/java/org/apache/solr/handler/replication/BackupAPI.java:
##########
@@ -0,0 +1,100 @@
+package org.apache.solr.handler.replication;
Review Comment:
[0] Don't forget your copyright header setup; it's nearly impossible IMO to
remember to put the header in yourself on every file you create!
##########
solr/core/src/test/org/apache/solr/handler/replication/BackupAPITest.java:
##########
@@ -0,0 +1,102 @@
+package org.apache.solr.handler.replication;
+
+import static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito;
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import javax.inject.Singleton;
+import javax.ws.rs.client.Entity;
+import javax.ws.rs.core.Application;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.handler.ReplicationHandler;
+import org.apache.solr.jersey.InjectionFactories;
+import org.apache.solr.jersey.SolrJacksonMapper;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.glassfish.hk2.utilities.binding.AbstractBinder;
+import org.glassfish.jersey.process.internal.RequestScoped;
+import org.glassfish.jersey.server.ResourceConfig;
+import org.glassfish.jersey.test.JerseyTest;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/** Unit test of v2 endpoints for Replication Handler backup command */
Review Comment:
[+1] This is a cool test; I've only experimented with using `JerseyTest` up
to this point, but I like what I see so far!
[0] You could probably do a javadoc "link", rather than trying to describe
the API being tested in words.
e.g.
```
/**
* Unit tests for {@link BackupAPI}
*/
```
##########
solr/core/src/java/org/apache/solr/handler/replication/BackupAPI.java:
##########
@@ -0,0 +1,100 @@
+package org.apache.solr.handler.replication;
+
+import static org.apache.solr.handler.ClusterAPI.wrapParams;
+import static
org.apache.solr.security.PermissionNameProvider.Name.CORE_EDIT_PERM;
+
+import io.swagger.v3.oas.annotations.Operation;
+import io.swagger.v3.oas.annotations.media.Schema;
+import io.swagger.v3.oas.annotations.parameters.RequestBody;
+import java.util.HashMap;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.MediaType;
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.common.annotation.JsonProperty;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.ReplicationHandler;
+import org.apache.solr.jersey.JacksonReflectMapWriter;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+/** V2 endpoint for Backup API used for User-Managed clusters and Single-Node
Installation. */
+@Path("/cores/{cores}/replication/backups")
+public class BackupAPI extends JerseyResource {
Review Comment:
[-1] Because Solr has a bunch of different "backup"-related features and
APIs, I think we should come up with a more descriptive name here. Maybe
"ReplicationBackupAPI", since that's where the functionality initially existed?
Maybe "SnapshotBackupAPI", since this triggers the "snapshot" functionality
and not a "true" backup? Idk. Any ideas?
##########
solr/core/src/java/org/apache/solr/handler/replication/BackupAPI.java:
##########
@@ -0,0 +1,100 @@
+package org.apache.solr.handler.replication;
+
+import static org.apache.solr.handler.ClusterAPI.wrapParams;
+import static
org.apache.solr.security.PermissionNameProvider.Name.CORE_EDIT_PERM;
+
+import io.swagger.v3.oas.annotations.Operation;
+import io.swagger.v3.oas.annotations.media.Schema;
+import io.swagger.v3.oas.annotations.parameters.RequestBody;
+import java.util.HashMap;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.MediaType;
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.common.annotation.JsonProperty;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.ReplicationHandler;
+import org.apache.solr.jersey.JacksonReflectMapWriter;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+/** V2 endpoint for Backup API used for User-Managed clusters and Single-Node
Installation. */
+@Path("/cores/{cores}/replication/backups")
Review Comment:
[0] Personally, I'd make the path-variable singular instead of plural (i.e.
"core"), since it's referring to a single specific core.
##########
solr/core/src/java/org/apache/solr/handler/replication/BackupAPI.java:
##########
@@ -0,0 +1,100 @@
+package org.apache.solr.handler.replication;
+
+import static org.apache.solr.handler.ClusterAPI.wrapParams;
+import static
org.apache.solr.security.PermissionNameProvider.Name.CORE_EDIT_PERM;
+
+import io.swagger.v3.oas.annotations.Operation;
+import io.swagger.v3.oas.annotations.media.Schema;
+import io.swagger.v3.oas.annotations.parameters.RequestBody;
+import java.util.HashMap;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.MediaType;
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.common.annotation.JsonProperty;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.ReplicationHandler;
+import org.apache.solr.jersey.JacksonReflectMapWriter;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+/** V2 endpoint for Backup API used for User-Managed clusters and Single-Node
Installation. */
+@Path("/cores/{cores}/replication/backups")
+public class BackupAPI extends JerseyResource {
+ private final SolrQueryRequest solrQueryRequest;
+ private final SolrQueryResponse solrQueryResponse;
+ private final ReplicationHandler replicationHandler;
+
+ @PathParam("cores")
+ private String coreName;
+
+ @Inject
+ public BackupAPI(
+ CoreContainer cc, SolrQueryRequest solrQueryRequest, SolrQueryResponse
solrQueryResponse) {
+ this.replicationHandler =
+ (ReplicationHandler)
cc.getCore(coreName).getRequestHandler(ReplicationHandler.PATH);
+ this.solrQueryRequest = solrQueryRequest;
+ this.solrQueryResponse = solrQueryResponse;
+ }
+ /**
+ * This API (POST /api/cores/coreName/replication/backups {...}) is
analogous to the v1
+ * /solr/coreName/replication?command=backup
+ */
+ @POST
+ @Produces({MediaType.APPLICATION_JSON})
+ @Operation(
+ summary = "Backup command using ReplicationHandler",
+ tags = {"cores"})
+ @PermissionName(CORE_EDIT_PERM)
+ public SolrJerseyResponse createBackup(
+ @RequestBody BackupReplicationPayload backupReplicationPayload) throws
Exception {
+ final SolrJerseyResponse response =
instantiateJerseyResponse(SolrJerseyResponse.class);
+ final Map<String, Object> v1Params = backupReplicationPayload.toMap(new
HashMap<>());
+ v1Params.put(ReplicationHandler.COMMAND, ReplicationHandler.CMD_BACKUP);
+ replicationHandler.handleRequestBody(wrapParams(solrQueryRequest,
v1Params), solrQueryResponse);
+ return response;
+ }
+
+ /* POJO for v2 endpoints request body.*/
+ public static class BackupReplicationPayload implements
JacksonReflectMapWriter {
Review Comment:
[0] The traditional v2 framework set up the convention of naming these
classes "Payloads", so this definitely isn't "wrong". But my personal
preference would be to go with "RequestBody" for our JAX-RS APIs (e.g.
BackupReplicationRequestBody).
If you don't have a strong preference for 'Payload', consider renaming to
use 'RequestBody' instead.
##########
solr/core/src/java/org/apache/solr/handler/replication/BackupAPI.java:
##########
@@ -0,0 +1,100 @@
+package org.apache.solr.handler.replication;
+
+import static org.apache.solr.handler.ClusterAPI.wrapParams;
+import static
org.apache.solr.security.PermissionNameProvider.Name.CORE_EDIT_PERM;
+
+import io.swagger.v3.oas.annotations.Operation;
+import io.swagger.v3.oas.annotations.media.Schema;
+import io.swagger.v3.oas.annotations.parameters.RequestBody;
+import java.util.HashMap;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.MediaType;
+import org.apache.solr.api.JerseyResource;
+import org.apache.solr.common.annotation.JsonProperty;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.ReplicationHandler;
+import org.apache.solr.jersey.JacksonReflectMapWriter;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.jersey.SolrJerseyResponse;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+
+/** V2 endpoint for Backup API used for User-Managed clusters and Single-Node
Installation. */
+@Path("/cores/{cores}/replication/backups")
+public class BackupAPI extends JerseyResource {
+ private final SolrQueryRequest solrQueryRequest;
+ private final SolrQueryResponse solrQueryResponse;
+ private final ReplicationHandler replicationHandler;
+
+ @PathParam("cores")
+ private String coreName;
+
+ @Inject
+ public BackupAPI(
+ CoreContainer cc, SolrQueryRequest solrQueryRequest, SolrQueryResponse
solrQueryResponse) {
+ this.replicationHandler =
+ (ReplicationHandler)
cc.getCore(coreName).getRequestHandler(ReplicationHandler.PATH);
Review Comment:
[0] I wasn't able to reproduce the injection issue you mentioned - in fact I
was having trouble triggering the API at all, but this code here strikes me as
a likely culprit for the issue you described.
We're using 'coreName' here to look up the SolrCore object, but what value
does 'coreName' have at this point? Idk whether it would be set or not when
the constructor gets called. I'd guess not: I'm not sure the "instance
variable" even exists for the `@PathParam` annotation to give it a value until
after the constructor is called. Reflection stuff is weird, so I wouldn't bet
on it for sure, but that'd be my guess.
Anyway, if 'coreName' is null here, I bet we'd get a null SolrCore and
trigger a NPE at this line.
----
Assuming this is the issue (which is a big 'IF', admittedly) - I'd suggest
we take the bit of functionality we need from ReplicationHandler and move it to
a public method here, instead of trying to reuse ReplicationHandler directly.
There are "strategic" reasons that Solr wants to move in this sort of
direction, see my comment in JIRA
[here](https://issues.apache.org/jira/browse/SOLR-16461?focusedCommentId=17619003&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17619003)
for a bit more context.
Happy to help with that if need be, lmk.
--
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]
