Enable merge on refresh and merge on commit on Opensearch #2535

reta · 2022-03-21T18:51:01Z

Signed-off-by: Andriy Redko andriy.redko@aiven.io

Description

Enable merge on refresh and merge on commit on Opensearch. Luckily, Apache Lucene 9.1 introduces MergeOnFlushMergePolicy which we could use from now on, activated by index.max_full_flush_merge (expert) index setting. The experiments I have done using [1] and pmc track shows significant reduction of the segments for a price of more merges:

./rally race    --track pmc      --target-hosts localhost:9200     --pipeline benchmark-only     --kill-running-processes --exclude-tasks="force-merge,refresh-after-force-merge,wait-until-merges-finish"

Baseline: 2.0.0-SNAPSHOT
Contender: 2.0.0-SNAPSHOT + "index.max_full_flush_merge": "10s"

Metric	Baseline	Contender	Diff	Unit	Diff %
...
Cumulative merge time of primary shards	33.7391	63.6844	29.9452	min	+88.76%
Cumulative merge count of primary shards	58	180	122		+210.34%
...
Segment count	133	43	-90		-67.67%
...

Metric	Baseline	Contender	Diff	Unit	Diff %
...
Cumulative merge time of primary shards	37.8788	61.7903	23.9114	min	+63.13%
Cumulative merge count of primary shards	65	175	110		+169.23%
...
Segment count	129	42	-87		-67.44%
...

[1] https://github.com/opensearch-project/opensearch-benchmark

Issues Resolved

Closes #1345

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
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.

opensearch-ci-bot · 2022-03-21T19:21:52Z

❌ Gradle Check failure a8c3de1084d485807d9a91cecbac567c3261006f
Log 3597

Reports 3597

reta · 2022-03-21T19:25:56Z

x Gradle Check failure a8c3de1 Log 3597

Reports 3597

#1561 :(

nknize

typo in the new settings constant ...FLASH_MERGE.

So I'm sure this MergePolicy will be promoted to lucene core but for now it's still sandboxed. Which means there are no bwc guarantees so we might consider feature flagging until it's a first class citizen?

/cc @mikemccand @msokolov any input on how much this policy might change that would give caution promoting it in OpenSearch as a first class citizen for 2.0?

server/src/main/java/org/opensearch/index/IndexSettings.java

reta · 2022-03-21T20:03:02Z

@nknize that is correct, it is sandboxed, I don't think we have a dedicated Property.Sandboxed scope for settings but we could introduce it to unlock experimental / sandboxed features.

opensearch-ci-bot · 2022-03-21T20:07:38Z

✅ Gradle Check success 37fb5b1ddd1b6d16c09aa2680d01ceee3fe65937
Log 3609

Reports 3609

opensearch-ci-bot · 2022-03-21T20:39:29Z

✅ Gradle Check success 7b0725cd18ffdb72129755fe935a9ce0d7c17439
Log 3616

Reports 3616

msokolov · 2022-03-21T21:13:03Z

The only planned change I'm aware of is to replace the tunable ( smallSegmentThresholdMB) and instead to derive that from pre-existing configs: https://issues.apache.org/jira/browse/LUCENE-10078. This is a newish addition, so it's possible there could be other changes, but so long as there isn't a lot of user-configuration provided, I don't think that should block this PR. I would be a little leery of exposing the time parameter, as opposed to a single boolean enable/disable parameter. Are users really going to know how to pick a good value for this?

reta · 2022-03-21T22:33:37Z

Are users really going to know how to pick a good value for this?

Thanks @msokolov ! We consider that to be an expert setting, exposing boolean instead would also be an issue - how to pick the "right" value for all possible index configurations? At least, with time value, there is no magic, the users could experiment. What do you think? Thank you!

nknize

At least, with time value, there is no magic, the users could experiment.

I think we should split the difference. Not many users will experiment with a setting like this; and it's only enabled if a time value is set. Which means most users won't use it (and reap the performance gains) unless we keep it simple. So (as much as I don't like increasing API surface area) let's do the following?:

change index.max_full_flush_merge to index.merge_on_flush.max_full_flush_merge_wait_time (or something shorter?)
add a new boolean setting index.merge_on_flush.enabled

Users can enable and reap the performance gains simply by setting index.merge_on_flush.enabled : true and the index.merge_on_flush.max_full_flush_merge_wait_time setting defaults to? 10s? 5s? (benchmark or test value?)
Users that want to experiment with index.merge_on_flush.max_full_flush_merge_wait_time can do so but we should probably document the tradeoffs.

Suggestions added inline

server/src/main/java/org/opensearch/index/IndexSettings.java

nknize · 2022-03-23T12:51:52Z

server/src/main/java/org/opensearch/index/IndexSettings.java

+     */
+    public static final Setting<TimeValue> INDEX_MAX_FULL_FLUSH_MERGE = Setting.timeSetting(
+        "index.max_full_flush_merge",
+        new TimeValue(0, TimeUnit.MILLISECONDS),


You benchmarked with 10s; should this be a reasonable default? Or 5s like the tests? /cc @msokolov

@nknize the only heuristic which comes to mind is that it should not be larger than index.refresh_interval: may be we could cap it to be max(half of the refresh interval, 10s) by default (if not specified), wdyt?

I think that's a reasonable default. Let's add that to this PR.

👍 will do that shortly

The original idea was to avoid creating lots of tiny segments while not interfering with the normal merge policy merges or other operations. The idea of the timeout was that we didn't want to block other operations for too long, since this merge-on-commit or merge-on-refresh is blocking the corresponding operation from completing. I don't know what refresh interval typically is set to - 1 minute maybe? That would lead to default of 10s. We've tested this with 30, and it works ok for our indexing workload, but YMMV. 10s seems safe to me

👍 thanks a lot @msokolov , 10s it be @nknize ?

server/src/main/java/org/opensearch/index/IndexSettings.java

opensearch-ci-bot · 2022-03-23T14:29:03Z

✅ Gradle Check success 56350d992dadbaee3b49864baa8c6fe8ec810608
Log 3698

Reports 3698

server/src/main/java/org/opensearch/index/IndexSettings.java

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

… enable commit-on-flush Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

…erty Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

opensearch-ci-bot · 2022-03-25T13:40:45Z

✅ Gradle Check success 1058c75
Log 3757

Reports 3757

nknize

Nice!! LGTM!

Enables merge on refresh and merge on commit in Opensearch by way of two new index options: index.merge_on_flush.max_full_flush_merge_wait_time and index.merge_on_flush.enabled. Default merge_on_flush is disabled and wait time is 10s. Signed-off-by: Andriy Redko <andriy.redko@aiven.io> (cherry picked from commit 908682d)

) Enables merge on refresh and merge on commit in OpenSearch by way of two new index options: index.merge_on_flush.max_full_flush_merge_wait_time and index.merge_on_flush.enabled. Default merge_on_flush is disabled and wait time is 10s. Signed-off-by: Andriy Redko <andriy.redko@aiven.io> (cherry picked from commit 908682d)

reta requested a review from a team as a code owner March 21, 2022 18:51

reta force-pushed the issue-1345 branch from a8c3de1 to 37fb5b1 Compare March 21, 2022 19:26

nknize requested changes Mar 21, 2022

View reviewed changes

server/src/main/java/org/opensearch/index/IndexSettings.java Outdated Show resolved Hide resolved

nknize self-requested a review March 23, 2022 12:45

nknize requested changes Mar 23, 2022

View reviewed changes

reta force-pushed the issue-1345 branch from 7b0725c to 56350d9 Compare March 23, 2022 13:49

andrross reviewed Mar 25, 2022

View reviewed changes

server/src/main/java/org/opensearch/index/IndexSettings.java Outdated Show resolved Hide resolved

reta added 4 commits March 25, 2022 09:01

Enable merge on refresh and merge on commit on Opensearch

1e00e74

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

Addressing code review comments

76cf63e

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

Addressing code review comments: added explicit additional setting to…

6ba8553

… enable commit-on-flush Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

Addressing code review comments: changed default and renamed the prop…

1058c75

…erty Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

reta force-pushed the issue-1345 branch from 56350d9 to 1058c75 Compare March 25, 2022 13:01

andrross approved these changes Mar 25, 2022

View reviewed changes

nknize self-requested a review March 25, 2022 18:41

nknize added enhancement Enhancement or improvement to existing feature or request Indexing & Search v3.0.0 Issues and PRs related to version 3.0.0 labels Mar 25, 2022

nknize approved these changes Mar 25, 2022

View reviewed changes

nknize merged commit 908682d into opensearch-project:main Mar 25, 2022

nknize added the backport 2.x Backport to 2.x branch label Mar 25, 2022

opensearch-trigger-bot bot mentioned this pull request Mar 25, 2022

[Backport 2.x] Enable merge on refresh and merge on commit on Opensearch #2600

Merged

reta mentioned this pull request Mar 28, 2022

[DOC]Documentation - Enable merge on refresh and merge on commit in Opensearch 2.1.0+ opensearch-project/documentation-website#477

Closed

nknize mentioned this pull request Mar 29, 2022

[Segment Replication] Design Proposal #2229

Closed

reta added the v2.1.0 Issues and PRs related to version 2.1.0 label May 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable merge on refresh and merge on commit on Opensearch #2535

Enable merge on refresh and merge on commit on Opensearch #2535

reta commented Mar 21, 2022

opensearch-ci-bot commented Mar 21, 2022

reta commented Mar 21, 2022

nknize left a comment •

edited

Loading

reta commented Mar 21, 2022

opensearch-ci-bot commented Mar 21, 2022

opensearch-ci-bot commented Mar 21, 2022

msokolov commented Mar 21, 2022

reta commented Mar 21, 2022 •

edited

Loading

nknize left a comment •

edited

Loading

nknize Mar 23, 2022

reta Mar 23, 2022

nknize Mar 24, 2022

reta Mar 24, 2022

msokolov Mar 24, 2022

reta Mar 24, 2022

opensearch-ci-bot commented Mar 23, 2022

opensearch-ci-bot commented Mar 25, 2022

nknize left a comment

Enable merge on refresh and merge on commit on Opensearch #2535

Enable merge on refresh and merge on commit on Opensearch #2535

Conversation

reta commented Mar 21, 2022

Description

Issues Resolved

Check List

opensearch-ci-bot commented Mar 21, 2022

reta commented Mar 21, 2022

nknize left a comment • edited Loading

Choose a reason for hiding this comment

reta commented Mar 21, 2022

opensearch-ci-bot commented Mar 21, 2022

opensearch-ci-bot commented Mar 21, 2022

msokolov commented Mar 21, 2022

reta commented Mar 21, 2022 • edited Loading

nknize left a comment • edited Loading

Choose a reason for hiding this comment

nknize Mar 23, 2022

Choose a reason for hiding this comment

reta Mar 23, 2022

Choose a reason for hiding this comment

nknize Mar 24, 2022

Choose a reason for hiding this comment

reta Mar 24, 2022

Choose a reason for hiding this comment

msokolov Mar 24, 2022

Choose a reason for hiding this comment

reta Mar 24, 2022

Choose a reason for hiding this comment

opensearch-ci-bot commented Mar 23, 2022

opensearch-ci-bot commented Mar 25, 2022

nknize left a comment

Choose a reason for hiding this comment

nknize left a comment •

edited

Loading

reta commented Mar 21, 2022 •

edited

Loading

nknize left a comment •

edited

Loading