From 8a456485c3c3e706f3b12b27bf03e4248218a7ca Mon Sep 17 00:00:00 2001 From: Kaushal Kumar Date: Wed, 3 Jul 2024 16:10:40 -0700 Subject: [PATCH] address comments Signed-off-by: Kaushal Kumar --- .../cluster/metadata/QueryGroup.java | 12 ++-- .../cluster/metadata/QueryGroupMetadata.java | 41 ++++++++------ .../org/opensearch/search/ResourceType.java | 2 +- .../metadata/QueryGroupMetadataTests.java | 56 +++++++++++++++++-- 4 files changed, 82 insertions(+), 29 deletions(-) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/QueryGroup.java b/server/src/main/java/org/opensearch/cluster/metadata/QueryGroup.java index 197c1e17e1f3b..beaab198073df 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/QueryGroup.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/QueryGroup.java @@ -11,7 +11,7 @@ import org.opensearch.cluster.AbstractDiffable; import org.opensearch.cluster.Diff; import org.opensearch.common.UUIDs; -import org.opensearch.common.annotation.PublicApi; +import org.opensearch.common.annotation.ExperimentalApi; import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; import org.opensearch.core.xcontent.ToXContentObject; @@ -37,10 +37,10 @@ * "updatedAt": 4513232415 * } */ -@PublicApi(since = "2.15") +@ExperimentalApi public class QueryGroup extends AbstractDiffable implements ToXContentObject { - public static final int MAX_CHARS_ALLOWED_IN_NAME = 50; + private static final int MAX_CHARS_ALLOWED_IN_NAME = 50; private final String name; private final String _id; private final ResiliencyMode resiliencyMode; @@ -154,7 +154,7 @@ public static QueryGroup fromXContent(final XContentParser parser) throws IOExce String fieldName = ""; // Map to hold resources final Map resourceLimits = new HashMap<>(); - while ((token = parser.nextToken()) != null) { + while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { fieldName = parser.currentName(); } else if (token.isValue()) { @@ -245,7 +245,7 @@ public static Builder builder() { * ENFORCED - means that it will never breach the assigned limits and will cancel as soon as the limits are breached * MONITOR - it will not cause any cancellation but just log the eligible task cancellations */ - @PublicApi(since = "2.15") + @ExperimentalApi public enum ResiliencyMode { SOFT("soft"), ENFORCED("enforced"), @@ -274,7 +274,7 @@ public static ResiliencyMode fromName(String s) { /** * Builder class for {@link QueryGroup} */ - @PublicApi(since = "2.15") + @ExperimentalApi public static class Builder { private String name; private String _id; diff --git a/server/src/main/java/org/opensearch/cluster/metadata/QueryGroupMetadata.java b/server/src/main/java/org/opensearch/cluster/metadata/QueryGroupMetadata.java index bd949cf64e8c0..79732bc505ee2 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/QueryGroupMetadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/QueryGroupMetadata.java @@ -16,7 +16,6 @@ import org.opensearch.core.common.Strings; import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; -import org.opensearch.core.xcontent.ConstructingObjectParser; import org.opensearch.core.xcontent.MediaTypeRegistry; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; @@ -47,22 +46,6 @@ public class QueryGroupMetadata implements Metadata.Custom { private final Map queryGroups; - @SuppressWarnings("unchecked") - static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( - "queryGroupsParser", - args -> new QueryGroupMetadata((Map) args[0]) - ); - - static { - PARSER.declareObjectArray(ConstructingObjectParser.constructorArg(), (p, c) -> { - Map queryGroupMap = new HashMap<>(); - while (p.nextToken() != XContentParser.Token.END_OBJECT) { - queryGroupMap.put(p.currentName(), QueryGroup.fromXContent(p)); - } - return queryGroupMap; - }, QUERY_GROUP_FIELD); - } - public QueryGroupMetadata(Map queryGroups) { this.queryGroups = queryGroups; } @@ -105,7 +88,29 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws } public static QueryGroupMetadata fromXContent(XContentParser parser) throws IOException { - return PARSER.parse(parser, null); + Map queryGroupMap = new HashMap<>(); + + if (parser.currentToken() == null) { + parser.nextToken(); + } + + if (parser.currentToken() != XContentParser.Token.START_OBJECT) { + throw new IllegalArgumentException( + "QueryGroupMetadata.fromXContent was expecting a { token but found : " + parser.currentToken() + ); + } + XContentParser.Token token = parser.currentToken(); + String fieldName = parser.currentName(); + while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { + if (token == XContentParser.Token.FIELD_NAME) { + fieldName = parser.currentName(); + } else { + QueryGroup queryGroup = QueryGroup.fromXContent(parser); + queryGroupMap.put(fieldName, queryGroup); + } + } + + return new QueryGroupMetadata(queryGroupMap); } @Override diff --git a/server/src/main/java/org/opensearch/search/ResourceType.java b/server/src/main/java/org/opensearch/search/ResourceType.java index 57140405ccf0b..fe5ce4dd2bb50 100644 --- a/server/src/main/java/org/opensearch/search/ResourceType.java +++ b/server/src/main/java/org/opensearch/search/ResourceType.java @@ -16,7 +16,7 @@ /** * Enum to hold the resource type */ -@PublicApi(since = "1.3") +@PublicApi(since = "2.x") public enum ResourceType { CPU("cpu"), MEMORY("memory"); diff --git a/server/src/test/java/org/opensearch/cluster/metadata/QueryGroupMetadataTests.java b/server/src/test/java/org/opensearch/cluster/metadata/QueryGroupMetadataTests.java index 4408d5a599399..d70a9ce5e10cd 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/QueryGroupMetadataTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/QueryGroupMetadataTests.java @@ -8,15 +8,46 @@ package org.opensearch.cluster.metadata; +import org.opensearch.cluster.Diff; +import org.opensearch.common.xcontent.json.JsonXContent; import org.opensearch.core.common.io.stream.NamedWriteableRegistry; -import org.opensearch.test.AbstractNamedWriteableTestCase; +import org.opensearch.core.common.io.stream.Writeable; +import org.opensearch.core.xcontent.XContentBuilder; +import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.search.ResourceType; +import org.opensearch.test.AbstractDiffableSerializationTestCase; +import java.io.IOException; import java.util.Collections; import java.util.Map; import static org.opensearch.cluster.metadata.QueryGroupTests.createRandomQueryGroup; -public class QueryGroupMetadataTests extends AbstractNamedWriteableTestCase { +public class QueryGroupMetadataTests extends AbstractDiffableSerializationTestCase { + + public void testToXContent() throws IOException { + long updatedAt = 1720047207; + QueryGroupMetadata queryGroupMetadata = new QueryGroupMetadata( + Map.of( + "ajakgakg983r92_4242", + new QueryGroup( + "test", + "ajakgakg983r92_4242", + QueryGroup.ResiliencyMode.ENFORCED, + Map.of(ResourceType.MEMORY, 0.5), + updatedAt + ) + ) + ); + XContentBuilder builder = JsonXContent.contentBuilder(); + builder.startObject(); + queryGroupMetadata.toXContent(builder, null); + builder.endObject(); + assertEquals( + "{\"ajakgakg983r92_4242\":{\"_id\":\"ajakgakg983r92_4242\",\"name\":\"test\",\"resiliency_mode\":\"enforced\",\"updatedAt\":1720047207,\"resourceLimits\":{\"memory\":0.5}}}", + builder.toString() + ); + } @Override protected NamedWriteableRegistry getNamedWriteableRegistry() { @@ -28,8 +59,25 @@ protected NamedWriteableRegistry getNamedWriteableRegistry() { } @Override - protected Class categoryClass() { - return QueryGroupMetadata.class; + protected Metadata.Custom makeTestChanges(Metadata.Custom testInstance) { + final QueryGroup queryGroup = createRandomQueryGroup("asdfakgjwrir23r25"); + final QueryGroupMetadata queryGroupMetadata = new QueryGroupMetadata(Map.of(queryGroup.get_id(), queryGroup)); + return queryGroupMetadata; + } + + @Override + protected Writeable.Reader> diffReader() { + return QueryGroupMetadata::readDiffFrom; + } + + @Override + protected Metadata.Custom doParseInstance(XContentParser parser) throws IOException { + return QueryGroupMetadata.fromXContent(parser); + } + + @Override + protected Writeable.Reader instanceReader() { + return QueryGroupMetadata::new; } @Override