Add to and from XContent to ClusterBlock and ClusterBlocks by shiv0408 · Pull Request #13694 · opensearch-project/OpenSearch

shiv0408 · 2024-05-15T22:24:03Z

Description

We need to upload the ClusterBlocks object to remote to enable the cluster state publication flow from remote. To do that we need to parse ClusterBlocks object to and from XContent.

Related Issues

Resolves #13692

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
Commits are signed per the DCO using --signoff
~~- [x] Commit changes are listed out in CHANGELOG.md file (See: Changelog)~~
Public documentation issue/PR created

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2024-05-15T22:33:02Z

❌ Gradle check result for 75ce997: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-05-15T23:23:06Z

❌ Gradle check result for 215a135: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

shiv0408 · 2024-05-16T00:16:17Z

Created a new issue for unrealted failing test org.opensearch.common.cache.stats.DefaultCacheStatsHolderTests.testConcurrentRemoval - #13695

github-actions · 2024-05-16T01:05:03Z

❌ Gradle check result for 141b86b: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

shiv0408 · 2024-05-16T06:54:59Z

Failing test already identified as flaky
org.opensearch.remotestore.RemoteStoreStatsIT.testDownloadStatsCorrectnessSinglePrimaryMultipleReplicaShards - #10152

github-actions · 2024-05-16T13:14:06Z

❌ Gradle check result for 141b86b: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-05-30T11:25:35Z

❌ Gradle check result for ee86138: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-05-30T13:53:58Z

✅ Gradle check result for ba27fd1: SUCCESS

codecov · 2024-05-30T13:55:57Z

Codecov Report

Attention: Patch coverage is 83.92857% with 18 lines in your changes missing coverage. Please review.

Project coverage is 71.56%. Comparing base (b15cb0c) to head (ba27fd1).
Report is 348 commits behind head on main.

❗ Current head ba27fd1 differs from pull request most recent head d69ae20

Please upload reports for the commit d69ae20 to get more accurate results.

Files	Patch %	Lines
...ava/org/opensearch/cluster/block/ClusterBlock.java	79.62%	6 Missing and 5 partials ⚠️
...va/org/opensearch/cluster/block/ClusterBlocks.java	87.71%	1 Missing and 6 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #13694      +/-   ##
============================================
+ Coverage     71.42%   71.56%   +0.14%     
- Complexity    59978    61136    +1158     
============================================
  Files          4985     5059      +74     
  Lines        282275   287628    +5353     
  Branches      40946    41671     +725     
============================================
+ Hits         201603   205829    +4226     
- Misses        63999    64761     +762     
- Partials      16673    17038     +365

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

server/src/main/java/org/opensearch/cluster/block/ClusterBlock.java

alchemist51 · 2024-06-03T11:47:17Z

server/src/main/java/org/opensearch/cluster/block/ClusterBlock.java

            builder.value(level.name().toLowerCase(Locale.ROOT));
        }
        builder.endArray();
+        if (context == Metadata.XContentContext.GATEWAY) {


Can this cause BWC failures for non-remote cluster?

We are only passing GATEWAY as a context when custom metadata should be stored as part of the persistent cluster state. Don't see any reason this could cause any BWC issue.

Is there a possibility of mismatch of nodes with old and new version of this code? (say OpenSearch 2.14 upgrading to 2.15). In that case, this additional fields (introduced in 2.15) can't be read by older nodes running in 2.14.

The XContent we are creating is not being sent to the nodes of older version. We will just be uploading this to remote repo. In case of upgrade, we will start publication of cluster state to nodes, when all the nodes have moved to 2.15. So, this would not cause any problem imo

Would it not be consumed by the data nodes from remote?

alchemist51 · 2024-06-03T11:50:02Z

server/src/main/java/org/opensearch/cluster/block/ClusterBlock.java

+        return new ClusterBlock(id, uuid, description, retryable, disableStatePersistence, allowReleaseResources, status, levels);
+    }
+
+    private static String skipBlockID(XContentParser parser) throws IOException {


Can we add explanation of why this is needed? Probably an example will help here

I have written the fromXContent is a way that if it can parse the ClusterBlock as a XContentFragment as well as a complete Object. This specific method is added to enable the parsing ClusterBlock as a complete XContentObject like given below.

{ "block-1" : { "uuid": .... } }

Although, we might never need to parse such object. But the UT I have written in ClusterBlockTests.java, we are creating a complete XContentObject from cluster block and trying to parse it back to ClusterBlock object, that is the reason this was required.

How do we differentiate between XContentObject and XConentFragment? Can we separate XContentObject parsing to a separate function and then use the above method to parse the XConentFragment inside that?

XContentObject has a paired start end object. While for fragment that is not true. Sure, we can separate the logic to parse the fragment to a innerParser and use that in fromXContent method which can accept both Object and a Fragment.

server/src/main/java/org/opensearch/cluster/block/ClusterBlock.java

server/src/main/java/org/opensearch/cluster/block/ClusterBlocks.java

server/src/main/java/org/opensearch/cluster/block/ClusterBlock.java

Signed-off-by: Shivansh Arora <hishiv@amazon.com>

github-actions · 2024-06-05T21:50:07Z

❌ Gradle check result for dd7babd: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: Shivansh Arora <hishiv@amazon.com>

github-actions · 2024-06-05T22:54:05Z

❌ Gradle check result for d69ae20: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

shiv0408 · 2024-06-06T07:04:19Z

❌ Gradle check result for d69ae20: FAILURE

#11979

backslasht · 2024-06-06T04:00:06Z

server/src/main/java/org/opensearch/cluster/block/ClusterBlock.java

            builder.value(level.name().toLowerCase(Locale.ROOT));
        }
        builder.endArray();
