-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add to and from XContent to ClusterBlock and ClusterBlocks #13694
base: main
Are you sure you want to change the base?
Conversation
❌ 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? |
75ce997
to
215a135
Compare
❌ 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? |
Created a new issue for unrealted failing test |
215a135
to
141b86b
Compare
❌ 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? |
Failing test already identified as flaky |
❌ 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? |
141b86b
to
ee86138
Compare
❌ 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? |
Signed-off-by: Shivansh Arora <hishiv@amazon.com>
ee86138
to
ba27fd1
Compare
Codecov ReportAttention: Patch coverage is
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. |
} | ||
|
||
private static String skipBlockID(XContentParser parser) throws IOException { | ||
if (parser.currentToken() == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what cases, we need this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answered in below comment.
for (ClusterBlockLevel level : levels) { | ||
builder.value(level.name().toLowerCase(Locale.ROOT)); | ||
} | ||
builder.endArray(); | ||
if (context == Metadata.XContentContext.GATEWAY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this cause BWC failures for non-remote cluster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
return new ClusterBlock(id, uuid, description, retryable, disableStatePersistence, allowReleaseResources, status, levels); | ||
} | ||
|
||
private static String skipBlockID(XContentParser parser) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add explanation of why this is needed? Probably an example will help here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
- [x] Commit changes are listed out in CHANGELOG.md file (See: Changelog)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.