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.

Sign up for GitHub

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

Jump to bottom

Make TranslogDeletionPolicy abstract for extension #1456

Merged
merged 2 commits into from
Nov 1, 2021
Merged

Make TranslogDeletionPolicy abstract for extension #1456

merged 2 commits into from
Nov 1, 2021

Conversation

adnapibar
Copy link
Contributor

@adnapibar adnapibar commented Oct 27, 2021 •
edited
Loading

Description

As part of the commit 2ebd0e0, we added a new method to the EnginePlugin to provide a custom TranslogDeletionPolicy. This commit makes minTranslogGenRequired method abstract in this class for overridden by plugins. The default implementation is provided by DefaultTranslogDeletionPolicy.

Issues Resolved

Relates #1260

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.

Signed-off-by: Rabi Panda adnapibar@gmail.com

@adnapibar adnapibar requested a review from nknize October 27, 2021 23:47
@opensearch-ci-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 6f9f31d412d6c2378393304d1a4bce1faaddc03a

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 6f9f31d412d6c2378393304d1a4bce1faaddc03a

@adnapibar adnapibar added the v1.2.0 Issues related to version 1.2.0 label Oct 27, 2021
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 6f9f31d412d6c2378393304d1a4bce1faaddc03a

@adnapibar
Copy link
Contributor Author

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 6f9f31d412d6c2378393304d1a4bce1faaddc03a
Log 843

Reports 843

@adnapibar adnapibar mentioned this pull request Oct 28, 2021
Merged
5 tasks
nknize
nknize requested changes Oct 28, 2021
View reviewed changes
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.

Looking good. A few questions, nits, and changes.

server/src/main/java/org/opensearch/index/translog/DefaultTranslogDeletionPolicy.java Show resolved Hide resolved
server/src/main/java/org/opensearch/index/translog/TranslogDeletionPolicy.java Outdated Show resolved Hide resolved
server/src/main/java/org/opensearch/index/translog/TranslogDeletionPolicy.java Outdated Show resolved Hide resolved
server/src/main/java/org/opensearch/index/translog/TranslogDeletionPolicy.java Show resolved Hide resolved
server/src/test/java/org/opensearch/index/engine/EngineConfigFactoryTests.java Show resolved Hide resolved
server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java Outdated Show resolved Hide resolved
@adnapibar adnapibar requested a review from nknize October 28, 2021 19:36
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success cfcdbb23e77082e8792c40e49d968bd2410d353a

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success cfcdbb23e77082e8792c40e49d968bd2410d353a

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success cfcdbb23e77082e8792c40e49d968bd2410d353a
Log 860

Reports 860

nknize
nknize approved these changes Oct 29, 2021
View reviewed changes
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.

Thanks! LGTM

server/src/main/java/org/opensearch/index/translog/DefaultTranslogDeletionPolicy.java Show resolved Hide resolved
server/src/main/java/org/opensearch/index/translog/TranslogDeletionPolicy.java Show resolved Hide resolved
@nknize nknize added the enhancement Enhancement or improvement to existing feature or request label Oct 29, 2021
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success c94a6749a53157d22372bf24bbb8ac8a89b2788e

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success c94a6749a53157d22372bf24bbb8ac8a89b2788e

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure c94a6749a53157d22372bf24bbb8ac8a89b2788e
Log 903

Reports 903

@adnapibar
Make TranslogDeletionPolicy abstract
219475f 
As part of the commit 2ebd0e0, we added a new method to the EnginePlugin to provide a custom TranslogDeletionPolicy. This commit makes minTranslogGenRequired method abstract in this class for implementation by child classes. The default implementation is provided by DefaultTranslogDeletionPolicy.

Signed-off-by: Rabi Panda <adnapibar@gmail.com>
@adnapibar
Refactor to move implementation logic out of TranslogDeletionPolicy
ecc278c 
Signed-off-by: Rabi Panda <adnapibar@gmail.com>
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success ecc278c

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success ecc278c

@nknize
Copy link
Collaborator

nknize commented Oct 29, 2021

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure ecc278c
Log 904

Reports 904

@adnapibar
Copy link
Contributor Author

Test failure not reproducible, going to retry!

@adnapibar
Copy link
Contributor Author

start gradle check

@nknize
Copy link
Collaborator

nknize commented Oct 29, 2021

Documenting initial failure for posterity. I don't see any transport issues,. It looks like a network connection may have died so could be a jenkins flake out.

