[fix] [broker] Fix isolated group not work problem. by horizonzy · Pull Request #21096 · apache/pulsar

horizonzy · 2023-08-30T17:15:21Z

Modifications

When upgraded the pulsar version from 2.9.2 to 2.10.3, and the isolated group feature not work anymore.
Finally, we found the problem. In IsolatedBookieEnsemblePlacementPolicy, when it gets the bookie rack from the metadata store cache, uses future.isDone() to avoid sync operation. If the future is incomplete, return empty blacklists.
The cache may expire due to the caffeine cache getExpireAfterWriteMillis config, if the cache expires, the future may be incomplete. (#21095 will correct the behavior)

In 2.9.2, it uses the sync to get data from the metadata store, we should also keep the behavior.

Documentation

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

Matching PR in forked repository

PR in forked repository:

hangc0276

Please add a test to protect this logic.

codecov-commenter · 2023-08-31T01:40:32Z

Codecov Report

❌ Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.18%. Comparing base (b26ee8a) to head (c033ac0).
⚠️ Report is 2188 commits behind head on master.

Files with missing lines	Patch %	Lines
...areness/IsolatedBookieEnsemblePlacementPolicy.java	64.28%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #21096       +/-   ##
=============================================
+ Coverage     33.46%   73.18%   +39.72%     
- Complexity    12179    32436    +20257     
=============================================
  Files          1623     1887      +264     
  Lines        127399   139985    +12586     
  Branches      13929    15413     +1484     
=============================================
+ Hits          42631   102449    +59818     
+ Misses        79158    29440    -49718     
- Partials       5610     8096     +2486

Flag	Coverage Δ
inttests	`24.14% <0.00%> (+0.06%)`	⬆️
systests	`25.11% <0.00%> (?)`
unittests	`72.47% <64.28%> (+40.52%)`	⬆️

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

Files with missing lines	Coverage Δ
...areness/IsolatedBookieEnsemblePlacementPolicy.java	`87.78% <64.28%> (+87.78%)`	⬆️

... and 1532 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Technoboy-

LGTM

hangc0276

Nice catch!

codelipenghui · 2023-09-01T13:39:34Z

+                Optional<BookiesRackConfiguration> optional =
+                        bookieMappingCache.get(BookieRackAffinityMapping.BOOKIE_INFO_ROOT_PATH).get(
+                                metaOpTimeout, TimeUnit.MILLISECONDS);


Can the zookeeper thread reach here? Just to confirm it will not introduce a deadlock.

I discuss with hang, and we will use a graceful way to avoid sync operation. I will push a next commit later.

hangc0276

LGTM. Please take a look at the failed tests, thanks.

eolivelli

+1
Great catch

codelipenghui

I would like to provide another approach for handling the cached rack configuration update.

Make allGroupsBookieMapping as a volatile variable
Update the cached rack configuration async

initialize

bookieMappingCache.get(BookieRackAffinityMapping.BOOKIE_INFO_ROOT_PATH).thenAccept(opt -> {
                opt.ifPresent(bookiesRackConfiguration -> cachedRackConfiguration = bookiesRackConfiguration);
            }).exceptionally(ex -> {
                log.warn("Failed to load bookies rack configuration while initialize the PlacementPolicy.");
                return null;
            });

getExcludedBookiesWithIsolationGroups

                CompletableFuture<Optional<BookiesRackConfiguration>> future =
                        bookieMappingCache.get(BookieRackAffinityMapping.BOOKIE_INFO_ROOT_PATH);

                BookiesRackConfiguration allGroupsBookieMapping;
                future.thenAccept(opt ->
                        opt.ifPresent(bookiesRackConfiguration -> cachedRackConfiguration = bookiesRackConfiguration)
                    ).exceptionally(ex -> {
                        log.warn("The newest bookie rack config is not available now.");
                        return null;
                    });
                allGroupsBookieMapping = cachedRackConfiguration;
                if (allGroupsBookieMapping == null) {
                    return excludedBookies;
                }

When the broker restarts, we might lose the rack configurations, but it can avoid any potential deadlocks.

The initialize method is called when starting the broker before the broker service becomes available to the client. So we will not lose the bookie rack configuration in most cases.

codelipenghui · 2023-09-04T04:00:01Z

+                                cachedRackConfiguration);
+                        allGroupsBookieMapping = cachedRackConfiguration;
+                    } else {
+                        throw new KeeperException.NoNodeException(ZkBookieRackAffinityMapping.BOOKIE_INFO_ROOT_PATH);


How bookie client will handle this exception, and what is the expected behavior from the producer/consumer's perspective? Will the producer/consumer be closed by this exception?

Not, we already caught this exception at line_284

horizonzy · 2023-09-04T04:24:14Z

 

I would like to provide another approach for handling the cached rack configuration update.

Make allGroupsBookieMapping as a volatile variable
Update the cached rack configuration async

initialize

bookieMappingCache.get(BookieRackAffinityMapping.BOOKIE_INFO_ROOT_PATH).thenAccept(opt -> {
                opt.ifPresent(bookiesRackConfiguration -> cachedRackConfiguration = bookiesRackConfiguration);
            }).exceptionally(ex -> {
                log.warn("Failed to load bookies rack configuration while initialize the PlacementPolicy.");
                return null;
            });

getExcludedBookiesWithIsolationGroups

                CompletableFuture<Optional<BookiesRackConfiguration>> future =
                        bookieMappingCache.get(BookieRackAffinityMapping.BOOKIE_INFO_ROOT_PATH);

                BookiesRackConfiguration allGroupsBookieMapping;
                future.thenAccept(opt ->
                        opt.ifPresent(bookiesRackConfiguration -> cachedRackConfiguration = bookiesRackConfiguration)
                    ).exceptionally(ex -> {
                        log.warn("The newest bookie rack config is not available now.");
                        return null;
                    });
                allGroupsBookieMapping = cachedRackConfiguration;
                if (allGroupsBookieMapping == null) {
                    return excludedBookies;
                }

When the broker restarts, we might lose the rack configurations, but it can avoid any potential deadlocks.

The initialize method is called when starting the broker before the broker service becomes available to the client. So we will not lose the bookie rack configuration in most cases.

It makes sense, we should print a debug log when the allGroupsBookieMapping is null.

codelipenghui · 2023-09-04T04:37:14Z

It makes sense, we should print a debug log when the allGroupsBookieMapping is null.

Yes, sure.

horizonzy · 2023-09-04T04:43:38Z

It makes sense, we should print a debug log when the allGroupsBookieMapping is null.

Yes, sure.

There is a case that needs discussed, we only update the rack config when the opt.isPresent. If the user deletes the zk nodes of path /bookies, the opt.isPresent will be false. Shall we delete the cached rack config or still use it?

hangc0276 · 2023-09-04T07:27:44Z

It makes sense, we should print a debug log when the allGroupsBookieMapping is null.

Yes, sure.

There is a case that needs discussed, we only update the rack config when the opt.isPresent. If the user deletes the zk nodes of path /bookies, the opt.isPresent will be false. Shall we delete the cached rack config or still use it?

@horizonzy We need to delete the cached rack configuration.

horizonzy · 2023-09-04T07:29:54Z

It makes sense, we should print a debug log when the allGroupsBookieMapping is null.

Yes, sure.

There is a case that needs discussed, we only update the rack config when the opt.isPresent. If the user deletes the zk nodes of path /bookies, the opt.isPresent will be false. Shall we delete the cached rack config or still use it?

@horizonzy We need to delete the cached rack configuration.

What do you think of it?

hangc0276 · 2023-09-04T08:00:38Z

It makes sense, we should print a debug log when the allGroupsBookieMapping is null.

Yes, sure.

There is a case that needs discussed, we only update the rack config when the opt.isPresent. If the user deletes the zk nodes of path /bookies, the opt.isPresent will be false. Shall we delete the cached rack config or still use it?

@horizonzy We need to delete the cached rack configuration.

What do you think of it?

@horizonzy Do checking the exception type work? If the exception is a NoNode exception, update the cached rack configuration to null.

horizonzy · 2023-09-04T10:07:04Z

It makes sense, we should print a debug log when the allGroupsBookieMapping is null.

Yes, sure.

There is a case that needs discussed, we only update the rack config when the opt.isPresent. If the user deletes the zk nodes of path /bookies, the opt.isPresent will be false. Shall we delete the cached rack config or still use it?

@horizonzy We need to delete the cached rack configuration.

What do you think of it?

@horizonzy Do checking the exception type work? If the exception is a NoNode exception, update the cached rack configuration to null.

It doesn't work. The zk metadata store handles it separately, not throw exceptions.

pulsar/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java

Lines 267 to 269 in b26ee8a

    
           if (code == Code.NONODE) { 
        
               // For get operations, we return an empty optional 
        
               op.getFuture().complete(Optional.empty());

hangc0276 · 2023-09-04T11:02:16Z

-                        bookieMappingCache.get(BookieRackAffinityMapping.BOOKIE_INFO_ROOT_PATH);
+                bookieMappingCache.get(BookieRackAffinityMapping.BOOKIE_INFO_ROOT_PATH)
+                        .thenAccept(opt -> cachedRackConfiguration = opt.orElse(null)).exceptionally(e -> {
+                            log.warn("Failed to update the newest bookies rack config.");


Change the log msg to log.warn("Failed to update the newest bookies rack config, and use the cached rack configuration: {}", cachedRackConfiguration)

This is an async operation, it won't affect the result of the current invocation, and printing this log may be misleading.

codelipenghui · 2023-09-04T23:16:53Z

/pulsarbot run-failure-checks

### Modifications When upgraded the pulsar version from 2.9.2 to 2.10.3, and the isolated group feature not work anymore. Finally, we found the problem. In IsolatedBookieEnsemblePlacementPolicy, when it gets the bookie rack from the metadata store cache, uses future.isDone() to avoid sync operation. If the future is incomplete, return empty blacklists. The cache may expire due to the caffeine cache `getExpireAfterWriteMillis` config, if the cache expires, the future may be incomplete. (#21095 will correct the behavior) In 2.9.2, it uses the sync to get data from the metadata store, we should also keep the behavior.

When upgraded the pulsar version from 2.9.2 to 2.10.3, and the isolated group feature not work anymore. Finally, we found the problem. In IsolatedBookieEnsemblePlacementPolicy, when it gets the bookie rack from the metadata store cache, uses future.isDone() to avoid sync operation. If the future is incomplete, return empty blacklists. The cache may expire due to the caffeine cache `getExpireAfterWriteMillis` config, if the cache expires, the future may be incomplete. (#21095 will correct the behavior) In 2.9.2, it uses the sync to get data from the metadata store, we should also keep the behavior.

shibd · 2023-10-22T03:03:08Z

@horizonzy Can you help create a PR to cherry-pick this change to branch-2.11?

horizonzy · 2023-10-23T03:06:31Z

@horizonzy Can you help create a PR to cherry-pick this change to branch-2.11?

Ok.

Fix isolated group not work problem.

2f435cc

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Aug 30, 2023

This was referenced Aug 30, 2023

[branch-2.10] [fix][broker] Fix isolated group not work #21097

Closed

[branch-2.10] [fix] [broker] Fix isolated group not work problem. #21098

Merged

hangc0276 reviewed Aug 30, 2023

View reviewed changes

hangc0276 requested review from cbornet, codelipenghui, gaozhangmin, mattisonchao, merlimat, poorbarcode and zymap August 30, 2023 23:43

hangc0276 assigned horizonzy Aug 30, 2023

hangc0276 added type/bug The PR fixed a bug or issue reported a bug area/broker ready-to-test category/reliability The function does not work properly in certain specific environments or failures. e.g. data lost release/3.0.2 release/2.11.3 release/3.1.1 labels Aug 30, 2023

hangc0276 added this to the 3.2.0 milestone Aug 30, 2023

add tests.

1f2126a

Technoboy- approved these changes Aug 31, 2023

View reviewed changes

hangc0276 approved these changes Aug 31, 2023

View reviewed changes

tune code.

478a1db

codelipenghui reviewed Sep 1, 2023

View reviewed changes

cache the RackConfiguration to avoid sync operation.

ec10843

hangc0276 approved these changes Sep 2, 2023

View reviewed changes

eolivelli approved these changes Sep 3, 2023

View reviewed changes

zymap approved these changes Sep 4, 2023

View reviewed changes

poorbarcode approved these changes Sep 4, 2023

View reviewed changes

codelipenghui reviewed Sep 4, 2023

View reviewed changes

Address the comments.

301cb90

hangc0276 reviewed Sep 4, 2023

View reviewed changes

horizonzy added 3 commits September 4, 2023 19:07

Fix checkstyle.

c79568e

Merge branch 'master' into fix-isolated-group-not-work-problem

8318516

Fix ci.

c033ac0

codelipenghui approved these changes Sep 4, 2023

View reviewed changes

hangc0276 merged commit abd7bfa into apache:master Sep 5, 2023

Technoboy- added cherry-picked/branch-3.1 cherry-picked/branch-3.0 labels Sep 5, 2023

codelipenghui added cherry-picked/branch-2.10 release/2.10.6 labels Oct 7, 2023

horizonzy mentioned this pull request Oct 23, 2023

[fix] [broker] [branch-2.11] Fix isolated group not work problem #21415

Merged

4 tasks

shibd added the cherry-picked/branch-2.11 label Oct 26, 2023

Technoboy- mentioned this pull request Dec 25, 2023

[Bug] Broker gets block while creating topic using IsolatedBookieEnsemblePlacement in missing cache #20148

Closed

2 tasks

lhotari mentioned this pull request May 13, 2024

[Bug] Deadlock in broker service while initializing bkClient #22699

Closed

3 tasks

Conversation

horizonzy commented Aug 30, 2023 • edited by Technoboy- Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Modifications

Documentation

Matching PR in forked repository

Uh oh!

hangc0276 left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Technoboy- left a comment

Choose a reason for hiding this comment

Uh oh!

hangc0276 left a comment

Choose a reason for hiding this comment

Uh oh!

codelipenghui Sep 1, 2023

Choose a reason for hiding this comment

Uh oh!

horizonzy Sep 1, 2023

Choose a reason for hiding this comment

Uh oh!

hangc0276 left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

codelipenghui left a comment

Choose a reason for hiding this comment

Uh oh!

codelipenghui Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

horizonzy Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

horizonzy commented Sep 4, 2023

Uh oh!

codelipenghui commented Sep 4, 2023

Uh oh!

horizonzy commented Sep 4, 2023

Uh oh!

hangc0276 commented Sep 4, 2023

Uh oh!

horizonzy commented Sep 4, 2023

Uh oh!

hangc0276 commented Sep 4, 2023

Uh oh!

horizonzy commented Sep 4, 2023

Uh oh!

hangc0276 Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

horizonzy Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

hangc0276 Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

codelipenghui commented Sep 4, 2023

Uh oh!

shibd commented Oct 22, 2023

Uh oh!

horizonzy commented Oct 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

horizonzy commented Aug 30, 2023 •

edited by Technoboy-

Loading

codecov-commenter commented Aug 31, 2023 •

edited

Loading