-
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
[Backport 2.x] Split the remote global metadata file to metadata attribute files #13703
base: 2.x
Are you sure you want to change the base?
[Backport 2.x] Split the remote global metadata file to metadata attribute files #13703
Conversation
❌ Gradle check result for 57dc35c: 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? |
57dc35c
to
4f9e367
Compare
…ensearch-project#12190) * Split the cluster state remote global metadata file to metadata attribute files Signed-off-by: Shivansh Arora <hishiv@amazon.com> (cherry picked from commit da3ab92)
Signed-off-by: Shivansh Arora <hishiv@amazon.com>
4f9e367
to
d7fd2f8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.x #13703 +/- ##
============================================
- Coverage 71.28% 71.24% -0.05%
- Complexity 60145 61325 +1180
============================================
Files 4957 5036 +79
Lines 282799 288517 +5718
Branches 41409 42152 +743
============================================
+ Hits 201591 205540 +3949
- Misses 64189 65661 +1472
- Partials 17019 17316 +297 ☔ View full report in Codecov by Sentry. |
❕ Gradle check result for d7fd2f8: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
❌ Gradle check result for abe93ab: 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? |
return parser.namedObject(Custom.class, name, null); | ||
} | ||
|
||
static Custom fromXContent(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.
@shiv0408 why breaking changes check fails
There are existing classes with existing static method signatures, that implement Metadata.Custom
, just one of the examples:
public class ComposableIndexTemplateMetadata implements Metadata.Custom {
....
public static ComposableIndexTemplateMetadata fromXContent(XContentParser parser) throws IOException {
return PARSER.parse(parser, null);
}
}
Now you are trying to add the conflicting static methods to the Metadata.Custom
, changing the existing static method signatures.
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.
Thanks @reta for highlighting this. This method was added to be able to access fromXContent from the interface itself. As it is conflicting with other concrete classes method, needs to be fixed before attempting to backport.
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.
@reta @shwetathareja I agree that this method is a little confusing, as this method was not getting used anywhere I have removed this for now. But the breaking changes check is still failing due to the other method that is added above, which is not conflicting with any method in implementations of this interface.
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.
If we merge this backport PR, will raise a separate PR to remove the method and backport that to 2.x.
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.
static Custom fromXContent(XContentParser parser, String name) throws IOException {
// handling any Exception is caller's responsibility
return parser.namedObject(Custom.class, name, null);
}
This has default implementation and shouldn't be considered as breaking change for plugins right?
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.
The baseline is the released 2.14 artifact. If we merge this then I believe the compatibility check will fail for all 2.x PRs as the code on this branch will be flagged as incompatible with 2.14. The options to unblock here that I see are:
- Work around the bug by moving the static method
fromXContent(XContentParser, String)
from the interface to somewhere else (such as in the Metadata class). - Disable the compatibility check
- Figure out how to get the latest version of the japicmp library to be used by our github action, which should fix the false positive 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.
@shwetathareja To get unblocked, could you try to update the version of siom79/japicmp
and then re-run the compability check command. If that works, then lets include the version bump in this PR and forward port to main?
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.
@peternied The issue is the gradle plugin we're using has not been updated to pull in the latest version of the underlying japicmp library.
The plugin code has been updated, but no release has been made yet.
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've created an issue on japicmp-gradle-plugin to release a new version [1], it has the updated dependency but hasn't released. To workaround add a implementation
dependency in gradle for the updated version of japicmp.
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.
@andrross I wouldnt prefer making a code change just to override japicmp issue.
Signed-off-by: Shivansh Arora <hishiv@amazon.com>
❌ Gradle check result for 1c47c99: 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>
Backport da3ab92, 2e49743 from #12190, #13869.