-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
HLRC: migration get assistance API #32744
HLRC: migration get assistance API #32744
Conversation
The request and response classes have been extracted from `IndexUpgradeInfoAction` into top-level classes, and moved to the protocol jar. The `UpgradeActionRequired` enum is also moved. Relates to elastic#29827
Pinging @elastic/es-core-infra |
public IndexUpgradeInfoResponse getAssistance(IndexUpgradeInfoRequest request, RequestOptions options) throws IOException { | ||
return restHighLevelClient.performRequestAndParseEntity(request, RequestConverters::getMigrationAssistance, options, | ||
IndexUpgradeInfoResponse::fromXContent, Collections.emptySet()); | ||
} |
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 consciously did not add the async variant of this API, I do not think it's needed.
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 always thought we're better off being consistent but I'm fine with it this way.
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 initial idea was that not all the API will have the async method variant. We ended up adding it to almost all the methods, but I think this one is another where async is not really needed?
|
||
public class IndexUpgradeInfoRequest extends MasterNodeReadRequest<IndexUpgradeInfoRequest> implements IndicesRequest.Replaceable { | ||
|
||
private String[] indices = Strings.EMPTY_ARRAY; |
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 changed the initial value for the request so that it points by default to all indices, rather than throwing a validation exception because indices are null. I also moved the validation to the indices setter method.
e -> { | ||
@SuppressWarnings("unchecked") | ||
Map<String, Object> value = (Map<String, Object>) e.getValue(); | ||
return UpgradeActionRequired.fromString((String)value.get(ACTION_REQUIRED.getPreferredName())); |
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.
this is pretty ugly yet the structure of this response does not look very objectparser friendly, as it includes a couple of levels that need to be skipped to get to the important info that needs to be retrieved.
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.
declareNamedObjects
almost does this, but not quite because it is at the top level.
The request and response classes have been extracted from `IndexUpgradeInfoAction` into top-level classes, and moved to the protocol jar. The `UpgradeActionRequired` enum is also moved. Relates to elastic#29827
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 left a few comments, I think only the one about the validation actually needs addressing. The others are more just me being opinionated.
public IndexUpgradeInfoResponse getAssistance(IndexUpgradeInfoRequest request, RequestOptions options) throws IOException { | ||
return restHighLevelClient.performRequestAndParseEntity(request, RequestConverters::getMigrationAssistance, options, | ||
IndexUpgradeInfoResponse::fromXContent, Collections.emptySet()); | ||
} |
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 always thought we're better off being consistent but I'm fine with it this way.
public class TransportIndexUpgradeInfoAction extends TransportMasterNodeReadAction<IndexUpgradeInfoAction.Request, | ||
IndexUpgradeInfoAction.Response> { | ||
public class TransportIndexUpgradeInfoAction extends TransportMasterNodeReadAction<IndexUpgradeInfoRequest, | ||
IndexUpgradeInfoResponse> { |
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 liked the old indentation better!
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.
when are we going to have the automatic formatter so we stop arguing about this? :) I haven't even made these changes, my IDE made them.
} | ||
|
||
public IndexUpgradeInfoRequest(String... indices) { | ||
this.indices = indices; |
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.
This skips the validation in the setter method.
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.
good point, thanks
private String[] indices = Strings.EMPTY_ARRAY; | ||
private IndicesOptions indicesOptions = IndicesOptions.fromOptions(false, true, true, true); | ||
|
||
// for serialization |
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.
Do we need this any more? I think maybe we don't now that we're using the reading ctor.
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 think you are right
e -> { | ||
@SuppressWarnings("unchecked") | ||
Map<String, Object> value = (Map<String, Object>) e.getValue(); | ||
return UpgradeActionRequired.fromString((String)value.get(ACTION_REQUIRED.getPreferredName())); |
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.
declareNamedObjects
almost does this, but not quite because it is at the top level.
heya @nik9000 I pushed some updates, can you have another look please? |
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.
LGTM
retest this please make it green too |
…listeners * elastic/master: (58 commits) [ML] Partition-wise maximum scores (elastic#32748) [DOCS] XContentBuilder#bytes method removed, using BytesReference.bytes(docBuilder) (elastic#32771) HLRC: migration get assistance API (elastic#32744) Add a task to run forbiddenapis using cli (elastic#32076) [Kerberos] Add debug log statement for exceptions (elastic#32663) Make x-pack core pull transport-nio (elastic#32757) Painless: Clean Up Whitelist Names (elastic#32791) Cat apis: Fix index creation time to use strict date format (elastic#32510) Clear Job#finished_time when it is opened (elastic#32605) (elastic#32755) Test: Only sniff host metadata for node_selectors (elastic#32750) Update scripted metric docs to use `state` variable (elastic#32695) Painless: Clean up PainlessCast (elastic#32754) [TEST] Certificate NONE not allowed in FIPS JVM (elastic#32753) [ML] Refactor ProcessCtrl into Autodetect and Normalizer builders (elastic#32720) Access build tools resources (elastic#32201) Tests: Disable rolling upgrade tests with system key on fips JVM (elastic#32775) HLRC: Ban LoggingDeprecationHandler (elastic#32756) Fix test reproducability in AbstractBuilderTestCase setup (elastic#32403) Only require java<version>_home env var if needed Tests: Muted ScriptDocValuesDatesTests.testJodaTimeBwc ...
The request and response classes have been extracted from `IndexUpgradeInfoAction` into top-level classes, and moved to the protocol jar. The `UpgradeActionRequired` enum is also moved. Relates to #29827
* 6.x: (96 commits) Introduce global checkpoint listeners (#32696) Use JDK 10 for 6.4 BWC builds (#32866) Remove unused imports - follow up to removal of test in issue 32855 Removed flaky test. Looks like randomisation makes these assertions unreliable. This test is superfluous - it was added to address #32770 but it later turned out there was an existing test that just required a fix to provide the missing test coverage. [test] mute IndexShardTests.testDocStats Test: Fix forbidden uses in test framework (#32824) Security: remove password hash bootstrap check (#32440) Move validation to server for put user requests (#32471) [ML] Add high level REST client docs for ML put job endpoint (#32843) Painless: Change fqn_only to no_import (#32817) [test] mute testSearchWithSignificantTermsAgg Backport: CompletableContext class to avoid throwable (#32829) [TEST] Select free port for Minio (#32837) SCRIPTING: Support BucketAggScript return null (#32811) (#32833) HLRC: Add Delete License API (#32586) Aggregations/HL Rest client fix: missing scores (#32774) HLRC: migration get assistance API (#32744) Fix NOOP bulk updates (#32819) Increase logging testRetentionPolicyChangeDuringRecovery AwaitsFix case-functions.sql-spec ...
The request and response classes have been extracted from
IndexUpgradeInfoAction
into top-level classes, and moved to the protocol jar. TheUpgradeActionRequired
enum is also moved.Relates to #29827