feat(labels): Add labels for polaris entities#4146
feat(labels): Add labels for polaris entities#4146vignesh-manel wants to merge 6 commits intoapache:mainfrom
Conversation
|
Hi @vignesh-manel ! Thanks a lot for this! I took a brief look over the PR and I do not see anything that's technically worrisome. Sure, there are a few things, but those can be sorted out w/o a lot of hassle. As this is a new feature, I just wonder whether it's better to go on the dev-mailing-list and start a discussion about this new feature to gather the community's opinion and eventually consensus. WDYT? Robert |
@snazy Thanks for taking a look, I have posted it in dev mail thread: https://lists.apache.org/thread/2ny3gx01dx85b3bzo58zkbddryrjhfr3 |
dimas-b
left a comment
There was a problem hiding this comment.
The feature LGTM at high level 👍 ... posting a couple of preliminary comments.
There was a problem hiding this comment.
@vignesh-manel : If I may suggest moving CLI changes to a separate PR, I think it would simplify review (just different audiences usually review core and CLI changes).
There was a problem hiding this comment.
Thanks for the review.
Removed the CLI changes from this PR, will create a different PR for CLI, once this is merged.
spec/polaris-management-service.yml
Outdated
| $ref: "#/components/schemas/StorageConfigInfo" | ||
| labels: | ||
| type: object | ||
| x-extra-annotation: "@com.fasterxml.jackson.annotation.JsonInclude(com.fasterxml.jackson.annotation.JsonInclude.Include.NON_EMPTY)" |
There was a problem hiding this comment.
I believe it is preferable to keep the OpenAPI definition language-neutral.
I believe null values are already not included. Could we represent empty labels as null here?
Alternatively, it might be a good idea to make a separate change to avoid serializing empty values across all endpoints (cf. #1955, https://lists.apache.org/thread/nfboq6yo01x3lw4t3d5gp78f6lgs5ddz).
WDYT?
There was a problem hiding this comment.
removed this, now the response will have labels as empty map {} , similar to other collection type attributes in the APIs
| // Update with labels omitted (null) — existing labels must be preserved | ||
| try (Response response = | ||
| managementApi | ||
| .request("v1/catalogs/{name}", Map.of("name", catalogName)) |
There was a problem hiding this comment.
nit: maybe add a helper method to ManagementApi?
There was a problem hiding this comment.
added helper method updateCatalogLabels
|
|
||
| // Filter by env=prod — only prodCatalog should match | ||
| try (Response response = | ||
| managementApi.request("v1/catalogs", Map.of(), Map.of("labelFilter", "env=prod")).get()) { |
There was a problem hiding this comment.
nit: new helper method in ManagementApi?
There was a problem hiding this comment.
added helper method listCatalogsByLabel
| public static final String EMPTY_MAP_STRING = "{}"; | ||
|
|
||
| /** Internal-property key under which entity labels are stored as a serialized JSON map. */ | ||
| public static final String LABELS_INTERNAL_KEY = "polaris.entity.labels"; |
There was a problem hiding this comment.
protected seems sufficient... Let's not expand the public API surface in "core" unless really necessary.
| */ | ||
| @JsonIgnore | ||
| public Map<String, String> getLabels() { | ||
| String raw = getInternalPropertiesAsMap().get(LABELS_INTERNAL_KEY); |
There was a problem hiding this comment.
WDYT about modelling each label as a separate internal property? e.g. polaris.entity.label.env=dev?
| * matches every entity. | ||
| */ | ||
| @JsonIgnore | ||
| public boolean matchesLabelFilter(@Nonnull Map<String, String> labelFilter) { |
There was a problem hiding this comment.
nit: how about containsAllLabels? I think it would represent actual behaviour better.
There was a problem hiding this comment.
renamed to containsAllLabels
spec/polaris-management-service.yml
Outdated
| operationId: listCatalogs | ||
| description: List all catalogs in this polaris service | ||
| parameters: | ||
| - name: labelFilter |
There was a problem hiding this comment.
suggestion: rename to label and use : as the separator. A sample query could be curl -X GET http://.../catalogs?label=env:prod&label=team:eng... I reads nicer from my POV.
There was a problem hiding this comment.
thanks for this suggestion, this is better way, updated the code accordingly
| } | ||
| Map<String, String> result = new HashMap<>(labelFilter.size()); | ||
| for (String entry : labelFilter) { | ||
| int idx = entry.indexOf('='); |
There was a problem hiding this comment.
We never actually process a label's key/value parts individually, right? Do we need to parse it at all?
5f72825 to
c314e2c
Compare
Add key-value label support to Catalog entities for organizing and filtering catalogs via the management REST API.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)