You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "stillalex (via GitHub)" <gi...@apache.org> on 2023/02/03 22:45:19 UTC
[GitHub] [solr] stillalex opened a new pull request, #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
stillalex opened a new pull request, #1332:
URL: https://github.com/apache/solr/pull/1332
https://issues.apache.org/jira/browse/SOLR-16393
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Solr:
* https://issues.apache.org/jira/projects/SOLR
For something minor (i.e. that wouldn't be worth putting in release notes), you can skip JIRA.
To create a Jira issue, you will need to create an account there first.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# Description
Alias apis
# Solution
Please provide a short description of the approach taken to implement your solution.
# Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
# Checklist
Please review the following and check all that apply:
- [ ] 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.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] 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)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] 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] stillalex commented on pull request #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #1332:
URL: https://github.com/apache/solr/pull/1332#issuecomment-1442378523
@gerlowskija added the new entry in the response, a test for the new api and updated the guide. please take a look!
--
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 #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1332:
URL: https://github.com/apache/solr/pull/1332#discussion_r1103164492
##########
solr/core/src/java/org/apache/solr/handler/admin/api/GetAliasesAPI.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.client.solrj.impl.BinaryResponseParser.BINARY_CONTENT_TYPE_V2;
+import static org.apache.solr.security.PermissionNameProvider.Name.COLL_READ_PERM;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.MediaType;
+import org.apache.solr.common.cloud.Aliases;
+import org.apache.solr.common.cloud.ZkStateReader;
+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;
+
+/**
+ * V2 APIs for listing aliases
+ *
+ * <p>This API <code>GET /api/aliases</code> is analogous to the v1 <code>GET /api/cluster/aliases
+ * </code> API.
+ */
+@Path("/aliases")
+public class GetAliasesAPI extends AdminAPIBase {
Review Comment:
[0] Nitpick that you can def ignore, but I'd prefer the name `ListAliasesAPI` for this class instead. Right now Solr has no API to fetch information about a single alias, but it'd be a reasonable API to add at some point. And if that ever happens, it'd be hard for a reader to distinguish between GetAliasesAPI and GetAliasAPI without a double-take 😆
##########
solr/core/src/java/org/apache/solr/jersey/SolrRequestAuthorizer.java:
##########
@@ -66,7 +67,9 @@ public void filter(ContainerRequestContext requestContext) throws IOException {
(AuthorizationContext.RequestType)
requestContext.getProperty(RequestContextKeys.REQUEST_TYPE);
final List<String> collectionNames =
- (List<String>) requestContext.getProperty(RequestContextKeys.COLLECTION_LIST);
+ Objects.requireNonNullElse(
Review Comment:
[Q] This seems like a reasonable change, but I'm curious how its related to the rest of the PR. Did you run into an NPE while writing the tests, or did it come up some other way?
--
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 #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1332:
URL: https://github.com/apache/solr/pull/1332#issuecomment-1456769251
Hey, sorry for the delay in circling back to this @stillalex - will take a look shortly with a mind to commit.
<img width="351" alt="Screen Shot 2023-03-06 at 1 53 57 PM" src="https://user-images.githubusercontent.com/9030708/223204161-5349de59-a741-4de5-b2e1-353613368a77.png">
Careful about doing this. If I'd happened to push up some tweaks or something around the same time, they could've been blown away. It also does weird things with line-level review comments in the Github UI, that can be a bit of a pain when reviewing.
--
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] stillalex commented on a diff in pull request #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1332:
URL: https://github.com/apache/solr/pull/1332#discussion_r1103337064
##########
solr/core/src/java/org/apache/solr/handler/admin/api/GetAliasesAPI.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.client.solrj.impl.BinaryResponseParser.BINARY_CONTENT_TYPE_V2;
+import static org.apache.solr.security.PermissionNameProvider.Name.COLL_READ_PERM;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import javax.inject.Inject;
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.MediaType;
+import org.apache.solr.common.cloud.Aliases;
+import org.apache.solr.common.cloud.ZkStateReader;
+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;
+
+/**
+ * V2 APIs for listing aliases
+ *
+ * <p>This API <code>GET /api/aliases</code> is analogous to the v1 <code>GET /api/cluster/aliases
+ * </code> API.
+ */
+@Path("/aliases")
+public class GetAliasesAPI extends AdminAPIBase {
Review Comment:
sure, renaming makes sense
--
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] stillalex commented on pull request #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #1332:
URL: https://github.com/apache/solr/pull/1332#issuecomment-1446899950
@gerlowskija gentle ping. please take another look and let me know your thoughts
--
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] stillalex commented on a diff in pull request #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1332:
URL: https://github.com/apache/solr/pull/1332#discussion_r1126981620
##########
solr/core/src/java/org/apache/solr/handler/admin/api/ListAliasesAPI.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.client.solrj.impl.BinaryResponseParser.BINARY_CONTENT_TYPE_V2;
+import static org.apache.solr.security.PermissionNameProvider.Name.COLL_READ_PERM;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import io.swagger.v3.oas.annotations.Operation;
+import io.swagger.v3.oas.annotations.Parameter;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+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 javax.ws.rs.core.MediaType;
+import org.apache.solr.common.cloud.Aliases;
+import org.apache.solr.common.cloud.ZkStateReader;
+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;
+
+/** V2 APIs for managing and inspecting collection aliases */
+@Path("/aliases")
+public class ListAliasesAPI extends AdminAPIBase {
+
+ @Inject
+ public ListAliasesAPI(
+ CoreContainer coreContainer,
+ SolrQueryRequest solrQueryRequest,
+ SolrQueryResponse solrQueryResponse) {
+ super(coreContainer, solrQueryRequest, solrQueryResponse);
+ }
+
+ /**
+ * V2 API for listing all aliases known by Solr.
+ *
+ * <p>This API <code>GET /api/aliases</code> is analogous to the v1 <code>GET /api/cluster/aliases
+ * </code> API.
+ */
+ @GET
+ @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML, BINARY_CONTENT_TYPE_V2})
+ @PermissionName(COLL_READ_PERM)
+ @Operation(
+ summary = "List the existing collection aliases.",
+ tags = {"aliases"})
+ public ListAliasesResponse getAliases() throws Exception {
+ recordCollectionForLogAndTracing(null, solrQueryRequest);
+
+ final ListAliasesResponse response = instantiateJerseyResponse(ListAliasesResponse.class);
+ final CoreContainer coreContainer = fetchAndValidateZooKeeperAwareCoreContainer();
+
+ ZkStateReader zkStateReader = coreContainer.getZkController().getZkStateReader();
+
+ // if someone calls listAliases, lets ensure we return an up to date response
+ zkStateReader.getAliasesManager().update();
+
+ Aliases aliases = zkStateReader.getAliases();
+
+ if (aliases != null) {
+ // the aliases themselves...
+ response.aliases = aliases.getCollectionAliasMap();
+ // Any properties for the above aliases.
+ Map<String, Map<String, String>> meta = new LinkedHashMap<>();
+ for (String alias : aliases.getCollectionAliasListMap().keySet()) {
+ Map<String, String> collectionAliasProperties = aliases.getCollectionAliasProperties(alias);
+ if (!collectionAliasProperties.isEmpty()) {
+ meta.put(alias, collectionAliasProperties);
+ }
+ }
+ response.properties = meta;
+ }
+
+ return response;
+ }
+
+ @GET
+ @Path("/{aliasName}")
+ @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML, BINARY_CONTENT_TYPE_V2})
+ @PermissionName(COLL_READ_PERM)
+ @Operation(
+ summary = "Get details for a specific collection alias.",
+ tags = {"aliases"})
+ public GetAliasByNameResponse getAliasByName(
+ @Parameter(description = "Alias name.", required = true) @PathParam("aliasName")
+ String aliasName)
+ throws Exception {
+ recordCollectionForLogAndTracing(null, solrQueryRequest);
+
+ final GetAliasByNameResponse response = instantiateJerseyResponse(GetAliasByNameResponse.class);
+ response.alias = aliasName;
+
+ final CoreContainer coreContainer = fetchAndValidateZooKeeperAwareCoreContainer();
+ ZkStateReader zkStateReader = coreContainer.getZkController().getZkStateReader();
+ // if someone calls listAliases, lets ensure we return an up to date response
Review Comment:
looks good
--
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] stillalex commented on pull request #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #1332:
URL: https://github.com/apache/solr/pull/1332#issuecomment-1426458975
thanks for talking a look. I think I wanted to have a more complete PR with all apis. will try to add more to this, but if it turns into too much code, I will opt for splitting, it will also make the review a bit easier I guess.
--
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] stillalex commented on pull request #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #1332:
URL: https://github.com/apache/solr/pull/1332#issuecomment-1459015763
thank you @gerlowskija for the help in getting this merged!
--
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] stillalex commented on pull request #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #1332:
URL: https://github.com/apache/solr/pull/1332#issuecomment-1442333965
> AFAICT GET /api/aliases/aliasName is a brand new API, so we have some freedom to make the response format whatever we would like here. Would you be amenable to adding the collection-list to this response, or was there a reason you avoided that on your initial pass?
ouch, good catch! you are absolutely right, I missed this completely. for sure the collections list needs to be present. any preference on the format? what about 'collections' (similar to the name used when creating the alias)?
```
"name": "alias1",
"collections": "collection1",
"properties": {}
```
--
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] stillalex commented on pull request #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #1332:
URL: https://github.com/apache/solr/pull/1332#issuecomment-1416495408
@gerlowskija if you have some time, please take a look!
--
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] stillalex commented on a diff in pull request #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1332:
URL: https://github.com/apache/solr/pull/1332#discussion_r1103338113
##########
solr/core/src/java/org/apache/solr/jersey/SolrRequestAuthorizer.java:
##########
@@ -66,7 +67,9 @@ public void filter(ContainerRequestContext requestContext) throws IOException {
(AuthorizationContext.RequestType)
requestContext.getProperty(RequestContextKeys.REQUEST_TYPE);
final List<String> collectionNames =
- (List<String>) requestContext.getProperty(RequestContextKeys.COLLECTION_LIST);
+ Objects.requireNonNullElse(
Review Comment:
hmm. hit this during unit testing. looked again and maybe it's due to my test rather than the value being null. will check again and remove this if it's not needed.
--
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 #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1332:
URL: https://github.com/apache/solr/pull/1332#discussion_r1107323124
##########
.gitignore:
##########
@@ -16,8 +16,12 @@ out/
# Eclipse
/.project
+**/.project
Review Comment:
[Q] Did you intend to add this gitignore change to the PR?
IMO this should be its own ticket since it's unrelated to the main thrust of this PR.
In the meantime, if this stuff is causing you pain as you develop, you could add it to your machine's ["global" gitignore file](https://sebastiandedeyne.com/setting-up-a-global-gitignore-file/).
--
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 #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1332:
URL: https://github.com/apache/solr/pull/1332#issuecomment-1456812218
Force-pushes also break this really nice UI feature that Github has of, "Only show me what's changed since the last time I posted a review", fwiw.
<img width="477" alt="Screen Shot 2023-03-06 at 2 17 58 PM" src="https://user-images.githubusercontent.com/9030708/223208993-6e85876b-75d3-4450-9b20-4bbcdc46d3d6.png">
--
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 #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija merged PR #1332:
URL: https://github.com/apache/solr/pull/1332
--
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 #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1332:
URL: https://github.com/apache/solr/pull/1332#issuecomment-1442275493
Alright, I fixed the ClusterAPI duplicate I mentioned, but in the process I found a few other things that were worth discussing here.
Specifically, the response that comes back from `GET /api/aliases/aliasName` isn't very useful. It's great that it returns the the free form properties. But there's not even a mention of the collections that the alias is a stand-in for.
```
$ curl -sk -X GET "http://localhost:8983/api/aliases/alias1" | jq '.'
{
"responseHeader": {
"status": 0,
"QTime": 1
},
"name": "alias1",
"properties": {}
}
```
AFAICT `GET /api/aliases/aliasName` is a brand new API, so we have some freedom to make the response format whatever we would like here. Would you be amenable to adding the collection-list to this response, or was there a reason you avoided that on your initial pass?
--
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 #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1332:
URL: https://github.com/apache/solr/pull/1332#issuecomment-1421221222
Hey, thanks for the PR Alex.
A lot of these have been blocked for awhile on SOLR-16531, but that's close enough to closed out that we should be good to start transitioning APIs again. I'll take a look shortly!
--
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 #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1332:
URL: https://github.com/apache/solr/pull/1332#issuecomment-1442139719
Small heads up for you @stillalex - I know it's tempting to force-push, but we've found that it can do weird things to Github comment history and make PRs just that bit harder to review. Obviously sometimes things get messy and you have to do what you have to do, but try to avoid those where you can at least please!
--
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] stillalex commented on a diff in pull request #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1332:
URL: https://github.com/apache/solr/pull/1332#discussion_r1107505494
##########
.gitignore:
##########
@@ -16,8 +16,12 @@ out/
# Eclipse
/.project
+**/.project
Review Comment:
don't worry about this one, I have opened a different PR for Eclipse things https://github.com/apache/solr/pull/1357 just forgot to remove it from 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 a diff in pull request #1332: SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1332:
URL: https://github.com/apache/solr/pull/1332#discussion_r1126924886
##########
solr/core/src/java/org/apache/solr/handler/admin/api/ListAliasesAPI.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.client.solrj.impl.BinaryResponseParser.BINARY_CONTENT_TYPE_V2;
+import static org.apache.solr.security.PermissionNameProvider.Name.COLL_READ_PERM;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import io.swagger.v3.oas.annotations.Operation;
+import io.swagger.v3.oas.annotations.Parameter;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+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 javax.ws.rs.core.MediaType;
+import org.apache.solr.common.cloud.Aliases;
+import org.apache.solr.common.cloud.ZkStateReader;
+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;
+
+/** V2 APIs for managing and inspecting collection aliases */
+@Path("/aliases")
+public class ListAliasesAPI extends AdminAPIBase {
+
+ @Inject
+ public ListAliasesAPI(
+ CoreContainer coreContainer,
+ SolrQueryRequest solrQueryRequest,
+ SolrQueryResponse solrQueryResponse) {
+ super(coreContainer, solrQueryRequest, solrQueryResponse);
+ }
+
+ /**
+ * V2 API for listing all aliases known by Solr.
+ *
+ * <p>This API <code>GET /api/aliases</code> is analogous to the v1 <code>GET /api/cluster/aliases
+ * </code> API.
+ */
+ @GET
+ @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML, BINARY_CONTENT_TYPE_V2})
+ @PermissionName(COLL_READ_PERM)
+ @Operation(
+ summary = "List the existing collection aliases.",
+ tags = {"aliases"})
+ public ListAliasesResponse getAliases() throws Exception {
+ recordCollectionForLogAndTracing(null, solrQueryRequest);
+
+ final ListAliasesResponse response = instantiateJerseyResponse(ListAliasesResponse.class);
+ final CoreContainer coreContainer = fetchAndValidateZooKeeperAwareCoreContainer();
+
+ ZkStateReader zkStateReader = coreContainer.getZkController().getZkStateReader();
+
+ // if someone calls listAliases, lets ensure we return an up to date response
+ zkStateReader.getAliasesManager().update();
+
+ Aliases aliases = zkStateReader.getAliases();
+
+ if (aliases != null) {
+ // the aliases themselves...
+ response.aliases = aliases.getCollectionAliasMap();
+ // Any properties for the above aliases.
+ Map<String, Map<String, String>> meta = new LinkedHashMap<>();
+ for (String alias : aliases.getCollectionAliasListMap().keySet()) {
+ Map<String, String> collectionAliasProperties = aliases.getCollectionAliasProperties(alias);
+ if (!collectionAliasProperties.isEmpty()) {
+ meta.put(alias, collectionAliasProperties);
+ }
+ }
+ response.properties = meta;
+ }
+
+ return response;
+ }
+
+ @GET
+ @Path("/{aliasName}")
+ @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML, BINARY_CONTENT_TYPE_V2})
+ @PermissionName(COLL_READ_PERM)
+ @Operation(
+ summary = "Get details for a specific collection alias.",
+ tags = {"aliases"})
+ public GetAliasByNameResponse getAliasByName(
+ @Parameter(description = "Alias name.", required = true) @PathParam("aliasName")
+ String aliasName)
+ throws Exception {
+ recordCollectionForLogAndTracing(null, solrQueryRequest);
+
+ final GetAliasByNameResponse response = instantiateJerseyResponse(GetAliasByNameResponse.class);
+ response.alias = aliasName;
+
+ final CoreContainer coreContainer = fetchAndValidateZooKeeperAwareCoreContainer();
+ ZkStateReader zkStateReader = coreContainer.getZkController().getZkStateReader();
+ // if someone calls listAliases, lets ensure we return an up to date response
Review Comment:
[-0] Small nit: copy/paste issue with 'listAliases'.
Relatedly though, maybe this code should be pulled into a private helper method that both `getAliasByName` and `getAliases` can call, to cut down on duplication.
--
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