QueryIndex rollover when field mapping limit is reached #725

petardz · 2022-12-21T22:19:33Z

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

Issue #, if available:

Description of changes:

Added handling of situation when queryIndex has more fields in mapping then set limit(default 1000).

queryIndex is now alias instead of concrete index
Index pattern is computed like this: queryIndex + "-1"

We keep mapping of source_index+monitorId --> concreteQueryIndex in map inside Monitor object.

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]>

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

alerting/src/main/kotlin/org/opensearch/alerting/BucketLevelMonitorRunner.kt

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

alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt

alerting/src/main/kotlin/org/opensearch/alerting/model/MonitorMetadata.kt

…hema; removed unused code Signed-off-by: Petar Dzepina <[email protected]>

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

codecov-commenter · 2022-12-28T02:40:05Z

Codecov Report

Merging #725 (c688613) into main (817d9ef) will decrease coverage by 0.81%.
The diff coverage is 66.36%.

@@             Coverage Diff              @@
##               main     #725      +/-   ##
============================================
- Coverage     76.11%   75.30%   -0.82%     
  Complexity      116      116              
============================================
  Files           124      125       +1     
  Lines          6618     6822     +204     
  Branches        982     1021      +39     
============================================
+ Hits           5037     5137     +100     
- Misses         1083     1154      +71     
- Partials        498      531      +33

Impacted Files	Coverage Δ
...in/kotlin/org/opensearch/alerting/MonitorRunner.kt	`63.29% <ø> (-0.46%)`	⬇️
...pensearch/alerting/model/AlertingConfigAccessor.kt	`75.00% <ø> (-3.38%)`	⬇️
...tlin/org/opensearch/alerting/util/AlertingUtils.kt	`53.12% <ø> (-7.41%)`	⬇️
...alerting/transport/TransportDeleteMonitorAction.kt	`66.29% <55.88%> (-8.34%)`	⬇️
.../opensearch/alerting/DocumentLevelMonitorRunner.kt	`77.58% <63.15%> (-4.58%)`	⬇️
...n/org/opensearch/alerting/model/MonitorMetadata.kt	`44.31% <64.00%> (+5.51%)`	⬆️
.../org/opensearch/alerting/MonitorMetadataService.kt	`65.07% <65.07%> (ø)`
...lerting/transport/TransportExecuteMonitorAction.kt	`82.08% <66.66%> (+0.55%)`	⬆️
...opensearch/alerting/util/DocLevelMonitorQueries.kt	`72.87% <69.81%> (-7.49%)`	⬇️
.../alerting/transport/TransportIndexMonitorAction.kt	`54.93% <75.00%> (-5.01%)`	⬇️
... and 10 more

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

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

lezzago · 2022-12-28T16:14:23Z

alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt

+            if (clusterService.state().metadata.hasIndex(ScheduledJob.DOC_LEVEL_QUERIES_INDEX)) {
+                val acknowledgedResponse: AcknowledgedResponse = client.suspendUntil {
+                    admin().indices().delete(DeleteIndexRequest(ScheduledJob.DOC_LEVEL_QUERIES_INDEX))
+                }
+                if (!acknowledgedResponse.isAcknowledged) {
+                    val errorMessage = "Deletion of old queryIndex [${ScheduledJob.DOC_LEVEL_QUERIES_INDEX}] index is not acknowledged!"
+                    log.error(errorMessage)
+                    throw AlertingException.wrap(OpenSearchStatusException(errorMessage, RestStatus.INTERNAL_SERVER_ERROR))
+                }
+            }


Just to confirm, even when it gets deleted here, we don't care for the old data since we will recreate it anyways, right?

Yes, it is checked if it exist and recreated if not, on every monitor update/execute API call and every monitor execution by job scheduler

lezzago · 2022-12-28T17:12:06Z

alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt

+                    // If we reached limit for total number of fields in mappings after rollover
+                    // it means that source index has more then (FIELD_LIMIT - 3) fields (every query index has 3 fields defined)


Can't we rollover multiple times?

queryIndex already has 3 fields in mappings so when source_index has 997 fields or more, we couldn't succeed ever so we must abort

but in any other case, yea, we can rollover

Ahh I see. Can we add a TODO to support indices that have at least 997 fields by looking into breaking up the data amongst multiple rolled over indices?

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

sbcd90

@petardz can we add an integration test for this scenario?

sbcd90 · 2022-12-29T02:17:44Z

@petardz can we add an integration test for this scenario?

i see it added. thanks @petardz

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

lezzago · 2022-12-29T16:13:29Z

alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt

+        const val PROPERTIES = "properties"
+        const val NESTED = "nested"
+        const val TYPE = "type"
+        const val INDEX_PATTERN_SUFFIX = "-000001"


Is the number getting incremented during rollovers? If so, where?

It is done by _rollover core's API here

lezzago · 2022-12-29T16:18:10Z

alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt

+        Assert.assertTrue(alerts.size == 1)
+
+        // Both monitors used same queryIndex. Since source index has close to limit amount of fields in mappings,
+        // we expect that creation of second monitor would rollover queryIndex


Wait is the expectation that the queryIndex is rolled over or not? It is not getting rolled over here.

This comment was incorrect. I fixed it with new commit and added few more in other tests

lezzago · 2022-12-29T16:18:57Z

alerting/src/test/kotlin/org/opensearch/alerting/MonitorDataSourcesIT.kt

+        Assert.assertTrue(alerts.size == 2)
+    }
+
+    fun `test queryIndex bwc`() {


Update name to test queryIndex bwc when index was not an alias

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

sbcd90 · 2022-12-29T09:05:33Z

alerting/src/main/kotlin/org/opensearch/alerting/AlertingPlugin.kt

+            xContentRegistry,
+            settings
+        )
+


are these objects needed? looks like lot of old alerting code is refactored into these classes?

sbcd90 · 2022-12-29T09:06:03Z

alerting/src/main/kotlin/org/opensearch/alerting/MonitorMetadataFactory.kt

+        this.settings = settings
+    }
+
+    suspend fun newInstance(monitor: Monitor, createWithRunContext: Boolean): MonitorMetadata {


this is an unused method?

opensearch-trigger-bot · 2022-12-31T02:19:01Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-725-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 6f487462862d92def1334599c1d0aaad5201a270
# Push it to GitHub
git push --set-upstream origin backport/backport-725-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-725-to-2.x.

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

opensearch-trigger-bot · 2023-02-17T19:38:50Z

The backport to 2.3 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.3 2.3
# Navigate to the new working tree
cd .worktrees/backport-2.3
# Create a new branch
git switch --create backport/backport-725-to-2.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 6f487462862d92def1334599c1d0aaad5201a270
# Push it to GitHub
git push --set-upstream origin backport/backport-725-to-2.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.3

Then, create a pull request where the base branch is 2.3 and the compare/head branch is backport/backport-725-to-2.3.

Signed-off-by: Petar Dzepina <[email protected]> (cherry picked from commit 6f48746)

initial commit

4d163d7

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

petardz requested a review from a team December 21, 2022 22:19

refactored to use metadata instead of monitor to store queryIndex sou…

1dd3134

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

lezzago reviewed Dec 24, 2022

View reviewed changes

alerting/src/main/kotlin/org/opensearch/alerting/BucketLevelMonitorRunner.kt Outdated Show resolved Hide resolved

petardz added 3 commits December 26, 2022 03:03

metadata fixes; refactor

cb42b15

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

added licence headers; added more tests; small refactor

b9cbff2

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

fixed IT

72daba8

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

lezzago reviewed Dec 28, 2022

View reviewed changes

alerting/src/main/kotlin/org/opensearch/alerting/util/DocLevelMonitorQueries.kt Show resolved Hide resolved

lezzago reviewed Dec 28, 2022

View reviewed changes

alerting/src/main/kotlin/org/opensearch/alerting/model/MonitorMetadata.kt Show resolved Hide resolved

petardz added 2 commits December 28, 2022 03:25

bwc handling; fixed monitorMetadata parsing; updated scheduled-job sc…

9173705

…hema; removed unused code Signed-off-by: Petar Dzepina <[email protected]>

incremented schema version

3dd83f9

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

petardz added 2 commits December 28, 2022 10:05

fixed schema verify version for scheduled-job to expect ver 6

63b8e4c

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

schema version fix

d4673e7

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

lezzago reviewed Dec 28, 2022

View reviewed changes

petardz added 2 commits December 28, 2022 18:51

added bwc test

1a704b7

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

reverted accessing fields by this

5fb8ab0

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

sbcd90 reviewed Dec 29, 2022

View reviewed changes

petardz added 3 commits December 29, 2022 11:41

added TODO

5472c53

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

added expand_wildcards to _refresh call in ITs

a19f46d

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

flaky test fix

b4e6335

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

lezzago reviewed Dec 29, 2022

View reviewed changes

petardz added 3 commits December 29, 2022 17:27

fixed comments in tests

ced5111

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

test name change

c8e8233

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

renamed method

ffc8228

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

lezzago approved these changes Dec 30, 2022

View reviewed changes

petardz added 2 commits December 30, 2022 01:33

removed unused code

0618056

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

refactored DeleteMonitor API; fixed IndexMonitor bug

c688613

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

lezzago approved these changes Dec 30, 2022

View reviewed changes

sbcd90 approved these changes Dec 31, 2022

View reviewed changes

sbcd90 added the backport 2.x label Dec 31, 2022

sbcd90 merged commit 6f48746 into opensearch-project:main Dec 31, 2022

petardz added a commit to petardz/alerting that referenced this pull request Jan 3, 2023

conflict resolve for opensearch-project#725

225a471

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

lezzago mentioned this pull request Jan 5, 2023

[Backport 2.x] QueryIndex rollover when field mapping limit is reached. #729

Merged

lezzago added backport 2.3 backport 2.4 labels Feb 17, 2023

opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 17, 2023

QueryIndex rollover when field mapping limit is reached (#725)

fc5e5de

Signed-off-by: Petar Dzepina <[email protected]> (cherry picked from commit 6f48746)

opensearch-trigger-bot bot mentioned this pull request Feb 17, 2023

[Backport 2.4] QueryIndex rollover when field mapping limit is reached #800

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QueryIndex rollover when field mapping limit is reached #725

QueryIndex rollover when field mapping limit is reached #725

petardz commented Dec 21, 2022

codecov-commenter commented Dec 28, 2022 •

edited

Loading

lezzago Dec 28, 2022

petardz Dec 28, 2022

lezzago Dec 28, 2022

petardz Dec 28, 2022 •

edited

Loading

petardz Dec 28, 2022

lezzago Dec 29, 2022

petardz Dec 29, 2022

sbcd90 left a comment

sbcd90 commented Dec 29, 2022

lezzago Dec 29, 2022

petardz Dec 29, 2022

lezzago Dec 29, 2022

lezzago Dec 29, 2022

petardz Dec 29, 2022

lezzago Dec 29, 2022

petardz Dec 29, 2022

sbcd90 Dec 29, 2022

sbcd90 Dec 29, 2022

opensearch-trigger-bot bot commented Dec 31, 2022

opensearch-trigger-bot bot commented Feb 17, 2023

		// If we reached limit for total number of fields in mappings after rollover
		// it means that source index has more then (FIELD_LIMIT - 3) fields (every query index has 3 fields defined)

QueryIndex rollover when field mapping limit is reached #725

QueryIndex rollover when field mapping limit is reached #725

Conversation

petardz commented Dec 21, 2022

codecov-commenter commented Dec 28, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petardz Dec 28, 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

sbcd90 left a comment

Choose a reason for hiding this comment

sbcd90 commented Dec 29, 2022

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

opensearch-trigger-bot bot commented Dec 31, 2022

opensearch-trigger-bot bot commented Feb 17, 2023

codecov-commenter commented Dec 28, 2022 •

edited

Loading

petardz Dec 28, 2022 •

edited

Loading