+        if (context == Metadata.XContentContext.GATEWAY) {


Is there a possibility of mismatch of nodes with old and new version of this code? (say OpenSearch 2.14 upgrading to 2.15). In that case, this additional fields (introduced in 2.15) can't be read by older nodes running in 2.14.

backslasht · 2024-06-06T04:29:07Z

server/src/main/java/org/opensearch/cluster/block/ClusterBlock.java

+            if (token == XContentParser.Token.FIELD_NAME) {
+                currentFieldName = parser.currentName();
+            } else if (token.isValue()) {
+                switch (Objects.requireNonNull(currentFieldName)) {


currentFiledName will always be non null after first field. For this logic to work, currentFieldName should be reset, once the corresponding value is parsed.

Yeah, it will always be non Null. I think I will get rid of this nonNull check rather than resetting the field every time we parse something.

backslasht · 2024-06-06T07:24:09Z

server/src/main/java/org/opensearch/cluster/block/ClusterBlock.java

+        return new ClusterBlock(id, uuid, description, retryable, disableStatePersistence, allowReleaseResources, status, levels);
+    }
+
+    private static String skipBlockID(XContentParser parser) throws IOException {


How do we differentiate between XContentObject and XConentFragment? Can we separate XContentObject parsing to a separate function and then use the above method to parse the XConentFragment inside that?

shiv0408 · 2024-06-07T19:10:24Z

#13576 (comment)

shiv0408 requested review from Bukhtawar, CEHENKLE, Rishikesh1159, VachaShah, anasalkouz, andrross, dblock, dbwiddis, dreamer-89, gbbafna, kotwanikunal, mch2, msfroh, nknize, owaiskazi19, reta, sachinpkale, saratvemulapalli, shwetathareja, sohami and tlfeng as code owners May 15, 2024 22:24

github-actions bot added Cluster Manager enhancement Enhancement or improvement to existing feature or request labels May 15, 2024

shiv0408 added the skip-changelog label May 15, 2024

shiv0408 force-pushed the cluster_blocks_xcontent branch from 75ce997 to 215a135 Compare May 15, 2024 22:50

shiv0408 force-pushed the cluster_blocks_xcontent branch from 215a135 to 141b86b Compare May 16, 2024 00:17

shiv0408 force-pushed the cluster_blocks_xcontent branch from 141b86b to ee86138 Compare May 30, 2024 11:15

shiv0408 force-pushed the cluster_blocks_xcontent branch from ee86138 to ba27fd1 Compare May 30, 2024 12:45

alchemist51 reviewed Jun 3, 2024

View reviewed changes

server/src/main/java/org/opensearch/cluster/block/ClusterBlock.java Show resolved Hide resolved

alchemist51 reviewed Jun 3, 2024

View reviewed changes

soosinha reviewed Jun 5, 2024

View reviewed changes

Add to and from XContent to ClusterBlock and ClusterBlocks

251daa9

Signed-off-by: Shivansh Arora <hishiv@amazon.com>

shiv0408 force-pushed the cluster_blocks_xcontent branch from ba27fd1 to dd7babd Compare June 5, 2024 21:39

Address PR comments

d69ae20

Signed-off-by: Shivansh Arora <hishiv@amazon.com>

shiv0408 force-pushed the cluster_blocks_xcontent branch from dd7babd to d69ae20 Compare June 5, 2024 22:18

backslasht suggested changes Jun 6, 2024

View reviewed changes

shiv0408 closed this Jun 7, 2024

shiv0408 deleted the cluster_blocks_xcontent branch June 7, 2024 19:11

opensearch-ci-bot mentioned this pull request Jun 13, 2024

[AUTOCUT] Gradle Check Flaky Test Report for QueueResizableOpenSearchThreadPoolExecutorTests #14297

Open

Conversation

shiv0408 commented May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

github-actions bot commented May 15, 2024

Uh oh!

github-actions bot commented May 15, 2024

Uh oh!

shiv0408 commented May 16, 2024

Uh oh!

github-actions bot commented May 16, 2024

Uh oh!

shiv0408 commented May 16, 2024

Uh oh!

github-actions bot commented May 16, 2024

Uh oh!

github-actions bot commented May 30, 2024

Uh oh!

github-actions bot commented May 30, 2024

Uh oh!

codecov bot commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jun 5, 2024

Uh oh!

github-actions bot commented Jun 5, 2024

Uh oh!

shiv0408 commented Jun 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shiv0408 commented Jun 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shiv0408 commented May 15, 2024 •

edited

Loading

codecov bot commented May 30, 2024 •

edited

Loading