Skip to content
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

Introduce Remote translog feature flag #4158

Merged
merged 4 commits into from
Aug 9, 2022

Conversation

Bukhtawar
Copy link
Collaborator

@Bukhtawar Bukhtawar commented Aug 8, 2022

Signed-off-by: Bukhtawar Khan [email protected]

Description

Introduce Remote translog feature flag, to gate remote translog changes from breaking remote segment store development and ensuring we can selectively enable remote segment store without requiring remote translog store. This clearly decouples durability semantics from refresh based(only remote segment based where clients would need to maintain non-flushed operations) to a per operation based durability semantics

For now we want to restrict enabling remote translog to cases when only remote segment store is enabled.

Issues Resolved

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.

@Bukhtawar Bukhtawar requested review from a team and reta as code owners August 8, 2022 08:55
@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2022

Gradle Check (Jenkins) Run Completed with:

@Bukhtawar
Copy link
Collaborator Author

Bukhtawar commented Aug 8, 2022

Failed due to #4089 #4162

@mch2
Copy link
Member

mch2 commented Aug 8, 2022

This looks more like an additional setting for remote store vs a feature flag in FeatureFlags.java. I think thats ok bc its still gated by remote store FF?

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2022

Gradle Check (Jenkins) Run Completed with:

@Bukhtawar
Copy link
Collaborator Author

This looks more like an additional setting for remote store vs a feature flag in FeatureFlags.java. I think thats ok bc its still gated by remote store FF?

That's correct @mch2. The FF would be common across remote segment store and remote translog store however to keep remote translog store gated and provide mechanisms to support remote segment store independent of remote translog store we are going with two separate index settings

@Bukhtawar Bukhtawar requested a review from mch2 August 8, 2022 17:24
Copy link
Member

@mch2 mch2 left a comment

Choose a reason for hiding this comment

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

LGTM.

nknize
nknize previously requested changes Aug 8, 2022
Copy link
Collaborator

@nknize nknize left a comment

Choose a reason for hiding this comment

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

Overall this LGTM, but I have questions about the silent validation when settings are null.

@@ -777,9 +775,30 @@ public void testRemoteStoreExplicitSetting() {
assertTrue(settings.isRemoteStoreEnabled());
}

public void testRemoteTranslogStoreDefaultSetting() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add a test if settings is null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nknize added tests for null settings

Signed-off-by: Bukhtawar Khan <[email protected]>
@Bukhtawar Bukhtawar requested a review from nknize August 8, 2022 19:26
@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2022

Gradle Check (Jenkins) Run Completed with:

@Bukhtawar Bukhtawar dismissed nknize’s stale review August 9, 2022 06:18

Addressed comments, will follow it up if there are are further changes needed

@Bukhtawar
Copy link
Collaborator Author

@nknize I am merging the PR to unblock other PRs, please feel free to add comments and I will address them in a follow up PR.

@Bukhtawar Bukhtawar merged commit a469a3c into opensearch-project:main Aug 9, 2022
@Bukhtawar Bukhtawar deleted the remote-translog-prs branch August 9, 2022 06:22
@Bukhtawar Bukhtawar added the backport 2.x Backport to 2.x branch label Aug 9, 2022
@opensearch-trigger-bot
Copy link
Contributor

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-4158-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 a469a3cbcb7a7294a81bf7e4122968c2a9b32ca4
# Push it to GitHub
git push --set-upstream origin backport/backport-4158-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-4158-to-2.x.

dreamer-89 pushed a commit to dreamer-89/OpenSearch that referenced this pull request Aug 12, 2022
* Introduce Remote translog feature flag
* Limit combinations of remote segment and translog store to valid ones

Signed-off-by: Bukhtawar Khan <[email protected]>
andrross pushed a commit to andrross/OpenSearch that referenced this pull request Oct 20, 2022
* Introduce Remote translog feature flag
* Limit combinations of remote segment and translog store to valid ones

Signed-off-by: Bukhtawar Khan <[email protected]>
(cherry picked from commit a469a3c)
andrross pushed a commit to andrross/OpenSearch that referenced this pull request Oct 21, 2022
* Introduce Remote translog feature flag
* Limit combinations of remote segment and translog store to valid ones

Signed-off-by: Bukhtawar Khan <[email protected]>
(cherry picked from commit a469a3c)
andrross pushed a commit to andrross/OpenSearch that referenced this pull request Oct 21, 2022
* Introduce Remote translog feature flag
* Limit combinations of remote segment and translog store to valid ones

Signed-off-by: Bukhtawar Khan <[email protected]>
(cherry picked from commit a469a3c)
andrross added a commit that referenced this pull request Oct 21, 2022
* Introduce Remote translog feature flag (#4158)

* Introduce Remote translog feature flag
* Limit combinations of remote segment and translog store to valid ones

Signed-off-by: Bukhtawar Khan <[email protected]>
(cherry picked from commit a469a3c)

* Add CHANGELOG entry

Signed-off-by: Andrew Ross <[email protected]>

Signed-off-by: Andrew Ross <[email protected]>
Co-authored-by: Bukhtawar Khan <[email protected]>
sachinpkale pushed a commit to sachinpkale/OpenSearch that referenced this pull request Jan 9, 2023
* Introduce Remote translog feature flag
* Limit combinations of remote segment and translog store to valid ones

Signed-off-by: Bukhtawar Khan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants