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.
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.
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.
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.
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.
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.
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
nik9000
left a comment
There was a problem hiding this comment.
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.
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.
I liked the old indentation better!
There was a problem hiding this comment.
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.
This skips the validation in the setter method.
| private String[] indices = Strings.EMPTY_ARRAY; | ||
| private IndicesOptions indicesOptions = IndicesOptions.fromOptions(false, true, true, true); | ||
|
|
||
| // for serialization |
There was a problem hiding this comment.
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.
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.
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? |
|
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
IndexUpgradeInfoActioninto top-level classes, and moved to the protocol jar. TheUpgradeActionRequiredenum is also moved.Relates to #29827