Watcher accounts constructed lazily #36656
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Pinging @elastic/es-core-features |
spinscale
reviewed
Dec 14, 2018
| // update both | ||
| if (randomBoolean()) { | ||
| // update secure first | ||
| secureSettingValue.set(randomAlphaOfLength(4)); |
There was a problem hiding this comment.
putting these three line calls in dedicated methods allows you to get rid of the "first/second" comments and its easy to figure out the order when reading
There was a problem hiding this comment.
I did the replacement but parameters have side effects and the functions look super ugly.
|
does this PR also justify a backport into 6.5? |
I think so yes... |
|
https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request-2/2425/console Was a legit failure! The problem is that Accounts were not cached, every request for an account rebuilded it. I have swapped |
spinscale
approved these changes
Dec 17, 2018
albertzaharovits
added a commit
that referenced
this pull request
Dec 17, 2018
This fixes two bugs about watcher notifications: * registering accounts that had only secure settings was not possible before; these accounts are very much practical for Slack and PagerDuty integrations. * removes the limitation that, for an account with both secure and cluster settings, the admin had to first change/add the secure settings and only then add the dependent dynamic cluster settings. The reverse order would trigger a SettingsException for an incomplete account. The workaround is to lazily instantiate account objects, hoping that when accounts are instantiated all the required settings are in place. Previously, the approach was to greedily validate all the account settings by constructing the account objects, even if they would not ever be used by actions. This made sense in a world where all the settings were set by a single API. But given that accounts have dependent settings (that must be used together) that have to be changed using different APIs (POST _nodes/reload_secure_settings and PUT _cluster/settings), the settings group would technically be in an invalid state in between the calls. This fix builds account objects, and validates the settings, when they are needed by actions.
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Dec 18, 2018
* master: (30 commits) Revert "[Geo] Integrate Lucene's LatLonShape (BKD Backed GeoShapes) as default `geo_shape` indexing approach (elastic#35320)" Deprecate types in get_source and exist_source (elastic#36426) Fix duplicate phrase in shrink/split error message (elastic#36734) ingest: support default pipelines + bulk upserts (elastic#36618) TESTS:Debug Log. IndexStatsIT#testFilterCacheStats [Geo] Integrate Lucene's LatLonShape (BKD Backed GeoShapes) as default `geo_shape` indexing approach (elastic#35320) [TEST] fix float comparison in RandomObjects#getExpectedParsedValue Initialize startup `CcrRepositories` (elastic#36730) ingest: fix on_failure with Drop processor (elastic#36686) SNAPSHOTS: Adjust BwC Versions in Restore Logic (elastic#36718) [Painless] Add boxed type to boxed type casts for method/return (elastic#36571) Do not resolve addresses in remote connection info (elastic#36671) Add back one line removed by mistake regarding java version check and COMPAT jvm parameter existence Fixing line length for EnvironmentTests and RecoveryTests (elastic#36657) SQL: Fix translation of LIKE/RLIKE keywords (elastic#36672) [DOCS] Adds monitoring requirement for ingest node (elastic#36665) SNAPSHOTS: Disable BwC Tests Until elastic#36659 Landed (elastic#36709) Add doc's sequence number + primary term to GetResult and use it for updates (elastic#36680) [CCR] Add time since last auto follow fetch to auto follow stats (elastic#36542) Watcher accounts constructed lazily (elastic#36656) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes two bugs about watcher notifications:
registering accounts that had only secure settings was not possible before; these accounts are very much practical for
SlackandPagerDutyintegrations.removes the limitation that, for an account with both secure and cluster settings, the admin had to first change/add the secure settings and only then the dependent dynamic cluster settings. The reverse order would trigger a
SettingsExceptionfor an incomplete account. This had been partially discussed in: Watcher deprecate notification service settings #36403 (comment)The workaround is to lazily instantiate account objects. Previously the approach was to greedily validate all the account settings by constructing the account objects even if they would not ever be used by actions. This made sense in a world where all the settings were set by a single API. But given that accounts have dependent settings (that must be used together) that have to be changed using different APIs (
POST _nodes/reload_secure_settingsandPUT _cluster/settings), the settings could be in an invalid state in between the calls.This fix only builds account objects (and validates the settings) when they are needed by actions.