-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update merge on refresh and merge on commit defaults in Opensearch (Lucene 9.3) #3561
Conversation
❌ Gradle Check failure a32befcdc868023d73566a56ce7d4c6077ff6e0d |
❌ Gradle Check failure 387ee14c2e2d0b68752ae72a35ee85247a410390 |
❌ Gradle Check failure 32ae3b1908d519e255b83f0c348a8442ff60f38a |
start gradle check |
❌ Gradle Check failure 32ae3b1908d519e255b83f0c348a8442ff60f38a |
start gradle check |
❌ Gradle Check failure 32ae3b1908d519e255b83f0c348a8442ff60f38a |
start gradle check |
❌ Gradle Check failure 32ae3b1908d519e255b83f0c348a8442ff60f38a |
❌ Gradle Check failure d592c66312e86fcaaae27c17b3420fe9c2ab52fc |
a3a3553
to
6837d1b
Compare
❌ Gradle Check failure 941fe47335cfb16d0375b3acb933c702d4d8a7f2 |
❌ Gradle Check failure a3a355339ef093356afee7acf39ec1fbe8b1ebd1 |
❌ Gradle Check failure 6837d1bb076a4eff4fbba2682b5a4b31a540b1a2 |
❌ Gradle Check failure d8cfd01d8b2d4a7b93f5da1eb34520d19b316f93 |
✅ Gradle Check success 6c33cb70bd11084133984dcb206009a51284e09b |
✅ Gradle Check success aac0ad7188301f1891caadfa8a44538e5638e673 |
✅ Gradle Check success 43d03ee329d25bdc90ef815497645e96f13f8234 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good. Just a general question on whether or not we want to switch away from a boolean setting to using a policy name.
@@ -574,7 +573,7 @@ public void testMergeSegmentsOnCommit() throws Exception { | |||
final Settings.Builder settings = Settings.builder() | |||
.put(defaultSettings.getSettings()) | |||
.put(IndexSettings.INDEX_MERGE_ON_FLUSH_MAX_FULL_FLUSH_MERGE_WAIT_TIME.getKey(), TimeValue.timeValueMillis(5000)) | |||
.put(IndexSettings.INDEX_MERGE_ON_FLUSH_ENABLED.getKey(), true); | |||
.put(IndexSettings.INDEX_MERGE_ON_FLUSH_POLICY.getKey(), "merge-on-flush"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to switch away from a boolean here at this point? I'm not sure we expect any merge on flush policy variations other than the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nknize the INDEX_MERGE_ON_FLUSH_POLICY
is a new setting (semantics of INDEX_MERGE_ON_FLUSH_ENABLED
is kept as boolean). The reason for that is that not every policy may implement merge-on-flush/commit but the composite one could be used for this reasons.
1abf3f6
to
913868d
Compare
@nknize may I ask you please to take a second look? Thank you! |
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
@nknize may I ask you please to take a second look? Thank you! |
…ucene 9.3) Signed-off-by: Andriy Redko <[email protected]>
Signed-off-by: Andriy Redko <[email protected]>
@dblock @dblock @Bukhtawar sorry guys, may I ask for your help here? sadly cannot get back @nknize attention on this change .... thank you! |
Hey @reta sorry about that! I had gh status set to away while I was out and it suppressed my notifications so I missed the ping. Will check it here shortly. |
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this@reta!
Thanks a lot for review @nknize, really appreciate it ! |
The backport to
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-3561-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 a34a4c04250b5c4c210ce79d14611953f5267963
# Push it to GitHub
git push --set-upstream origin backport/backport-3561-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 |
…ucene 9.3) (opensearch-project#3561) * Update merge on refresh and merge on commit defaults in Opensearch (Lucene 9.3) Signed-off-by: Andriy Redko <[email protected]> * Addressing code review comments, adding more tests Signed-off-by: Andriy Redko <[email protected]> (cherry picked from commit a34a4c0) Signed-off-by: Andriy Redko <[email protected]>
…ucene 9.3) (#3561) (#4013) * Update merge on refresh and merge on commit defaults in Opensearch (Lucene 9.3) Signed-off-by: Andriy Redko <[email protected]> * Addressing code review comments, adding more tests Signed-off-by: Andriy Redko <[email protected]> (cherry picked from commit a34a4c0) Signed-off-by: Andriy Redko <[email protected]>
Signed-off-by: Andriy Redko [email protected]
Description
The Apache Lucene 9.3 made merge-on-refresh enabled by default [1], [2]. The OpenSearch merge on refresh and merge on commit defaults have to be updated in order to accommodate this change.
Additional context, the merge-on-refresh is now fully implemented by
MergePolicy
. Some merge polices (likeTieredMergePolicy
andLogMergePolicy
) already benefit from this right away, but for others the wrapping merge policy (likeMergeOnFlushMergePolicy
) still has (or could) to be applied. This pull request acknowledged the merge-on-refresh as default and also allows to disable it or supply the merge policy to be used.[1] apache/lucene#921
[2] apache/lucene#935
Issues Resolved
Closes #3553
Check List
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.