alias in rollup target_index field #445

petardz · 2022-08-05T20:29:13Z

Signed-off-by: Petar Dzepina [email protected]

Issue #, if available: #61

Description of changes:

Added support for using alias in rollup's target_index field

CheckList:

Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Petar Dzepina <[email protected]>

…ensearch-project#435) * tests * small refactor/improvements * added wildcard check when creating rollup job; removed resolving targetIndex on Rollup init; added test for wildcards * lint fixes * moved target_index validation in getRollup resp handler * removed catch block * exception fix * linter fix * fixed IT fail * added Exception catch block Signed-off-by: Petar Dzepina <[email protected]>

* reverted old error messages * reverted checking for matching jobs on whole set instead of job by job; Added picking rollup job deterministic * fixed sorting Signed-off-by: Petar Dzepina <[email protected]>

* fixed failing rollupInterceptorIT test * reverted old error messages * reverted checking for matching jobs on whole set instead of job by job; Added picking rollup job deterministic * fixed sorting * added ITs for multi rollup index search * added ITs for multi rollup index search#2 * detekt fixes * changed index names and rollup job * detekt fix * empty commit to trigger test pipeline Signed-off-by: Petar Dzepina <[email protected]>

Signed-off-by: Petar Dzepina <[email protected]>

* defekt fixes * tests * small refactor/improvements * added wildcard check when creating rollup job; removed resolving targetIndex on Rollup init; added test for wildcards * lint fixes * moved target_index validation in getRollup resp handler * added using toMap() * exception fix * linter fix * fixed IT fail * added Exception catch block Signed-off-by: Petar Dzepina <[email protected]>

Signed-off-by: Petar Dzepina <[email protected]>

codecov-commenter · 2022-08-10T23:32:45Z

Codecov Report

Merging #445 (efead8e) into main (63984b2) will decrease coverage by 0.02%.
The diff coverage is 64.04%.

@@             Coverage Diff              @@
##               main     #445      +/-   ##
============================================
- Coverage     75.84%   75.82%   -0.03%     
- Complexity     2482     2514      +32     
============================================
  Files           316      316              
  Lines         14551    14633      +82     
  Branches       2250     2276      +26     
============================================
+ Hits          11036    11095      +59     
- Misses         2251     2271      +20     
- Partials       1264     1267       +3

Impacted Files	Coverage Δ
.../rollup/util/RollupFieldValueExpressionResolver.kt	`65.38% <50.00%> (-24.62%)`	⬇️
...arch/indexmanagement/rollup/RollupMapperService.kt	`59.68% <66.17%> (+3.33%)`	⬆️
...pensearch/indexmanagement/IndexManagementPlugin.kt	`90.00% <100.00%> (ø)`
.../indexmanagement/rollup/settings/RollupSettings.kt	`95.74% <100.00%> (ø)`
...nsearch/indexmanagement/rollup/util/RollupUtils.kt	`80.91% <100.00%> (+0.07%)`	⬆️
...statemanagement/model/destination/CustomWebhook.kt	`67.14% <0.00%> (-28.58%)`	⬇️
...ent/indexstatemanagement/util/ManagedIndexUtils.kt	`76.10% <0.00%> (-1.77%)`	⬇️
...nsform/action/stop/TransportStopTransformAction.kt	`77.77% <0.00%> (ø)`
.../opensearch/indexmanagement/rollup/model/Rollup.kt	`86.51% <0.00%> (+0.46%)`	⬆️
...nt/indexstatemanagement/ManagedIndexCoordinator.kt	`69.88% <0.00%> (+0.58%)`	⬆️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

...main/kotlin/org/opensearch/indexmanagement/rollup/util/RollupFieldValueExpressionResolver.kt

src/main/kotlin/org/opensearch/indexmanagement/rollup/settings/RollupSettings.kt

src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupMapperService.kt

src/test/kotlin/org/opensearch/indexmanagement/rollup/runner/RollupRunnerIT.kt

src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupMapperService.kt

Signed-off-by: Petar Dzepina <[email protected]>

src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupMapperService.kt

Signed-off-by: Petar Dzepina <[email protected]>

khushbr · 2022-10-31T21:33:09Z

src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupMapperService.kt

+        rollup: Rollup,
+        targetIndexResolvedName: String,


A couple of documentation chages:

Let us add KDoc for description of parameters and return values. Explain the various terminology and any assumptions we are making with their operation.

rename rollup to rollupJob

rename targetIndexResolvedName

done

"rollup" is used throughout all Rollup related files. We should probably rename all or none.

What do you suggest? This is called "resolved name" since it can be resolved to concrete index name from mustache script or from alias

I was thinking of calling it plainly as concreteIndex ?

src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupMapperService.kt

khushbr · 2022-10-31T21:56:27Z

src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupMapperService.kt

+        if (rollup.isTargetIndexAlias()) {
+            if (aliasValidationResult !is RollupJobValidationResult.Valid) {
+                return aliasValidationResult
+            } else if (!isRollupIndex(targetIndexResolvedName, clusterService.state())) {


Can you elaborate more on this if condition? From what I gather:
If Target Index is an alias and the alias is not valid and the target index isn't a Rollup Index, then => Prepare Target Index.

These conditions seem orthogonal and don't make a coherent sense of what we are trying to do here. Are we saying this function will validate and update and create a Target index? It seems too many responsibilities present within a single function.

If Target Index is an alias and the alias IS valid and the target index isn't a Rollup Index

Only then we would "prepare target_index"

isRollupIndex would check for existence of rollup setting in index. This will be false if we're at first job run, because in case of alias, user setup index and not us.

Thank you for adding the context.
Can we add all these cases to validateAndAttemptToUpdateTargetIndex() documentation? We are appending new alias feature on top of the existing functionality in `validateAndAttemptToUpdateTargetIndex(), so let us update the KDoc for this function.

src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupMapperService.kt

khushbr · 2022-10-31T22:17:40Z

src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupMapperService.kt

@@ -96,6 +163,52 @@ class RollupMapperService(
        }
    }

+    suspend fun addRollupSettingToIndex(targetIndexResolvedName: String, hasLegacyPlugin: Boolean): Boolean {
+        val settings = if (hasLegacyPlugin) {
+            Settings.builder().put(LegacyOpenDistroRollupSettings.ROLLUP_INDEX.key, true).build()


Does the builder have the retries built it ?

You mean on updateSettings call on client? If yes, then exception would be thrown and job would fail. We don't have any retry in place for these kind of errors.

This is a conversation outside this PR, but I am assuming we do not retry the updateSettings call as we expect the next cycle of ISM execution to take care of any transient errors ?

src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupMapperService.kt

khushbr · 2022-10-31T22:26:44Z

src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupMapperService.kt

+            // 2. Put rollup mappings
+            val putMappingRequest: PutMappingRequest =
+                PutMappingRequest(targetIndexResolvedName).source(IndexManagementIndices.rollupTargetMappings, XContentType.JSON)
+            val respMappings: AcknowledgedResponse = client.admin().indices().suspendUntil {
+                putMapping(putMappingRequest, it)
+            }
+            if (!respMappings.isAcknowledged) {
+                return RollupJobValidationResult.Invalid("Failed to put initial rollup mappings for target index [$targetIndexResolvedName]")
+            }


This code has been repeated multiple times in repo (refer this), can we instead re-use the checkAndUpdateIndexMapping() helper function in IndexUtils.kt

checkAndUpdateIndexMapping() wouldn't work because of shouldUpdate call

shouldUpdateIndex() will fail because at this point there are no Index mapping. Would it make sense to add a mappingCheck as param, default value as true to checkAndUpdateIndexMapping() and then re-use it ?

Signed-off-by: Petar Dzepina <[email protected]>

* added support for mustache scripting of rollup.target_index field (#435) * tests * small refactor/improvements * added wildcard check when creating rollup job; removed resolving targetIndex on Rollup init; added test for wildcards * lint fixes * moved target_index validation in getRollup resp handler * removed catch block * fixed IT fail * added Exception catch block Signed-off-by: Petar Dzepina <[email protected]> (cherry picked from commit 70cf4ea)

* added support for mustache scripting of rollup.target_index field (#435) * tests * small refactor/improvements * added wildcard check when creating rollup job; removed resolving targetIndex on Rollup init; added test for wildcards * lint fixes * moved target_index validation in getRollup resp handler * removed catch block * fixed IT fail * added Exception catch block Signed-off-by: Petar Dzepina <[email protected]> (cherry picked from commit 70cf4ea) Co-authored-by: Petar Dzepina <[email protected]>

bowenlan-amzn · 2022-11-03T18:57:42Z

src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupMapperService.kt

+        if (rollupJobs != null &&
+            (rollupJobs.size > 1 || rollupJobs[0].id != rollup.id)
+        ) {
+            errorMessage = "More than one rollup job present on the backing index, cannot add alias for target index: [$targetIndexResolvedName]"


Do we really add alias to target index at any point?
Or we should say More than one rollup job present on the backing index of the target alias, cannot perform rollup to this target alias.

…ch-project#586) * added support for mustache scripting of rollup.target_index field (opensearch-project#435) * tests * small refactor/improvements * added wildcard check when creating rollup job; removed resolving targetIndex on Rollup init; added test for wildcards * lint fixes * moved target_index validation in getRollup resp handler * removed catch block * fixed IT fail * added Exception catch block Signed-off-by: Petar Dzepina <[email protected]> (cherry picked from commit 70cf4ea) Co-authored-by: Petar Dzepina <[email protected]>

…ch-project#586) * added support for mustache scripting of rollup.target_index field (opensearch-project#435) * tests * small refactor/improvements * added wildcard check when creating rollup job; removed resolving targetIndex on Rollup init; added test for wildcards * lint fixes * moved target_index validation in getRollup resp handler * removed catch block * fixed IT fail * added Exception catch block Signed-off-by: Petar Dzepina <[email protected]> (cherry picked from commit 70cf4ea) Co-authored-by: Petar Dzepina <[email protected]> Signed-off-by: Ronnak Saxena <[email protected]>

petardz requested a review from a team August 5, 2022 20:29

petardz force-pushed the alias_in_target_index branch 4 times, most recently from fad78ae to ad231f9 Compare August 5, 2022 20:42

petardz changed the title ~~initial commit~~ alias in target_index field Aug 5, 2022

petardz changed the title ~~alias in target_index field~~ alias in rollup target_index field Aug 5, 2022

petardz force-pushed the alias_in_target_index branch from ee3d235 to d121174 Compare August 9, 2022 22:38

khushbr requested a review from Angie-Zhang August 9, 2022 23:17

petardz force-pushed the alias_in_target_index branch 2 times, most recently from 7a6bade to 89031ce Compare August 10, 2022 18:40

petardz added 8 commits August 10, 2022 21:05

initial commit

9d94156

Signed-off-by: Petar Dzepina <[email protected]>

lint fix

9adfb37

Signed-off-by: Petar Dzepina <[email protected]>

refactor and ITs

4e7d173

Signed-off-by: Petar Dzepina <[email protected]>

* added support for searching multiple rollup indices with same mapping

bae728e

* reverted old error messages * reverted checking for matching jobs on whole set instead of job by job; Added picking rollup job deterministic * fixed sorting Signed-off-by: Petar Dzepina <[email protected]>

fixed rollupFieldValueExpressionResolverTests

7ca8c83

Signed-off-by: Petar Dzepina <[email protected]>

petardz force-pushed the alias_in_target_index branch from 89031ce to 8b9a825 Compare August 10, 2022 19:16

petardz added 4 commits August 10, 2022 21:18

merge from main

7a78788

Signed-off-by: Petar Dzepina <[email protected]>

fixed ITs and added more

a6dc287

Signed-off-by: Petar Dzepina <[email protected]>

unit test fix

d94cec2

Signed-off-by: Petar Dzepina <[email protected]>

removed debug logging from rollupRunner

8d78152

Signed-off-by: Petar Dzepina <[email protected]>

Merge branch 'opensearch-project:main' into alias_in_target_index

1193c9b

praveensameneni added the v2.4.0 'Issues and PRs related to version v2.4.0' label Oct 3, 2022

bowenlan-amzn reviewed Oct 17, 2022

View reviewed changes

Petar Dzepina added 2 commits October 25, 2022 20:38

more tweaks; more ITs

07b851f

Signed-off-by: Petar Dzepina <[email protected]>

merge from main

29f2cb5

Signed-off-by: Petar Dzepina <[email protected]>

bowenlan-amzn added the backport 2.x label Oct 25, 2022

bowenlan-amzn reviewed Oct 25, 2022

View reviewed changes

Petar Dzepina added 4 commits October 26, 2022 18:40

added more tests; added more descriptive messages

3424178

Signed-off-by: Petar Dzepina <[email protected]>

test fix; method rename

c1c5b24

Signed-off-by: Petar Dzepina <[email protected]>

test fix opensearch-project#2

c970286

Signed-off-by: Petar Dzepina <[email protected]>

empty commit

efead8e

Signed-off-by: Petar Dzepina <[email protected]>

Angie-Zhang previously approved these changes Oct 31, 2022

View reviewed changes

khushbr reviewed Oct 31, 2022

View reviewed changes

small changes

dfc5f7b

Signed-off-by: Petar Dzepina <[email protected]>

petardz dismissed Angie-Zhang’s stale review via dfc5f7b November 2, 2022 22:33

khushbr approved these changes Nov 3, 2022

View reviewed changes

Angie-Zhang approved these changes Nov 3, 2022

View reviewed changes

khushbr merged commit 70cf4ea into opensearch-project:main Nov 3, 2022

opensearch-trigger-bot bot mentioned this pull request Nov 3, 2022

[Backport 2.x] alias in rollup target_index field #586

Merged

bowenlan-amzn added the backport 2.4 label Nov 3, 2022

opensearch-trigger-bot bot mentioned this pull request Nov 3, 2022

[Backport 2.4] alias in rollup target_index field #588

Merged

bowenlan-amzn reviewed Nov 3, 2022

View reviewed changes

bowenlan-amzn mentioned this pull request Jan 10, 2023

[FEATURE] Allow Transform job to target alias index #656

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

alias in rollup target_index field #445

alias in rollup target_index field #445

petardz commented Aug 5, 2022 •

edited by bowenlan-amzn

Loading

codecov-commenter commented Aug 10, 2022 •

edited

Loading

khushbr Oct 31, 2022 •

edited

Loading

petardz Nov 2, 2022 •

edited

Loading

khushbr Nov 3, 2022

khushbr Oct 31, 2022

petardz Nov 1, 2022 •

edited

Loading

khushbr Nov 3, 2022

khushbr Oct 31, 2022

petardz Nov 1, 2022

khushbr Nov 3, 2022

khushbr Oct 31, 2022

petardz Nov 2, 2022

khushbr Nov 3, 2022

bowenlan-amzn Nov 3, 2022

alias in rollup target_index field #445

alias in rollup target_index field #445

Conversation

petardz commented Aug 5, 2022 • edited by bowenlan-amzn Loading

codecov-commenter commented Aug 10, 2022 • edited Loading

Codecov Report

khushbr Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

petardz Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petardz Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petardz commented Aug 5, 2022 •

edited by bowenlan-amzn

Loading

codecov-commenter commented Aug 10, 2022 •

edited

Loading

khushbr Oct 31, 2022 •

edited

Loading

petardz Nov 2, 2022 •

edited

Loading

petardz Nov 1, 2022 •

edited

Loading