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

[Backport 2.x] Integrate Engine with decoupled Translog interfaces #3822

Merged
merged 19 commits into from
Aug 2, 2022
Merged

[Backport 2.x] Integrate Engine with decoupled Translog interfaces #3822

merged 19 commits into from
Aug 2, 2022

Conversation

satyajitg28
Copy link

Description

The PR aims to Integrate Engine with decoupled Translog interfaces. Original CR link. Ensuring Engine is backward compatible.

Issues Resolved

[List any issues this PR will resolve]

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.

@satyajitg28 satyajitg28 requested review from a team and reta as code owners July 8, 2022 16:42
@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2022

Gradle Check (Jenkins) Run Completed with:

@satyajitg28 satyajitg28 marked this pull request as draft July 9, 2022 03:17
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@satyajitg28 satyajitg28 changed the title Integrate Engine with decoupled Translog interfaces [2.x] [Backport 2.x] Integrate Engine with decoupled Translog interfaces Jul 14, 2022
@satyajitg28 satyajitg28 marked this pull request as ready for review July 14, 2022 13:32
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@satyajitg28
Copy link
Author

looks like a flaky test.
./gradlew ':server:test' --tests "org.opensearch.index.shard.RemoveCorruptedShardDataCommandTests.testCorruptedTranslog" passed on local.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2022 •
edited
Loading

Codecov Report

Merging #3822 (1c08624) into 2.x (bb353b6) will decrease coverage by 0.05%.
The diff coverage is 73.45%.

@@             Coverage Diff              @@
##                2.x    #3822      +/-   ##
============================================
- Coverage     70.54%   70.49%   -0.06%     
+ Complexity    56546    56545       -1     
============================================
  Files          4533     4533              
  Lines        272158   272232      +74     
  Branches      40003    40004       +1     
============================================
- Hits         191994   191907      -87     
- Misses        64020    64234     +214     
+ Partials      16144    16091      -53
Impacted Files Coverage Δ
...opensearch/index/translog/NoOpTranslogManager.java 71.42% <ø> (+71.42%) ⬆️
...index/translog/listener/TranslogEventListener.java 80.00% <ø> (+13.33%) ⬆️
...search/index/translog/InternalTranslogManager.java 54.62% <25.00%> (+14.45%) ⬆️
...n/java/org/opensearch/index/engine/NoOpEngine.java 60.19% <52.63%> (-2.89%) ⬇️
.../opensearch/index/engine/NRTReplicationEngine.java 56.49% <62.79%> (-6.60%) ⬇️
...va/org/opensearch/index/engine/ReadOnlyEngine.java 71.69% <80.00%> (+1.42%) ⬆️
...va/org/opensearch/index/engine/InternalEngine.java 74.72% <80.76%> (-0.13%) ⬇️
...va/org/opensearch/index/engine/EngineTestCase.java 87.48% <93.33%> (+1.92%) ⬆️
.../main/java/org/opensearch/index/engine/Engine.java 74.79% <100.00%> (+1.51%) ⬆️
...nslog/listener/CompositeTranslogEventListener.java 96.42% <100.00%> (-0.13%) ⬇️
... and 497 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

Bukhtawar
Bukhtawar reviewed Jul 15, 2022
View reviewed changes
test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java Outdated
}
} finally {
IOUtils.close(replicaEngine, storeReplica, engine, store, () -> terminate(threadPool));
}
}

protected void assertEngineCleanedUp(Engine engine, Translog translog) throws Exception {
if (engine.isClosed.get() == false) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this check redundant

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it was redundant. Removed it.

server/src/test/java/org/opensearch/index/shard/IndexShardTests.java Outdated
@@ -4036,8 +4036,7 @@ public void testCloseShardWhileResettingEngine() throws Exception {
CountDownLatch closeDoneLatch = new CountDownLatch(1);
IndexShard shard = newStartedShard(false, Settings.EMPTY, config -> new InternalEngine(config) {
@Override
public InternalEngine recoverFromTranslog(TranslogRecoveryRunner translogRecoveryRunner, long recoverUpToSeqNo)
throws IOException {
public Engine recoverFromTranslog(TranslogRecoveryRunner translogRecoveryRunner, long recoverUpToSeqNo) throws IOException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we revert this change. Any reason why couldn't use TranslogEventListener here?

Copy link
Author

Choose a reason for hiding this comment

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

Reverted the above change. Using TranslogEventListener, the test is getting stuck. Will debug it further.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@satyajitg28 satyajitg28 requested a review from reta July 18, 2022 05:27
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@satyajitg28
Copy link
Author

Test failing: DiskThresholdDeciderIT.testHighWatermarkNotExceeded. Looks like a flaky test.

Satyajit Ganguly added 6 commits July 19, 2022 10:31
Removed new abstract methods from Engine
9c9a88c 
Signed-off-by: Satyajit Ganguly <satyajga@amazon.com>
NoOpEngine and ReadOnlyEngine changes to fix AsyncTranslogTrim test
4b876ca 
Signed-off-by: Satyajit Ganguly <satyajga@amazon.com>
IndexShardTests changes
49150b8 
Signed-off-by: Satyajit Ganguly <satyajga@amazon.com>
spotless checks
99969fe 
Signed-off-by: Satyajit Ganguly <satyajga@amazon.com>
RemoveCorruptedShardDataCommandTests changes in accordance with 2.x b…
555f250 
…ranch

Signed-off-by: Satyajit Ganguly <satyajga@amazon.com>
CR comments addressed
c78e81a 
Signed-off-by: Satyajit Ganguly <satyajga@amazon.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@satyajitg28
Copy link
Author

Again this test is failing: DiskThresholdDeciderIT.testHighWatermarkNotExceeded.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Synced IndexShard with 2.x
c00693f 
Signed-off-by: Satyajit Ganguly <satyajga@amazon.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@dblock
Copy link
Member

dblock commented Jul 21, 2022

@Bukhtawar care to take a look at this backport?

@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2022

Gradle Check (Jenkins) Run Completed with:

Integrated NRTReplicationEngine with TranslogManager
cf972a0 
Signed-off-by: Satyajit Ganguly <satyajga@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2022

Gradle Check (Jenkins) Run Completed with:

Fixed a test in NRTReplicationEngineTests
1c08624 
Signed-off-by: Satyajit Ganguly <satyajga@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2022

Gradle Check (Jenkins) Run Completed with:

@Bukhtawar Bukhtawar merged commit 57ce4a9 into opensearch-project:2.x Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reta reta reta approved these changes

@Bukhtawar Bukhtawar Awaiting requested review from Bukhtawar Bukhtawar is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@satyajitg28 @codecov-commenter @dblock @reta @Bukhtawar