Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Upgrade mapping #278

Merged
merged 3 commits into from
Oct 20, 2020

Conversation

kaituo
Copy link
Member

@kaituo kaituo commented Oct 19, 2020

Issue #, if available:

Description of changes:
This PR updates the current AD index mapping if a new version is detected. Previously, we didn't do so. This will cause issues when upgrading AD binary version: newly added fields will not be searchable, though we can still send get requests to fetch them.

This PR also makes the cache miss handling setting a dynamic setting.

Testing done:

  1. manual upgrade testing passes with fgac on and off.
  2. will add unit tests later.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov
Copy link

codecov bot commented Oct 19, 2020

Codecov Report

Merging #278 into master will decrease coverage by 0.64%.
The diff coverage is 42.05%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #278      +/-   ##
============================================
- Coverage     70.72%   70.08%   -0.65%     
- Complexity     1797     1815      +18     
============================================
  Files           187      190       +3     
  Lines          8687     8867     +180     
  Branches        735      756      +21     
============================================
+ Hits           6144     6214      +70     
- Misses         2194     2298     +104     
- Partials        349      355       +6     
Flag Coverage Δ Complexity Δ
#plugin 69.26% <42.05%> (-0.69%) 1815.00 <18.00> (+18.00) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...stroforelasticsearch/ad/AnomalyDetectorRunner.java 7.69% <0.00%> (ø) 1.00 <0.00> (ø)
...distroforelasticsearch/ad/constant/CommonName.java 66.66% <ø> (ø) 1.00 <0.00> (ø)
...elasticsearch/ad/util/ThrowingConsumerWrapper.java 85.71% <ø> (ø) 2.00 <0.00> (ø)
...sticsearch/ad/indices/AnomalyDetectionIndices.java 50.00% <23.91%> (-23.34%) 39.00 <9.00> (+9.00) ⬇️
...elasticsearch/ad/util/ThrowingSupplierWrapper.java 40.00% <40.00%> (ø) 2.00 <2.00> (?)
...istroforelasticsearch/ad/constant/CommonValue.java 50.00% <50.00%> (ø) 1.00 <1.00> (?)
...oforelasticsearch/ad/AnomalyDetectorJobRunner.java 75.63% <80.00%> (+0.11%) 36.00 <1.00> (+1.00)
...opendistroforelasticsearch/ad/indices/ADIndex.java 94.73% <94.73%> (ø) 4.00 <4.00> (?)
...stroforelasticsearch/ad/AnomalyDetectorPlugin.java 93.38% <100.00%> (+0.11%) 13.00 <0.00> (ø)
...stroforelasticsearch/ad/caching/PriorityCache.java 89.35% <100.00%> (+0.14%) 57.00 <0.00> (ø)
... and 13 more

@@ -124,6 +126,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
? WriteRequest.RefreshPolicy.parse(request.param(REFRESH))
: WriteRequest.RefreshPolicy.IMMEDIATE;
RestRequest.Method method = request.getHttpRequest().method();
anomalyDetectionIndices.updateMappingIfNecessary();
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this fail with FGAC ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah..lets move this to transport layer. Since Anomaly Detection indices will be part of system index, we cannot update them.

Copy link
Member Author

@kaituo kaituo Oct 20, 2020

Choose a reason for hiding this comment

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

yeah, moved to transport layer. After moving, tested working with fgac.

@kaituo kaituo added the bug Something isn't working label Oct 19, 2020
This PR updates the current AD index mapping if a new version is detected.  Previously, we didn't do so.  This will cause issues when upgrading AD binary version: newly added fields will not be searchable, though we can still send get requests to fetch them.

Testing done:
1. manual upgrade testing passes.
2. will add unit tests later.
Copy link
Contributor

@ylwu-amzn ylwu-amzn left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the change

@kaituo kaituo merged commit 8def1ba into opendistro-for-elasticsearch:master Oct 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants