Skip to content

[ZOOKEEPER-3642] Fix potential data inconsistency due to DIFF sync after partial SNAP sync #1224

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
Closed
lvfangmin wants to merge 1 commit into from
Closed

[ZOOKEEPER-3642] Fix potential data inconsistency due to DIFF sync after partial SNAP sync #1224

lvfangmin wants to merge 1 commit into from

Conversation

@lvfangmin
Copy link
Contributor

@lvfangmin lvfangmin commented Jan 17, 2020

No description provided.

@lvfangmin
Copy link
Contributor Author

lvfangmin commented Feb 12, 2020

@eolivelli @hanm interested to take a look at this, the potential issue is mentioned in the description of the Jira.

hanm
hanm approved these changes Feb 13, 2020
View reviewed changes
Copy link
Contributor

@hanm hanm left a comment

Choose a reason for hiding this comment

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

LGTM, with two nits.

zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
public synchronized void shutdown(boolean fullyShutDown) {
if (!canShutdown()) {
if (fullyShutDown && zkDb != null) {
zkDb.clear();
Copy link
Contributor

@hanm hanm Feb 13, 2020

Choose a reason for hiding this comment

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

It's not obvious to me how we could end up in this code path - if we are here then the shutdown must have been called and that specific shut down must not be a full shutdown. Do we have a concrete example when we will hit this code path?

Copy link
Contributor Author

@lvfangmin lvfangmin Feb 26, 2020

Choose a reason for hiding this comment

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

In case the follower exited before finish syncing with leader, the ZK server will not start, canShutdown will return false, but we still need to clear the db here.

zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java
new byte[1], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);

// wait some time to let this get written to disk
Thread.sleep(500);
Copy link
Contributor

@hanm hanm Feb 13, 2020

Choose a reason for hiding this comment

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

I think in general we advocate not using sleep (at least, not using sleep alone) in unit tests as it's not reliable and proved to be a source of flaky-ness. Is there a better approach to wait for the log being flushed to disk (preferably also verify that fact in test)?

Copy link
Contributor Author

@lvfangmin lvfangmin Feb 26, 2020

Choose a reason for hiding this comment

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

That's a reasonable concern, I'll try to find ways to check that in the test here.

@hanm
Copy link
Contributor

hanm commented Oct 19, 2020

@eolivelli do you want to give a look at this given you were tagged?

If no issues I will merge this once we get a green build on per-merge job (I think my nits are addressed - the sleeping in test is not ideal but I would not consider it's blocking).

eolivelli
eolivelli approved these changes Oct 19, 2020
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

@hanm please go ahead and merge

@hanm
Copy link
Contributor

hanm commented Oct 20, 2020

ok, I will merge after CI is back to good shape.

@eolivelli
Copy link
Contributor

eolivelli commented Oct 20, 2020

@hanm I have sent a fix for CI
#1510

@hanm
Copy link
Contributor

hanm commented Oct 21, 2020

thanks for quick fix on CI issue @eolivelli ; looks like the test needs update to work with latest master. I'll do a rebase if @lvfangmin didn't have a chance to rebase.

@eolivelli
Copy link
Contributor

eolivelli commented Oct 21, 2020

thank you @hanm
the fix should be pretty simple, ping me when you are ready

[2020-10-21T02:26:03.778Z] [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:testCompile (default-testCompile) on project zookeeper: Compilation failure

[2020-10-21T02:26:03.778Z] [ERROR] /home/jenkins/jenkins-home/workspace/eper-precommit-github-pr_PR-1224/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java:[1733,17] no suitable method found for assertNotNull(java.lang.String,org.apache.zookeeper.data.Stat)

[2020-10-21T02:26:03.778Z] [ERROR]     method org.junit.jupiter.api.Assertions.assertNotNull(java.lang.Object,java.util.function.Supplier<java.lang.String>) is not applicable

[2020-10-21T02:26:03.778Z] [ERROR]       (argument mismatch; org.apache.zookeeper.data.Stat cannot be converted to java.util.function.Supplier<java.lang.String>)

[2020-10-21T02:26:03.778Z] [ERROR]     method org.junit.jupiter.api.Assertions.assertNotNull(java.lang.Object,java.lang.String) is not applicable

[2020-10-21T02:26:03.778Z] [ERROR]       (argument mismatch; org.apache.zookeeper.data.Stat cannot be converted to java.lang.String)

[2020-10-21T02:26:03.778Z] [ERROR] 

[2020-10-21T02:26:03.778Z] [ERROR] -> [Help 1]

@hanm hanm mentioned this pull request Oct 24, 2020
Closed
@hanm
Copy link
Contributor

hanm commented Oct 24, 2020

@eolivelli #1515 is rebased version, thanks.

eolivelli pushed a commit that referenced this pull request Nov 23, 2020
@hanm @eolivelli
ZOOKEEPER-3642: Fix potential data inconsistency due to DIFF sync aft…
efbd660 
…er partial SNAP sync.

Based on #1224 ; fixed unit test build issue.

Author: Fangmin Lyu <fangmin@apache.org>
Author: Michael Han <hanm@apache.org>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Originally developed by Fangmin Lyu <fangmin@apache.org>

Closes #1515 from hanm/ZOOKEEPER-3642
asfgit pushed a commit that referenced this pull request May 17, 2022
@MuktiKrishnan
ZOOKEEPER-3642: Fix potential data inconsistency due to DIFF sync aft…
161e505 
…er partial SNAP sync.

Based on #1224 ; fixed unit test build issue.

Author: Fangmin Lyu <fangmin@apache.org>
Author: Michael Han <hanm@apache.org>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Originally developed by Fangmin Lyu <fangmin@apache.org>

Closes #1515 from hanm/ZOOKEEPER-3642

(cherry picked from commit a53cfeb)
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Aug 31, 2022
@hanm @RokLenarcic
ZOOKEEPER-3642: Fix potential data inconsistency due to DIFF sync aft…
0b973ef 
…er partial SNAP sync.

Based on apache#1224 ; fixed unit test build issue.

Author: Fangmin Lyu <fangmin@apache.org>
Author: Michael Han <hanm@apache.org>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Originally developed by Fangmin Lyu <fangmin@apache.org>

Closes apache#1515 from hanm/ZOOKEEPER-3642
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Aug 31, 2022
@hanm @RokLenarcic
ZOOKEEPER-3642: Fix potential data inconsistency due to DIFF sync aft…
bc5058d 
…er partial SNAP sync.

Based on apache#1224 ; fixed unit test build issue.

Author: Fangmin Lyu <fangmin@apache.org>
Author: Michael Han <hanm@apache.org>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Originally developed by Fangmin Lyu <fangmin@apache.org>

Closes apache#1515 from hanm/ZOOKEEPER-3642
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Aug 31, 2022
@hanm @RokLenarcic
ZOOKEEPER-3642: Fix potential data inconsistency due to DIFF sync aft…
bc0b78b 
…er partial SNAP sync.

Based on apache#1224 ; fixed unit test build issue.

Author: Fangmin Lyu <fangmin@apache.org>
Author: Michael Han <hanm@apache.org>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Originally developed by Fangmin Lyu <fangmin@apache.org>

Closes apache#1515 from hanm/ZOOKEEPER-3642
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Sep 3, 2022
@hanm @RokLenarcic
ZOOKEEPER-3642: Fix potential data inconsistency due to DIFF sync aft…
5f7fd34 
…er partial SNAP sync.

Based on apache#1224 ; fixed unit test build issue.

Author: Fangmin Lyu <fangmin@apache.org>
Author: Michael Han <hanm@apache.org>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Originally developed by Fangmin Lyu <fangmin@apache.org>

Closes apache#1515 from hanm/ZOOKEEPER-3642
@jonmv jonmv mentioned this pull request Sep 23, 2022
Closed
@kezhuw
Copy link
Member

kezhuw commented Aug 12, 2024

Superceded by #1515.

@kezhuw kezhuw closed this Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hanm hanm hanm approved these changes

@eolivelli eolivelli eolivelli approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lvfangmin @hanm @eolivelli @kezhuw