REPRODUCE WITH: ./gradlew ':server:internalClusterTest' --tests "org.opensearch.versioning.ConcurrentSeqNoVersioningIT.testSeqNoCASLinearizability" -Dtests.seed=4D5AB234E47676D0 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=nb-NO -Dtests.timezone=America/Kentucky/Louisville -Druntime.java=17

REPRODUCE WITH: ./gradlew ':server:internalClusterTest' --tests "org.opensearch.versioning.ConcurrentSeqNoVersioningIT.testSeqNoCASLinearizability" -Dtests.seed=4D5AB234E47676D0 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=nb-NO -Dtests.timezone=America/Kentucky/Louisville -Druntime.java=17

org.opensearch.versioning.ConcurrentSeqNoVersioningIT > testSeqNoCASLinearizability FAILED
    MasterNotDiscoveredException[NotMasterException[no longer master. source: [remove-data-stream [*]]]]; nested: NotMasterException[no longer master. source: [remove-data-stream [*]]];
        at __randomizedtesting.SeedInfo.seed([4D5AB234E47676D0:96949C5EFE645464]:0)
        at org.opensearch.action.support.master.TransportMasterNodeAction$AsyncSingleAction$2.onTimeout(TransportMasterNodeAction.java:275)
        at org.opensearch.cluster.ClusterStateObserver$ContextPreservingListener.onTimeout(ClusterStateObserver.java:369)
        at org.opensearch.cluster.ClusterStateObserver$ObserverClusterStateListener.onTimeout(ClusterStateObserver.java:287)
        at org.opensearch.cluster.service.ClusterApplierService$NotifyTimeout.run(ClusterApplierService.java:692)
        at org.opensearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:733)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at java.lang.Thread.run(Thread.java:833)

        Caused by:
        NotMasterException[no longer master. source: [remove-data-stream [*]]]

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure ecc278c
Log 905

Reports 905

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure ecc278c
Log 906

Reports 906

@adnapibar
Copy link
Contributor Author

One more not reproducible

REPRODUCE WITH: ./gradlew ':server:internalClusterTest' --tests "org.opensearch.recovery.TruncatedRecoveryIT.testCancelRecoveryAndResume" -Dtests.seed=F63FA8E71804BD8E -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=zh-SG -Dtests.timezone=Etc/GMT -Druntime.java=17
  2> java.lang.AssertionError: timed out waiting for green state
        at __randomizedtesting.SeedInfo.seed([F63FA8E71804BD8E:782C841F8F00CAC4]:0)
        at org.junit.Assert.fail(Assert.java:88)
        at org.opensearch.test.OpenSearchIntegTestCase.ensureColor(OpenSearchIntegTestCase.java:1019)
        at org.opensearch.test.OpenSearchIntegTestCase.ensureGreen(OpenSearchIntegTestCase.java:958)
        at org.opensearch.test.OpenSearchIntegTestCase.ensureGreen(OpenSearchIntegTestCase.java:947)
        at org.opensearch.recovery.TruncatedRecoveryIT.testCancelRecoveryAndResume(TruncatedRecoveryIT.java:180)

@adnapibar
Copy link
Contributor Author

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success ecc278c
Log 907

Reports 907

@nknize nknize merged commit d45f5df into opensearch-project:main Nov 1, 2021
@nknize nknize added the pending backport Identifies an issue or PR that still needs to be backported label Nov 1, 2021
@nknize
Copy link
Collaborator

nknize commented Nov 1, 2021

Ready for backport

adnapibar added a commit that referenced this pull request Nov 1, 2021
@adnapibar
Make TranslogDeletionPolicy abstract for extension (#1456) (#1478)
9b5365f 
As part of the commit 2ebd0e0, we added a new method to the EnginePlugin to provide a
custom TranslogDeletionPolicy. This commit makes minTranslogGenRequired method
abstract in this class for implementation by child classes. The default implementation
is provided by DefaultTranslogDeletionPolicy.

Signed-off-by: Rabi Panda <adnapibar@gmail.com>
@dblock dblock removed the pending backport Identifies an issue or PR that still needs to be backported label Dec 6, 2021
@adnapibar adnapibar deleted the abstract-translog-del-policy branch November 15, 2022 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrross andrross andrross left review comments

@nknize nknize nknize approved these changes

Assignees
No one assigned
Labels
enhancement Enhancement or improvement to existing feature or request v1.2.0 Issues related to version 1.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@adnapibar @opensearch-ci-bot @nknize @andrross @dblock