This repository has been archived by the owner on Aug 2, 2022. It is now read-only.
Fix for upgrading mapping #309
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
This PR fixes two bugs of upgrading mapping: First, previously we only tried once when upgrading mapping. It is possible we need to try a few more times. The drawback is that if the code has a bug, we retry endlessly. I should fix it in the future. Second, we enclose the upgrade mapping function call under a user context in a recent commit. Upgrade mapping will fail when security plugin is enabled since a regular user cannot upgrade mappings of system indices. The PR also adds upper bounds to our dynamic settings if reasonable. Testing done: 1. Tested that upgrade mapping succeeds with new changes. 2. Tested the upper bounds of the changed settings are in effect.
Codecov Report
@@ Coverage Diff @@
## master #309 +/- ##
============================================
+ Coverage 72.06% 72.14% +0.07%
- Complexity 1969 1988 +19
============================================
Files 199 201 +2
Lines 9465 9553 +88
Branches 844 847 +3
============================================
+ Hits 6821 6892 +71
- Misses 2233 2248 +15
- Partials 411 413 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
saratvemulapalli
approved these changes
Nov 9, 2020
ylwu-amzn
reviewed
Nov 9, 2020
| ActionListener.wrap(r -> updateRunning.set(false), exception -> logger.error("Fail to updatea all mappings")), | ||
| ActionListener.wrap(r -> updateRunning.set(false), exception -> { | ||
| // TODO: don't retry endlessly. Can be annoying if there are too many exception logs. | ||
| updateRunning.set(false); |
There was a problem hiding this comment.
If remove this line, will not update index mapping if any exception happens?
There was a problem hiding this comment.
if removing this line, we will only try to update mapping once per node.
ylwu-amzn
reviewed
Nov 9, 2020
| ActionListener.wrap(r -> updateRunning.set(false), exception -> { | ||
| // TODO: don't retry endlessly. Can be annoying if there are too many exception logs. | ||
| updateRunning.set(false); | ||
| logger.error("Fail to updatea all mappings"); |
There was a problem hiding this comment.
Add ad to the error message.
ylwu-amzn
approved these changes
Nov 10, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Issue #, if available:
Description of changes:
This PR fixes two bugs of upgrading mapping:
First, previously we only tried once when upgrading mapping. It is possible we need to try a few more times. The drawback is that if the code has a bug, we retry endlessly. I should fix it in the future.
Second, we enclose the upgrade mapping function call under a user context in a recent commit. Upgrade mapping will fail when security plugin is enabled since a regular user cannot upgrade mappings of system indices.
The PR also adds upper bounds to our dynamic settings if reasonable.
Testing done:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.