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

Backport get settings API changes to 6.x #30494

Merged
merged 8 commits into from
May 17, 2018

Conversation

tomcallahan
Copy link
Contributor

@tomcallahan tomcallahan commented May 9, 2018

Backport PR for #29229

@colings86 colings86 added the :Core/Infra/Settings Settings infrastructure and APIs label May 11, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@javanna
Copy link
Member

javanna commented May 11, 2018

hi @tomcallahan I guess what differs in this PR compared to the one that was merged in master is the versions in the bw comp for the changes we made to transport serialization? Anything else?

@tomcallahan
Copy link
Contributor Author

Yes @javanna - I wanted to be sure I had things correct.

That said, I actually think we may have some issue here and I could use your help. This is failing the bwc tests. It occurs to me that in a mixed cluster, we may not be able to support the include_defaults flag with the new transport-layer-based design. If the node receiving the query is a new node, it will not perform the defaults processing in the rest layer. If the TransportGetSettingsAction runs on an older node, it will not perform the defaults processing. Therefore the defaults will never be returned to the user. I am thinking I now may need to change the rest action in this backported version to provide that functionality if it is not coming back in the GetSettingsResponse - WDYT?

@jasontedor
Copy link
Member

Typically what we would do here is add a skip for the test against the old versions that do not support the new functionality (here, 6.3.99) and we accept that in a mixed-version cluster situation a user will not receive consistent results depending on where the request is routed. Since this is a temporary situation (a mixed-version cluster) we are okay with this.

@tomcallahan tomcallahan requested a review from javanna May 16, 2018 21:15
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

left one comment on the skipped tests, LGTM otherwise.

@@ -9,6 +9,9 @@ setup:
index: test-index
---
Test reset index settings:
- skip:
version: " - 6.3.99"
reason: "include_defaults behavior is not expected to be consistent when nodes below 6.4.0 are present"
Copy link
Member

Choose a reason for hiding this comment

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

looking at the goal of this test, which is to make sure that settings are resettable, I wonder if we should rather take out the last get settings with include defaults set to true. After all, the previous request already verifies that the setting was reset. I find this slightly better than skipping the whole test.

@tomcallahan tomcallahan merged commit 20f9365 into elastic:6.x May 17, 2018
tomcallahan pushed a commit that referenced this pull request May 17, 2018
IndicesClientDocumentationIT.java is missing a callout, causing
the docs build to break.  This commit adds the missing callout.

Relates #30494
tomcallahan pushed a commit to tomcallahan/elasticsearch that referenced this pull request May 17, 2018
Get Settings API changes have now been backported to version 6.4, and
therefore the latest version must send and expect the extra fields when
communicating with 6.4+ code.

Relates elastic#29229 elastic#30494
tomcallahan added a commit that referenced this pull request May 18, 2018
….0 (#30706)

Get Settings API changes have now been backported to version 6.4, and
therefore the latest version must send and expect the extra fields when
communicating with 6.4+ code.

Relates #29229 #30494
dnhatn added a commit that referenced this pull request May 19, 2018
* 6.x:
  Mute testCorruptFileThenSnapshotAndRestore
  Plugins: Remove meta plugins (#30670)
  Upgrade to Lucene-7.4.0-snapshot-59f2b7aec2 (#30726)
  Docs: Add uptasticsearch to list of clients (#30738)
  [TEST] Reduce forecast overflow to disk test memory limit (#30727)
  [DOCS] Removes redundant index.asciidoc files (#30707)
  [DOCS] Moves X-Pack configurationg pages in table of contents (#30702)
  [ML][TEST] Fix bucket count assertion in ModelPlotsIT (#30717)
  [ML][TEST] Make AutodetectMemoryLimitIT less fragile (#30716)
  [Build] Add test admin when starting gradle run with trial license and
  [ML] provide tmp storage for forecasting and possibly any ml native jobs #30399
  Tests: Fail if test watches could not be triggered (#30392)
  Watcher: Prevent duplicate watch triggering during upgrade (#30643)
  [ML] add version information in case of crash of native ML process (#30674)
  Add detailed assert message to IndexAuditUpgradeIT (#30669)
  Preserve REST client auth despite 401 response (#30558)
  Make TransportClusterStateAction abide to our style (#30697)
  [DOCS] Fixes edit URLs for stack overview (#30583)
  [DOCS] Add missing callout in IndicesClientDocumentationIT
  Backport get settings API changes to 6.x (#30494)
  Silence sleep based watcher test
  [DOCS] Replace X-Pack terms with attributes
  Improve explanation in rescore (#30629)
  [test] packaging: add windows boxes (#30402)
  [ML] Clean left behind model state docs (#30659)
  filters agg docs duplicated 'bucket' word removal (#30677)
  top_hits doc example description update (#30676)
  MovingFunction Pipeline agg backport to 6.x (#30658)
  [Docs] Replace InetSocketTransportAddress with TransportAdress (#30673)
  [TEST] Account for increase in ML C++ memory usage (#30675)
  User proper write-once semantics for GCS repository (#30438)
  Deprecate `nGram` and `edgeNGram` names for ngram filters (#30209)
  Watcher: Fix watch history template for dynamic slack attachments (#30172)
  Fix _cluster/state to always return cluster_uuid (#30656)
ywelsch pushed a commit to ywelsch/elasticsearch that referenced this pull request May 23, 2018
….0 (elastic#30706)

Get Settings API changes have now been backported to version 6.4, and
therefore the latest version must send and expect the extra fields when
communicating with 6.4+ code.

Relates elastic#29229 elastic#30494
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Settings Settings infrastructure and APIs >non-issue v6.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants