gerlowskija commented on code in PR #1704:
URL: https://github.com/apache/solr/pull/1704#discussion_r1233125232
##########
solr/core/src/java/org/apache/solr/handler/admin/api/CoreReplicationAPI.java:
##########
@@ -46,12 +50,24 @@ public CoreReplicationAPI(SolrCore solrCore,
SolrQueryRequest req, SolrQueryResp
@GET
@Path("/indexversion")
- @Produces({"application/json", "application/xml", BINARY_CONTENT_TYPE_V2})
+ @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML,
BINARY_CONTENT_TYPE_V2})
@PermissionName(CORE_READ_PERM)
public IndexVersionResponse fetchIndexVersion() throws IOException {
return doFetchIndexVersion();
}
+ @GET
+ @Path("/files/generation/{gen}")
Review Comment:
[Q] For a lot of these v2 APIs, I've been working from a
[spreadsheet](https://docs.google.com/spreadsheets/d/1HAoBBFPpSiT8mJmgNZKkZAPwfCfPvlc08m5jz3fQBpA)
that describes what the new endpoints should be.
In the spreadsheet, the suggested v2 API was just `GET
/cores/coreName/replication/files`. (I think at the time I was assuming
"generation" would be a required query parameter, rather than a path-param.
Though it's possible I didn't even know about it at that point.)
Not that we have to stick to that necessarily - if you think it makes sense
to keep "generation" in the path, it's def an option.
Any thoughts?
##########
solr/solr-ref-guide/modules/deployment-guide/pages/user-managed-index-replication.adoc:
##########
@@ -433,9 +433,27 @@
http://_follower_host:port_/solr/_core_name_/replication?command=details
`filelist`::
Retrieve a list of Lucene files present in the specified host's index.
+
+
+====
+[.tab-label]*V1 API*
+
[source,bash]
+----
http://_host:port_/solr/_core_name_/replication?command=filelist&generation=<_generation-number_>
+
+----
+====
++
Review Comment:
[+1] Thanks for thinking of the docs!
##########
solr/core/src/java/org/apache/solr/handler/admin/api/ReplicationAPIBase.java:
##########
@@ -38,10 +68,140 @@ public ReplicationAPIBase(
}
protected CoreReplicationAPI.IndexVersionResponse doFetchIndexVersion()
throws IOException {
+ ReplicationHandler replicationHandler =
+ (ReplicationHandler)
solrCore.getRequestHandler(ReplicationHandler.PATH);
+ return replicationHandler.getIndexVersionResponse();
+ }
+ protected NamedList<Object> doFetchFiles(long generation) {
ReplicationHandler replicationHandler =
(ReplicationHandler)
solrCore.getRequestHandler(ReplicationHandler.PATH);
+ return getFileList(generation, replicationHandler);
+ }
- return replicationHandler.getIndexVersionResponse();
+ protected NamedList<Object> getFileList(long generation, ReplicationHandler
replicationHandler) {
+ final IndexDeletionPolicyWrapper delPol = solrCore.getDeletionPolicy();
+ final NamedList<Object> filesResponse = new NamedList<>();
+
+ IndexCommit commit = null;
+ try {
+ if (generation == -1) {
Review Comment:
[Q] Mostly thinking out loud here, but I wonder whether we should just fall
back on "-1"
as a default. (Right now we require users to provide an explicit
generation value).
Probably not something to handle in this PR regardless, just wanted to jot
down the thought somewhere...
##########
solr/core/src/java/org/apache/solr/handler/admin/api/CoreReplicationAPI.java:
##########
@@ -46,12 +50,24 @@ public CoreReplicationAPI(SolrCore solrCore,
SolrQueryRequest req, SolrQueryResp
@GET
@Path("/indexversion")
- @Produces({"application/json", "application/xml", BINARY_CONTENT_TYPE_V2})
+ @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML,
BINARY_CONTENT_TYPE_V2})
@PermissionName(CORE_READ_PERM)
public IndexVersionResponse fetchIndexVersion() throws IOException {
return doFetchIndexVersion();
}
+ @GET
+ @Path("/files/generation/{gen}")
+ @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML,
BINARY_CONTENT_TYPE_V2})
+ @PermissionName(CORE_READ_PERM)
+ public NamedList<Object> fetchFiles(
Review Comment:
[0] The method name "fetchFiles" sounds a little bit like you're getting the
file-content itself, rather than a list of file names/metadata. Might be worth
updating.
[-0] One of the values of the JAX-RS framework is that we can be explicit
about the inputs and outputs of our APIs. If possible, we should avoid
returning Namedlist here and instead return some other strongly typed class
that makes it clearer what the response should look 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: [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]
