-
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
Implemented XContent serialisation for GetIndexResponse #31675
Conversation
Pinging @elastic/es-core-infra |
@elasticmachine test this please |
add to whitelist |
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 question inline about one part of the change. The rest looks right to me, I just don't know the particulars well enough to be sure about one thing so I left a question about it. The rest looks right to me.
Do you want to backport this to 6.x so you can backport #31703 to 6.4?
for (String index : concreteIndices) { | ||
Settings indexSettings = state.metaData().index(index).getSettings(); | ||
if (request.humanReadable()) { | ||
indexSettings = IndexMetaData.addHumanReadableSettings(indexSettings); | ||
} | ||
settingsMapBuilder.put(index, indexSettings); | ||
if (request.includeDefaults()) { | ||
Settings defaultIndexSettings = | ||
settingsFilter.filter(indexScopedSettings.diff(indexSettings, Settings.EMPTY)); |
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 replacing this: indexScopedSettings.diff(settings, RestGetIndicesAction.this.settings)
which doesn't look 100% the same. It feels like EMPTY
should be the cluster settings.
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.
To be honest I am not entirely sure about this either. I was following the example 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.
Ok. I'll play with it a little myself and see if I can prove to myself it doesn't matter. Or that it does.
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.
OK! That was pretty quick! I think this change is ok because of this line.
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 for the explanation!
public int hashCode() { | ||
return | ||
(31 * Arrays.hashCode(indices)) + | ||
Objects.hash( |
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 believe it should be safe to do Objects.hash(Arrays.hashCode(indices), aliases, mappings, settings, defaultSettings)
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 believe so as well. I don't remember why I did it like this anymore. I will make the change.
++ |
This commit does the all the server side work adding the Get Index API to the REST high-level-client. A follow up would be the client side changes
-- will be reenabled after backport
After changing the version to 6.4. The mixed-cluster-tests failed as expected. I believe the following steps need to be taken now:
@nik9000 My question is what would happen if some other PR gets a green build with the BWC checks disabled before step 3 is completed. Will that be merged? |
Yup. This is a "one person at a time" process. |
This PR does the server side work for adding the Get Index API to the REST high-level-client, namely moving resolving default settings to the transport action. A follow up would be the client side changes.
-- It was disabled by elastic#31675
thanks a lot for working on this @sohaibiftikhar , great work! |
* 6.x: Fix not waiting for Netty ThreadDeathWatcher in IT (#31758) (#31789) [Docs] Correct default window_size (#31582) S3 fixture should report 404 on unknown bucket (#31782) [ML] Limit ML filter items to 10K (#31731) Fixture for Minio testing (#31688) [ML] Return statistics about forecasts as part of the jobsstats and usage API (#31647) [DOCS] Add missing get mappings docs to HLRC (#31765) [DOCS] Starting Elasticsearch (#31701) Fix coerce validation_method in GeoBoundingBoxQueryBuilder (#31747) Painless: Complete Removal of Painless Type (#31699) Consolidate watcher setting update registration (#31762) [DOCS] Adds empty 6.3.1 release notes page ingest: Introduction of a bytes processor (#31733) [test] don't run bats tests for suse boxes (#31749) Add analyze API to high-level rest client (#31577) Implemented XContent serialisation for GetIndexResponse (#31675) [DOCS] Typos DOC: Add examples to the SQL docs (#31633) Add support for AWS session tokens (#30414) Watcher: Reenable start/stop yaml tests (#31754) JDBC: Fix stackoverflow on getObject and timestamp conversion (#31735) Support multiple system store types (#31650) Add write*Blob option to replace existing blob (#31729) Split CircuitBreaker-related tests (#31659) Painless: Add Context Docs (#31190) Docs: Remove missing reference Migrate scripted metric aggregation scripts to ScriptContext design (#30111) Watcher: Fix chain input toXcontent serialization (#31721) Remove _all example (#31711) rest-high-level: added get cluster settings (#31706) Docs: Match the examples in the description (#31710) [Docs] Correct typos (#31720) Extend allowed characters for grok field names (#21745) (#31653) (#31722) [DOCS] Check for Windows and *nix file paths (#31648) [ML] Validate ML filter_id (#31535) Fix gradle4.8 deprecation warnings (#31654) Update numbers to reflect 4-byte UTF-8-encoded characters (#27083)
* master: [ML] Rate limit established model memory updates (#31768) [Docs] Correct default window_size (#31582) S3 fixture should report 404 on unknown bucket (#31782) Detach Transport from TransportService (#31727) [ML] Limit ML filter items to 10K (#31731) [ML] Return statistics about forecasts as part of the jobsstats and usage API (#31647) Fixture for Minio testing (#31688) [DOCS] Add missing get mappings docs to HLRC (#31765) [DOCS] Starting Elasticsearch (#31701) Painless: Complete Removal of Painless Type (#31699) Fix not waiting for Netty ThreadDeathWatcher in IT (#31758) Consolidate watcher setting update registration (#31762) Build: re-enabled bwc (#31769) ingest: Introduction of a bytes processor (#31733) Fix coerce validation_method in GeoBoundingBoxQueryBuilder (#31747) Add analyze API to high-level rest client (#31577) [DOCS] Typos DOC: Add examples to the SQL docs (#31633) Add support for AWS session tokens (#30414) Watcher: Reenable start/stop yaml tests (#31754) Implemented XContent serialisation for GetIndexResponse (#31675) JDBC: Fix stackoverflow on getObject and timestamp conversion (#31735) resolveHasher defaults to NOOP (#31723) Account for XContent overhead in in-flight breaker Split CircuitBreaker-related tests (#31659) Add write*Blob option to replace existing blob (#31729) Painless: Add Context Docs (#31190) Watcher: Fix chain input toXcontent serialization (#31721) Docs: Match the examples in the description (#31710) rest-high-level: added get cluster settings (#31706) [Docs] Correct typos (#31720) Clean up double semicolon code typos (#31687) [DOCS] Check for Windows and *nix file paths (#31648) [ML] Validate ML filter_id (#31535) Revert long lines Fix TransportChangePasswordActionTests
This PR does the server side work for adding the Get Index API to the REST high-level-client.
A follow up would be the client side changes.