Add immutable 'operator' metadata classes for cluster state #87763

grcevski · 2022-06-16T19:08:12Z

This PR relates to #86224 (Operator/File cluster settings) where we need to store various information in the Metadata about the most recent processing of the operator cluster state update.

Since the original PR is too large on its own, I'm separating the individual components of the PR for easier review.

elasticmachine · 2022-06-16T19:08:15Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

rjernst

Thanks for splitting this out. Overall the use of records is nice here, and I appreciate all the javadocs! I left some questions. The downside of the way the PR is split is I can't tell how any of these classes are intended to be used, so it is difficult to comment on particular methods and whether they could be simplified. I'm not sure how to remedy that...

rjernst · 2022-06-22T04:23:50Z

server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java

+     * @param namespace the namespace to lookup operator metadata for
+     * @return {@link OperatorMetadata} or null if not found
+     */
+    public OperatorMetadata operatorMetadata(String namespace) {


Why have this method and also the one above returning the entire map? If the one above is needed, then it could also be used in place of this method?

rjernst · 2022-06-22T04:27:11Z

server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java

@@ -1146,6 +1181,7 @@ public Metadata apply(Metadata part) {
            builder.indices(indices.apply(part.indices));
            builder.templates(templates.apply(part.templates));
            builder.customs(customs.apply(part.customs));
+            builder.put(operatorMetadata.apply(part.operatorMetadata));


Does the jdk map diff return an immutable/unmodifiable map? We could either wrap or copy this here to make it immutable, or even better open a separate issue to make the jdk map diff's apply return an immutable map (I've had the need for this in recent hppc PRs)

I made an issue here #88019. In the interim until that's fixed I'll wrap this in an unmodifiable map.

rjernst · 2022-06-22T04:28:03Z

server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java

+         * @param operatorMetadata a map of namespace to {@link OperatorMetadata}
+         * @return {@link Builder}
+         */
+        public Builder put(Map<String, OperatorMetadata> operatorMetadata) {


Is this used anywhere?

Ah yeah, I used to call this method putOperatorState, but it was too long and it didn't match the naming of other methods we have in Metadata, we simply call .put with different types. Here's the usage here:

https://github.com/elastic/elasticsearch/pull/86224/files#diff-8ca6818d706ec1720e045ea8a1f4f8e4b5dda47c5610aed2bf8e7d55121d8391R129

rjernst · 2022-06-22T04:30:05Z

server/src/main/java/org/elasticsearch/cluster/metadata/OperatorErrorMetadata.java

+ */
+public record OperatorErrorMetadata(
+    Long version,
+    org.elasticsearch.cluster.metadata.OperatorErrorMetadata.ErrorKind errorKind,


nit: static import ErrorKind?

rjernst · 2022-06-22T04:33:05Z

server/src/main/java/org/elasticsearch/cluster/metadata/OperatorErrorMetadata.java

+     * @throws IOException
+     */
+    public static OperatorErrorMetadata readFrom(StreamInput in) throws IOException {
+        Builder builder = new Builder().version(in.readLong())


Why create a builder, these can be passed to the canonical ctor?

rjernst · 2022-06-22T04:34:11Z

server/src/main/java/org/elasticsearch/cluster/metadata/OperatorErrorMetadata.java

+         * @param metadata {@link OperatorErrorMetadata}
+         * @param builder {@link XContentBuilder}
+         */
+        public static void toXContent(OperatorErrorMetadata metadata, XContentBuilder builder) throws IOException {


This could be a member method of the record?

rjernst · 2022-06-22T04:34:45Z

server/src/main/java/org/elasticsearch/cluster/metadata/OperatorErrorMetadata.java

+         * @return {@link OperatorErrorMetadata}
+         * @throws IOException
+         */
+        public static OperatorErrorMetadata fromXContent(XContentParser parser) throws IOException {


Shouldn't this be on the record, not the builder?

Actually for this one is a lot more convenient to be a builder method, we are iterating over the parser elements and we fill in the pieces of data we parse inside the builder. I had made the toXContent part of the builder too since they share the static strings, but I can remove that wrapping.

rjernst · 2022-06-22T04:36:14Z

server/src/main/java/org/elasticsearch/cluster/metadata/OperatorHandlerMetadata.java

+         * @param metadata {@link OperatorHandlerMetadata}
+         * @param builder {@link XContentBuilder}
+         */
+        public static void toXContent(OperatorHandlerMetadata metadata, XContentBuilder builder) throws IOException {


Why is toXContent needed on both the builder (as a static that takes the record?) and on the record itself?

I'll move it to the record only, similar to other places, I'm sharing the string constants for to and from xcontent.

rjernst · 2022-06-22T04:38:45Z

server/src/main/java/org/elasticsearch/cluster/metadata/OperatorMetadata.java

+
+            String currentFieldName = skipNamespace(parser);
+            XContentParser.Token token;
+            while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {


Could a ConstructingObjectParser be used instead of this loop (same goes for other fromXContent in this PR)? If using that, would a builder even be necessary?

I'll look into that, it would be great if we can.

grcevski · 2022-06-24T14:55:50Z

Thanks for splitting this out. Overall the use of records is nice here, and I appreciate all the javadocs! I left some questions. The downside of the way the PR is split is I can't tell how any of these classes are intended to be used, so it is difficult to comment on particular methods and whether they could be simplified. I'm not sure how to remedy that...

Yeah, I figured it will be super hard without context. I'll start adding comments on various PR lines with context from the original PR in how it's used. I hope that will help.

grcevski · 2022-06-24T20:29:51Z

server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java

@@ -942,6 +947,15 @@ public Map<String, Custom> customs() {
        return this.customs;
    }

+    /**


Here's an example in the related PR of how this will be used:

https://github.com/elastic/elasticsearch/pull/86224/files#diff-d017ebae07494c580da12a539a666fe1d87537f167e71761cf60451b8913d554R127

The related PR is out of date, it calls the API operatorState and it has the extra helper method to perform the additional .get.

Instead of:

OperatorMetadata existingMetadata = state.metadata().operatorState(namespace);

we'll be calling:

OperatorMetadata existingMetadata = state.metadata().operatorMetadata.get(namespace);

grcevski · 2022-06-24T20:36:41Z

server/src/main/java/org/elasticsearch/cluster/metadata/OperatorErrorMetadata.java

+ * This information is held by the {@link OperatorMetadata} class.
+ * </p>
+ */
+public record OperatorErrorMetadata(Long version, ErrorKind errorKind, List<String> errors)


The error metadata is set when a file fails to get processed for some reason. In this case we launch a cluster state update task that will update this metadata. Example here:

https://github.com/elastic/elasticsearch/pull/86224/files#diff-a7da9bd5e6ce66769b5113745baa42c88b7c21b916a8ea290434048fc5af2c5fR29

The builder code is now removed, so the OperatorErrorMetadata will be directly created.

grcevski · 2022-06-24T20:40:10Z

server/src/main/java/org/elasticsearch/cluster/metadata/OperatorHandlerMetadata.java

+ * multiple namespaces. See {@link OperatorMetadata} and {@link Metadata}.
+ * </p>
+ */
+public record OperatorHandlerMetadata(String name, Set<String> keys)


The OperatorHandlerMetadata is stored when we successfully process a new operator cluster state. Here's an example usage from the task that does the final update of the cluster state:

https://github.com/elastic/elasticsearch/pull/86224/files#diff-8ca6818d706ec1720e045ea8a1f4f8e4b5dda47c5610aed2bf8e7d55121d8391R40

specifically here:

https://github.com/elastic/elasticsearch/pull/86224/files#diff-8ca6818d706ec1720e045ea8a1f4f8e4b5dda47c5610aed2bf8e7d55121d8391R128

The only difference is that I renamed the API so instead of:

Metadata.Builder metadataBuilder = Metadata.builder(state.metadata()).putOperatorState(operatorMetadataBuilder.build());

we'll do:

Metadata.Builder metadataBuilder = Metadata.builder(state.metadata()).put(operatorMetadataBuilder.build());

grcevski · 2022-06-24T20:41:20Z

server/src/main/java/org/elasticsearch/cluster/metadata/OperatorMetadata.java

+ * and cannot be modified by the end user.
+ * </p>
+ */
+public record OperatorMetadata(


This is always used in the prior examples I posted for the OperatorErrorMetadata and OperatorHandlerMetadata. It's the top level objects that contains both, either or neither can be set.

grcevski · 2022-06-24T20:42:34Z

I think I applied all suggestions and I linked with the relevant code in the WIP PR. I hope it's sufficiently described for a review.

grcevski · 2022-06-24T20:54:21Z

@elasticmachine update branch

… into operator/metadata

rjernst

LGTM. Just a couple suggestions for avoiding TreeSets

rjernst · 2022-06-29T04:26:16Z

server/src/main/java/org/elasticsearch/cluster/metadata/OperatorHandlerMetadata.java

+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        builder.startObject(name());
+        builder.stringListField(KEYS.getPreferredName(), new TreeSet<>(keys())); // ordered set here to ensure output consistency


Building a TreeSet would seem more complicated and costly (lots of object creations) than simply sorting the keys?

rjernst · 2022-06-29T04:31:52Z

server/src/main/java/org/elasticsearch/cluster/metadata/OperatorMetadata.java

+        builder.startObject(namespace());
+        builder.field(VERSION.getPreferredName(), version);
+        builder.startObject(HANDLERS.getPreferredName());
+        var sortedKeys = new TreeSet<>(handlers.keySet());


Instead of using a TreeSet here to do the sorting, we can use the stream api?

handlers.entrySet().stream().sorted(Map.Entry.comparingByKey()).forEach(e -> { e.getValue().toXContent(builder, params); }

rjernst · 2022-06-29T13:45:34Z

server/src/main/java/org/elasticsearch/cluster/metadata/OperatorMetadata.java

+     *
+     * <p>
+     * These settings cannot be updated by the end user and are set outside of the
+     * REST layer, e.g. through file based settings or by plugin/modules.


I forgot to mention this naming issue. Since all of these classes are about more than operator settings, I think we need better terminology that applies to file based settings and programmatic fixed settings. I'm not sure what that naming should be, but as it is the "operator" terminology will be confusing when we start using this in plugins and modules.

Perhaps a name referencing cluster state. Some ideas:

unmodifiable cluster state immutable cluster state ....

So then this class might be ImmutableClusterStateMetadata. It's not the best, but it demonstrates the idea.

Good idea, I've been struggling with terminology on this. I like the immutable prefix, it's really what it is. Operator is just the mode we set them in initially.

grcevski · 2022-06-29T20:24:31Z

@elasticmachine update branch

Nikola Grcevski added 2 commits June 16, 2022 15:01

Add operator state metadata

930f777

Remove unused method

c3b87a0

grcevski added >non-issue :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team v8.4.0 labels Jun 16, 2022

grcevski mentioned this pull request Jun 16, 2022

[WIP] Operator and plugin read-only settings #86224

Closed

6 tasks

rjernst requested changes Jun 22, 2022

View reviewed changes

Merge branch 'master' into operator/metadata

c727334

Apply feedback

0b6aafe

grcevski commented Jun 24, 2022

View reviewed changes

grcevski requested a review from rjernst June 24, 2022 20:42

elasticmachine and others added 6 commits June 24, 2022 21:54

Merge branch 'master' into operator/metadata

46462d5

Remove closing paragraph

9b3bf18

Merge branch 'operator/metadata' of github.com:grcevski/elasticsearch…

444058e

… into operator/metadata

Merge branch 'master' into operator/metadata

a83ec5f

Fix compile error

6be97a9

Merge branch 'master' into operator/metadata

02fa602

rjernst approved these changes Jun 29, 2022

View reviewed changes

Merge branch 'master' into operator/metadata

90f812f

rjernst reviewed Jun 29, 2022

View reviewed changes

Nikola Grcevski added 2 commits June 29, 2022 13:10

Implement PR feedback

4bea906

Adjust tests for renamed field

4c07b5c

Merge branch 'master' into operator/metadata

4a96f57

grcevski changed the title ~~Add operator metadata classes for cluster state~~ Add immutable 'operator' metadata classes for cluster state Jun 29, 2022

grcevski merged commit e1d03ef into elastic:master Jun 29, 2022

grcevski deleted the operator/metadata branch June 29, 2022 22:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add immutable 'operator' metadata classes for cluster state #87763

Add immutable 'operator' metadata classes for cluster state #87763

grcevski commented Jun 16, 2022

elasticmachine commented Jun 16, 2022

rjernst left a comment

rjernst Jun 22, 2022

rjernst Jun 22, 2022

grcevski Jun 24, 2022

rjernst Jun 22, 2022

grcevski Jun 24, 2022

rjernst Jun 22, 2022

rjernst Jun 22, 2022

rjernst Jun 22, 2022

rjernst Jun 22, 2022

grcevski Jun 24, 2022

rjernst Jun 22, 2022

grcevski Jun 24, 2022

rjernst Jun 22, 2022

grcevski Jun 24, 2022

grcevski commented Jun 24, 2022

grcevski Jun 24, 2022

grcevski Jun 24, 2022

grcevski Jun 24, 2022

grcevski Jun 24, 2022

grcevski commented Jun 24, 2022

grcevski commented Jun 24, 2022

rjernst left a comment

rjernst Jun 29, 2022

rjernst Jun 29, 2022

rjernst Jun 29, 2022

rjernst Jun 29, 2022

grcevski Jun 29, 2022

grcevski commented Jun 29, 2022

Add immutable 'operator' metadata classes for cluster state #87763

Add immutable 'operator' metadata classes for cluster state #87763

Conversation

grcevski commented Jun 16, 2022

elasticmachine commented Jun 16, 2022

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grcevski commented Jun 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grcevski commented Jun 24, 2022

grcevski commented Jun 24, 2022

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grcevski commented Jun 29, 2022