Skip to content
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

Introduce private settings #33327

Merged
merged 13 commits into from Sep 3, 2018
Merged

Conversation

jasontedor
Copy link
Member

@jasontedor jasontedor commented Aug 31, 2018

This commit introduces the formal notion of a private setting. This enables us to register some settings that we had previously not registered as fully-fledged settings to avoid them being exposed via APIs such as the create index API. For example, we had hacks in the codebase to allow index.version.created to be passed around inside of settings objects, but was not registered as a setting so that if a user tried to use the setting on any API then they would get an exception. This prevented users from setting index.version.created on index creation, or updating it via the index settings API. By introducing private settings, we can continue to reject these attempts, yet now we can represent these settings as actual settings. In this change, we register index.version.created as an actual setting. We do not cutover all settings that we had been treating as private in this pull request, it is already quite large due to moving some tests around to account for the fact that some tests need to be able to set the index.version.created. This can be done in a follow-up change.

Relates #31286

This commit introduces the formal notion of a private setting. This
enables us to register some settings that we had previously not
registered as fully-fledged settings to avoid them being exposed via
APIs such as the create index API. For example, we had hacks in the
codebase to allow index.version.created to be passed around inside of
settings objects, but was not registered as a setting so that if a user
tried to use the setting on any API then they would get an
exception. This prevented users from setting index.version.created on
index creation, or updating it via the index settings API. By
introducing private settings, we can continue to reject these attempts,
yet now we can represent these settings as actual settings. In this
change, we register index.version.created as an actual setting. We do
not cutover all settings that we had been treating as private in this
pull request, it is already quite large due to moving some tests around
to account for the fact that some tests need to be able to set the
index.version.created. This can be done in a follow-up change.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jasontedor
Copy link
Member Author

jasontedor commented Aug 31, 2018

Note to my beloved reviewers: this pull request looks large because I moved a lot of tests that require setting index.version.created to separate classes so that I could enable only them to muck around with private settings yet leave other tests that do not need to from being able to do so. You can skim any test that is not in a settings test class (for example, look at the settings tests added in PrivateSettingsIT, but not in LegacyMapperServiceTests).

@jasontedor
Copy link
Member Author

jasontedor commented Aug 31, 2018

@dnhatn This will enable you to add settings validation that depends on index.version.created for the index.soft_deletes.enabled setting.

Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM left a minor nit.

@@ -473,6 +480,9 @@ public boolean isRelease() {
if ("CURRENT".equals(field.getName())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use switch / case here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed 635da8b.

@@ -200,7 +201,6 @@ public boolean isPrivateSetting(String key) {
switch (key) {
case IndexMetaData.SETTING_CREATION_DATE:
case IndexMetaData.SETTING_INDEX_UUID:
case IndexMetaData.SETTING_VERSION_CREATED:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you just converted one setting here to reduce the footprint right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup!

We do not cutover all settings that we had been treating as private in this pull request, it is already quite large due to moving some tests around to account for the fact that some tests need to be able to set the index.version.created. This can be done in a follow-up change.

* master: (197 commits)
  Prevent NPE parsing the stop datafeed request. (elastic#33347)
  HLRC: Add ML get overall buckets API (elastic#33297)
  Core: Fix epoch millis java time formatter (elastic#33302)
  [Docs] Improve tuning for speed advice (elastic#33315)
  [Rollup] Fix Caps Comparator to handle calendar/fixed time (elastic#33336)
  [CI] Mute  IndexShardTests#testIndexCheckOnStartup fails elastic#33345
  [CI] Mute LuceneChangesSnapshotTests#testUpdateAndReadChangesConcurrently
  Security for _field_names field should not override field statistics (elastic#33261)
  Add early termination support to BucketCollector (elastic#33279)
  Fix extractjar task  ci  (elastic#33272)
  Mute testFollowIndexAndCloseNode
  Logging: Drop Settings from some logging ctors (elastic#33332)
  HLREST: add update by query API (elastic#32760)
  TEST: Increase timeout testFollowIndexAndCloseNode (elastic#33333)
  HLRC: ML Flush job (elastic#33187)
  HLRC: Adding ML Job stats (elastic#33183)
  LLREST: Drop deprecated methods (elastic#33223)
  Mute testSyncerOnClosingShard
  [DOCS] Moves machine learning APIs to docs folder (elastic#31118)
  Mute test watcher usage stats output
  ...
@jasontedor jasontedor merged commit 09bf4e5 into elastic:master Sep 3, 2018
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 4, 2018
…e-default-distribution

* elastic/master: (213 commits)
  ML: Fix build after HLRC change
  Fix inner hits retrieval when stored fields are disabled (_none_) (elastic#33018)
  SQL: Show/desc commands now support table ids (elastic#33363)
  Mute testValidateFollowingIndexSettings
  HLRC: Add delete by query API (elastic#32782)
  [ML] The sort field on get records should default to the record_score (elastic#33358)
  [ML] Minor improvements to categorization Grok pattern creation (elastic#33353)
  [DOCS] fix a couple of typos (elastic#33356)
  Disable assemble task instead of removing it (elastic#33348)
  Simplify the return type of FieldMapper#parse. (elastic#32654)
  [ML] Delete forecast API (elastic#31134) (elastic#33218)
  Introduce private settings (elastic#33327)
  [Docs] Add search timeout caveats (elastic#33354)
  TESTS: Fix Race Condition in Temp Path Creation (elastic#33352)
  Fix from_range in search_after in changes snapshot (elastic#33335)
  TESTS+DISTR.: Fix testIndexCheckOnStartup Flake (elastic#33349)
  Null completion field should not throw IAE (elastic#33268)
  Adds code to help with IndicesRequestCacheIT failures (elastic#33313)
  Prevent NPE parsing the stop datafeed request. (elastic#33347)
  HLRC: Add ML get overall buckets API (elastic#33297)
  ...
jasontedor added a commit that referenced this pull request Sep 4, 2018
This commit introduces the formal notion of a private setting. This
enables us to register some settings that we had previously not
registered as fully-fledged settings to avoid them being exposed via
APIs such as the create index API. For example, we had hacks in the
codebase to allow index.version.created to be passed around inside of
settings objects, but was not registered as a setting so that if a user
tried to use the setting on any API then they would get an
exception. This prevented users from setting index.version.created on
index creation, or updating it via the index settings API. By
introducing private settings, we can continue to reject these attempts,
yet now we can represent these settings as actual settings. In this
change, we register index.version.created as an actual setting. We do
not cutover all settings that we had been treating as private in this
pull request, it is already quite large due to moving some tests around
to account for the fact that some tests need to be able to set the
index.version.created. This can be done in a follow-up change.
@jasontedor jasontedor deleted the private-index-settings branch September 4, 2018 21:57
@colings86 colings86 removed the v7.0.0 label Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants