[fix][test] Fix flaky test deleteNamespaceGracefully #18220

poorbarcode · 2022-10-27T09:07:28Z

Motivation

https://github.com/poorbarcode/pulsar/actions/runs/3267034877/jobs/5371836130
https://github.com/apache/pulsar/actions/runs/3242148385/jobs/5326408439

The original implementation determines whether to wait for the creation of __chang_event based on whether a broker has taken ownership of the bundle or not. However, there are often 'set namespace policy', 'unload namespace', 'delete namespace', 'split bundle', and other operations in the test case. When the bundle unload and the 'bundle checkconcurrently execute, the methoddeleteNamespaceGraceFully` will run unstably.

Modifications

Instead of waiting for __change_evnt to be created, it would be simpler to try again if it failed.
change method name deleteNamespaceGraceFully -> deleteNamespaceWithRetry
Originally, deleteNamespaceGraceFully in class BrokerTestBase, and then MockedPulsarServiceBaseTest called BrokerTestBase.deleteNamespaceGraceFully(). But MockedPulsarServiceBaseTest is the super class of BrokerTestBase, so move method deleteNamespaceGraceFully to MockedPulsarServiceBaseTest.

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository:

[ci][fix][test]fix flaky test deleteNamespaceGracefully poorbarcode/pulsar#31

nodece · 2022-10-31T04:10:56Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApi2Test.java

        for (String tenant : admin.tenants().getTenants()) {
            for (String namespace : admin.namespaces().getNamespaces(tenant)) {
-                deleteNamespaceGraceFullyByMultiPulsars(namespace, true, admin, pulsar,
+                deleteNamespaceGraceFully(namespace, true, admin, pulsar,


I have a question, can we use the admin.namespaces().deleteNamespace() instead of this?

deleteNamespaceGraceFully looks like a hack operation.

When admin.namespaces().deleteNamespace() and auto create topic __change_event are concurrent executed, there is a problem #17070, and deleteNamespaceGraceFully is used to solve this problem

I try to fix the flaky test this way: make delete namespace after __change_events is successfully created, but there still has another race condition: the __change_events/__compaction async creates and __change_events delete by namespace delete. Therefore, I will disabled systemTopic in method testDeleteTenant to solve this flaky test.

Do you point?

If right, I suggest we should fix this first.

In user use, the probability of occurrence is almost 0. Even if it actually occurs, the user can try to execute the delete namespace again. But to solve this problem fundamentally requires a big change

gaoran10 · 2022-11-20T14:12:33Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java

+    }
+
+    /**
+     * Wait until system topic "__change_event" and subscription "__compaction" are created, and then delete the namespace.


Does the description need to be modified?

Already fixed.

gaoran10 · 2022-11-20T14:13:10Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java

+            canPausedNamespaceService.pause();
+        }
+
+        Awaitility.await().until(() -> {


Maybe we can add the most wait time.

Already fixed.

nodece · 2022-11-22T04:13:49Z

@poorbarcode Could you fix these conflicts?

codecov-commenter · 2022-11-23T16:37:40Z

Codecov Report

Merging #18220 (801b233) into master (cd85a67) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #18220      +/-   ##
============================================
- Coverage     47.39%   47.35%   -0.05%     
- Complexity    10479    10483       +4     
============================================
  Files           698      698              
  Lines         68070    68070              
  Branches       7279     7279              
============================================
- Hits          32264    32235      -29     
- Misses        32228    32255      +27     
- Partials       3578     3580       +2

Flag	Coverage Δ
unittests	`47.35% <ø> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../pulsar/broker/service/SharedConsumerAssignor.java	`3.70% <0.00%> (-74.08%)`	⬇️
...apache/pulsar/broker/service/EntryAndMetadata.java	`0.00% <0.00%> (-40.75%)`	⬇️
.../apache/pulsar/broker/loadbalance/LoadManager.java	`61.11% <0.00%> (-16.67%)`	⬇️
...sistent/PersistentDispatcherMultipleConsumers.java	`51.55% <0.00%> (-7.96%)`	⬇️
...pulsar/broker/service/PulsarCommandSenderImpl.java	`73.84% <0.00%> (-4.62%)`	⬇️
.../pulsar/broker/service/BrokerServiceException.java	`42.59% <0.00%> (-3.71%)`	⬇️
...tent/PersistentDispatcherSingleActiveConsumer.java	`55.17% <0.00%> (-3.14%)`	⬇️
...oker/service/schema/SchemaRegistryServiceImpl.java	`60.66% <0.00%> (-2.71%)`	⬇️
...g/apache/pulsar/broker/admin/impl/BrokersBase.java	`60.45% <0.00%> (-1.37%)`	⬇️
...apache/pulsar/proxy/server/LookupProxyHandler.java	`56.46% <0.00%> (-1.30%)`	⬇️
... and 36 more

poorbarcode · 2022-11-23T17:06:08Z

/pulsarbot rerun-failure-checks

poorbarcode · 2022-11-24T02:31:21Z

/pulsarbot rerun-failure-checks

poorbarcode · 2022-11-24T04:20:09Z

/pulsarbot rerun-failure-checks

poorbarcode · 2022-11-24T05:24:07Z

/pulsarbot rerun-failure-checks

poorbarcode · 2022-11-24T06:14:55Z

@nodece

Could you fix these conflicts?

Already fixed.

(cherry picked from commit c544ea3)

This reverts commit a3e593a.

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 27, 2022

poorbarcode force-pushed the flaky/deleteNsGraceFully branch from 7cb7b20 to 05212ce Compare October 27, 2022 09:12

poorbarcode mentioned this pull request Oct 28, 2022

[ci][fix][test]fix flaky test deleteNamespaceGracefully poorbarcode/pulsar#31

Closed

nodece reviewed Oct 31, 2022

View reviewed changes

poorbarcode force-pushed the flaky/deleteNsGraceFully branch from 05212ce to dce5599 Compare November 1, 2022 18:41

poorbarcode force-pushed the flaky/deleteNsGraceFully branch from dce5599 to d0d9f3f Compare November 10, 2022 13:39

poorbarcode force-pushed the flaky/deleteNsGraceFully branch 2 times, most recently from 8dc781c to d740815 Compare November 18, 2022 13:06

gaoran10 reviewed Nov 20, 2022

View reviewed changes

Technoboy- assigned poorbarcode Nov 21, 2022

Technoboy- added this to the 2.12.0 milestone Nov 21, 2022

Technoboy- added area/test ready-to-test labels Nov 21, 2022

Technoboy- closed this Nov 21, 2022

Technoboy- reopened this Nov 21, 2022

Technoboy- changed the title ~~[fix][test]fix flaky test deleteNamespaceGracefully~~ [fix][test] Fix flaky test deleteNamespaceGracefully Nov 21, 2022

poorbarcode force-pushed the flaky/deleteNsGraceFully branch 2 times, most recently from ebec5a7 to 882cd56 Compare November 21, 2022 08:58

nodece approved these changes Nov 21, 2022

View reviewed changes

gaoran10 approved these changes Nov 22, 2022

View reviewed changes

poorbarcode added 2 commits November 23, 2022 23:53

[fix][test]fix flaky test deleteNamespaceGracefully

5e21215

simpler impl: just retry

d442374

poorbarcode force-pushed the flaky/deleteNsGraceFully branch from 882cd56 to d442374 Compare November 23, 2022 15:56

delete CanPausedNamespaceService

801b233

nodece merged commit c544ea3 into apache:master Nov 24, 2022

poorbarcode deleted the flaky/deleteNsGraceFully branch November 24, 2022 09:57

lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Dec 9, 2022

[fix][test] Fix flaky test deleteNamespaceGracefully (apache#18220)

d44a4dd

lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Jan 10, 2023

[fix][test] Fix flaky test deleteNamespaceGracefully (apache#18220)

0e5c7ca

liangyepianzhou pushed a commit that referenced this pull request Feb 25, 2023

[fix][test] Fix flaky test deleteNamespaceGracefully (#18220)

a3e593a

(cherry picked from commit c544ea3)

liangyepianzhou mentioned this pull request Feb 25, 2023

[branch-2.10][test]Run and fix tests #19636

Merged

15 tasks

liangyepianzhou added a commit that referenced this pull request Feb 26, 2023

Revert "[fix][test] Fix flaky test deleteNamespaceGracefully (#18220)"

ab8af95

This reverts commit a3e593a.

This was referenced May 24, 2023

Flaky-test: AdminApi2Test.resetClusters #20376

Open

[fix][broker] partitioned __change_events topic is policy topic #20392

Merged

[fix][test] Fix flaky test deleteNamespaceGracefully #18220

[fix][test] Fix flaky test deleteNamespaceGracefully #18220

Uh oh!

Conversation

poorbarcode commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Documentation

Matching PR in forked repository

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodece commented Nov 22, 2022

Uh oh!

codecov-commenter commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

poorbarcode commented Nov 23, 2022

Uh oh!

poorbarcode commented Nov 24, 2022

Uh oh!

poorbarcode commented Nov 24, 2022

Uh oh!

poorbarcode commented Nov 24, 2022

Uh oh!

poorbarcode commented Nov 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

poorbarcode commented Oct 27, 2022 •

edited

Loading

codecov-commenter commented Nov 23, 2022 •

edited

Loading