fix Flaky-test: BookieZKExpireTest.testBookieServerZKSessionExpireBehaviour #3418

wenbingshen · 2022-07-23T08:24:11Z

Motivation

fix Flaky-test: BookieZKExpireTest.testBookieServerZKSessionExpireBehaviour by disable retry session expired.

issue #3206 was fixed by PR #3415, but it still introduced another flaky test: BookieZKExpireTest.testBookieServerZKSessionExpireBehaviour

According to the investigation, the reason is:
ZookeeperClient still creates a new zookeeper instance when the old zookeeper client session time out.

Due to the asynchronous execution of two threads executing bookie temporary node re-registration and zk instance re-creation, the test program sometimes succeeds and sometimes fails.

When the temporary node re-registration is performed before the zk re-instantiation, the temporary node creation will use the old zk instance, which will cause a session timeout error, the bookie service will be shutdown, and the test will be successful;
When the zk re-instantiation precedes the re-registration of the temporary node, the temporary node creation will use the new re-instantiated zk instance, then the temporary node will be successfully created, the bookie service is running normally, and the test fails.

        try {
            connectExecutor.submit(clientCreator);
        } catch (RejectedExecutionException ree) {
            if (!closed.get()) {
                logger.error("ZooKeeper reconnect task is rejected : ", ree);
            }
        } catch (Exception t) {
            logger.error("Failed to submit zookeeper reconnect task due to runtime exception : ", t);
        }

Changes

Add a retryExpired flag to indicate whether to run the zk instance and retry to create a new instance after the session times out.
Set this flag to false for ZKMetadataBookieDriver;
Other ZookeeperClient and normal ZookeeperClient applications will generate the default value true or set to true, which is consistent with the original behavior.

Test the behavior of this PR:

Before this PR:
Executed the test 10 times, all failed.

After this PR:
Executed the test 10 times, all successful.

…aviour by disable retry session expired

wenbingshen · 2022-07-23T08:25:54Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/zookeeper/ZooKeeperClient.java

+
+        if (!retryExpired) {
+            logger.warn("ZooKeeper session expired retries have been disabled");
+            return;
+        }
+


Recreating new zookeeper instances is prohibited here.

wenbingshen · 2022-07-23T08:31:06Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieZKExpireTest.java

+            // allow client.waitForConnection() timeout
+            Thread.sleep(10000);
            assertFalse("Bookie should shutdown on losing zk session", server.isBookieRunning());
            assertFalse("Bookie Server should shutdown on losing zk session", server.isRunning());


The increase from 3 seconds to 10 seconds is because creating a bookie temporary node needs to wait for the old zk client to establish a connection with the server. There is a timeout period of conf.getZkTimeout()=6 seconds. Once the timeout period expires, the bookie service will Closed due to session timeout, the extra 4 seconds is to consolidate the stability of the test and give the bookie service a little extra time to perform the shutdown.

wenbingshen · 2022-07-23T08:32:33Z

ping @dlg99 @hangc0276 @shoothzj @eolivelli PTAL.

HQebupt · 2022-07-23T11:36:13Z

@StevenLuMT PTAL

StevenLuMT

Is there a better way to fix it?

StevenLuMT · 2022-07-24T01:25:23Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java

@@ -165,7 +165,8 @@ public String getScheme() {
    protected void initialize(AbstractConfiguration<?> conf,
                              StatsLogger statsLogger,
                              RetryPolicy zkRetryPolicy,
-                              Optional<Object> optionalCtx) throws MetadataException {
+                              Optional<Object> optionalCtx,
+                              boolean zkRetryExpired) throws MetadataException {


this check(zkRetryExpired) is difficult to understand, what scene is set to true and what scene is set to false

dlg99 · 2022-07-26T00:24:37Z

can you predictably repro the problem? Maybe my fix was not good enough :( but I only could repro it under docker with limited number of CPUs and after the fix it did not fail after running in the loop 150 times.
BookieStateManager's registration listener will shutdown the bookie if it cannot reconnect immediately so I guess there is still some timing issue between it and the test killing zk session / letting it recover.

StevenLuMT · 2022-08-24T07:59:25Z

fix old workflow,please see #3455 for detail

poorbarcode · 2023-12-05T09:34:54Z

@dlg99

can you predictably repro the problem? Maybe my fix was not good enough :( but I only could repro it under docker with limited number of CPUs and after the fix it did not fail after running in the loop 150 times.
BookieStateManager's registration listener will shutdown the bookie if it cannot reconnect immediately so I guess there is still some timing issue between it and the test killing zk session / letting it recover.

When I run the test in my local, it is very flaky

hangc0276 · 2023-12-06T01:44:52Z

@dlg99

can you predictably repro the problem? Maybe my fix was not good enough :( but I only could repro it under docker with limited number of CPUs and after the fix it did not fail after running in the loop 150 times.
BookieStateManager's registration listener will shutdown the bookie if it cannot reconnect immediately so I guess there is still some timing issue between it and the test killing zk session / letting it recover.

When I run the test in my local, it is very flaky

@poorbarcode There is a race condition describe in the motivation.

…4144) ### Motivation The `BookieZKExpireTest.testBookieServerZKSessionExpireBehaviour` is a flaky test and the root cause is described in #3418. There is a race condition in the zookeeper reconnection and listener and it is an expected behavior in Bookie. We created one issue to track this #4142. We can skip this flaky test to unblock the pending PRs.

…4144) ### Motivation The `BookieZKExpireTest.testBookieServerZKSessionExpireBehaviour` is a flaky test and the root cause is described in #3418. There is a race condition in the zookeeper reconnection and listener and it is an expected behavior in Bookie. We created one issue to track this #4142. We can skip this flaky test to unblock the pending PRs. (cherry picked from commit 11ccebb)

…pache#4144) ### Motivation The `BookieZKExpireTest.testBookieServerZKSessionExpireBehaviour` is a flaky test and the root cause is described in apache#3418. There is a race condition in the zookeeper reconnection and listener and it is an expected behavior in Bookie. We created one issue to track this apache#4142. We can skip this flaky test to unblock the pending PRs.

…pache#4144) ### Motivation The `BookieZKExpireTest.testBookieServerZKSessionExpireBehaviour` is a flaky test and the root cause is described in apache#3418. There is a race condition in the zookeeper reconnection and listener and it is an expected behavior in Bookie. We created one issue to track this apache#4142. We can skip this flaky test to unblock the pending PRs. (cherry picked from commit 11ccebb)

fix Flaky-test: BookieZKExpireTest.testBookieServerZKSessionExpireBeh…

bbca7b6

…aviour by disable retry session expired

wenbingshen commented Jul 23, 2022

View reviewed changes

mock retryExpired anyBoolean in mockZkBuilder

74f3f5c

StevenLuMT requested changes Jul 24, 2022

View reviewed changes

hangc0276 assigned wenbingshen Jul 25, 2022

hangc0276 added the flaky-tests label Jul 25, 2022

hangc0276 added this to the 4.16.0 milestone Jul 25, 2022

hangc0276 modified the milestones: 4.16.0, 4.17.0 Jul 29, 2022

wenbingshen mentioned this pull request Sep 20, 2022

Add RegistrationSessionExpiredPolicy for session expired event #3495

Open

hangc0276 mentioned this pull request Dec 6, 2023

Use Flaky flag to skip testBookieServerZKSessionExpireBehaviour test #4144

Merged

eolivelli removed this from the 4.17.0 milestone May 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix Flaky-test: BookieZKExpireTest.testBookieServerZKSessionExpireBehaviour #3418

fix Flaky-test: BookieZKExpireTest.testBookieServerZKSessionExpireBehaviour #3418

wenbingshen commented Jul 23, 2022

wenbingshen Jul 23, 2022

wenbingshen Jul 23, 2022

wenbingshen commented Jul 23, 2022

HQebupt commented Jul 23, 2022

StevenLuMT left a comment

StevenLuMT Jul 24, 2022

dlg99 commented Jul 26, 2022

StevenLuMT commented Aug 24, 2022

poorbarcode commented Dec 5, 2023

hangc0276 commented Dec 6, 2023

fix Flaky-test: BookieZKExpireTest.testBookieServerZKSessionExpireBehaviour #3418

Are you sure you want to change the base?

fix Flaky-test: BookieZKExpireTest.testBookieServerZKSessionExpireBehaviour #3418

Conversation

wenbingshen commented Jul 23, 2022

Motivation

Changes

Test the behavior of this PR:

wenbingshen Jul 23, 2022

Choose a reason for hiding this comment

wenbingshen Jul 23, 2022

Choose a reason for hiding this comment

wenbingshen commented Jul 23, 2022

HQebupt commented Jul 23, 2022

StevenLuMT left a comment

Choose a reason for hiding this comment

StevenLuMT Jul 24, 2022

Choose a reason for hiding this comment

dlg99 commented Jul 26, 2022

StevenLuMT commented Aug 24, 2022

poorbarcode commented Dec 5, 2023

hangc0276 commented Dec 6, 2023