[fix][broker] Fix PulsarRegistrationClient and ZkRegistrationClient not aware rack info problem. #18672

horizonzy · 2022-11-29T10:53:34Z

Master Issue: #18671

Before #16825, the method getRack will update the rack info every invoke, so it can get the newest rack info.
After #16825, only Pulsar start or using pulsar admin set-bookie-rack to change zk node /bookies, then update the rack info. When the pulsar start, the PulsarRegistrationClient#bookieServiceInfoCache is empty; it can't resolve bookieId, so can't update the rack info.

Motivation

Make Pulsar and AutoRecovery aware of the rack info after its restart.

Modifications

After writeable bookies change, notify BookieRackAffinityMapping to update the rack info.

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end deployment with large payloads (10MB)
Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

Documentation

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

Matching PR in forked repository

PR in forked repository:

github-actions · 2022-11-29T10:54:00Z

@horizonzy Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

codecov-commenter · 2022-11-29T14:38:45Z

Codecov Report

Merging #18672 (24cff81) into master (bc1e0cb) will decrease coverage by 11.19%.
The diff coverage is 76.00%.

@@              Coverage Diff              @@
##             master   #18672       +/-   ##
=============================================
- Coverage     47.50%   36.31%   -11.19%     
- Complexity    10505    13890     +3385     
=============================================
  Files           698     1665      +967     
  Lines         67984   141229    +73245     
  Branches       7272    16809     +9537     
=============================================
+ Hits          32297    51294    +18997     
- Misses        32112    83994    +51882     
- Partials       3575     5941     +2366

Flag	Coverage Δ
inttests	`24.13% <76.00%> (?)`
unittests	`33.81% <68.00%> (-13.70%)`	⬇️

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

Impacted Files	Coverage Δ
...ookie/rackawareness/BookieRackAffinityMapping.java	`70.00% <72.22%> (ø)`
.../metadata/bookkeeper/PulsarRegistrationClient.java	`64.60% <85.71%> (ø)`
.../transaction/buffer/metadata/AbortTxnMetadata.java	`28.57% <0.00%> (-57.15%)`	⬇️
...a/org/apache/bookkeeper/mledger/ManagedCursor.java	`37.50% <0.00%> (-48.22%)`	⬇️
...java/org/apache/pulsar/proxy/stats/TopicStats.java	`58.82% <0.00%> (-41.18%)`	⬇️
...oker/loadbalance/LoadResourceQuotaUpdaterTask.java	`44.44% <0.00%> (-33.34%)`	⬇️
.../client/impl/schema/generic/GenericJsonWriter.java	`36.36% <0.00%> (-30.31%)`	⬇️
...pache/pulsar/broker/admin/v2/PersistentTopics.java	`46.82% <0.00%> (-27.29%)`	⬇️
...ent/impl/schema/generic/JsonRecordBuilderImpl.java	`36.11% <0.00%> (-25.80%)`	⬇️
.../apache/pulsar/broker/admin/impl/ClustersBase.java	`51.77% <0.00%> (-24.89%)`	⬇️
... and 1192 more

hangc0276 · 2022-11-30T01:31:40Z

@horizonzy Thanks for your contribution. We have changed the resolver and would you please help add a test to cover both two resolvers?

horizonzy · 2022-11-30T01:40:36Z

@horizonzy Thanks for your contribution. We have changed the resolver and would you please help add a test to cover both two resolvers?

Fine

yangl · 2022-12-12T01:14:15Z

Great work, fix the #18834

yangl

LGTM

yangl · 2022-12-13T01:51:53Z

btw, the else logic MetadataStore store should use the broker metadataStoreUrl, because when we set the rack info by admin bookies set-bookie-rack the configured json info on the broker used meta store url.

- String metadataServiceUri = ConfigurationStringUtil.castToString(conf.getProperty("metadataServiceUri"));
+ String metadataServiceUri = ConfigurationStringUtil.castToString(conf.getProperty("metadataStoreUrl"));

pulsar/pulsar-broker-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java

Lines 79 to 108 in 82237d3

    
               String url; 
        
               String metadataServiceUri = ConfigurationStringUtil.castToString(conf.getProperty("metadataServiceUri")); 
        
               if (StringUtils.isNotBlank(metadataServiceUri)) { 
        
                   try { 
        
                       url = metadataServiceUri.replaceFirst(METADATA_STORE_SCHEME + ":", "") 
        
                               .replace(";", ","); 
        
                   } catch (Exception e) { 
        
                       throw new MetadataException(Code.METADATA_SERVICE_ERROR, e); 
        
                   } 
        
               } else { 
        
                   String zkServers = ConfigurationStringUtil.castToString(conf.getProperty("zkServers")); 
        
                   if (StringUtils.isBlank(zkServers)) { 
        
                       String errorMsg = String.format("Neither %s configuration set in the BK client configuration nor " 
        
                               + "metadataServiceUri/zkServers set in bk server configuration", METADATA_STORE_INSTANCE); 
        
                       throw new RuntimeException(errorMsg); 
        
                   } 
        
                   url = zkServers; 
        
               } 
        
               try { 
        
                   int zkTimeout = Integer.parseInt((String) conf.getProperty("zkTimeout")); 
        
                   store = MetadataStoreExtended.create(url, 
        
                           MetadataStoreConfig.builder() 
        
                                   .metadataStoreName(MetadataStoreConfig.METADATA_STORE) 
        
                                   .sessionTimeoutMillis(zkTimeout) 
        
                                   .build()); 
        
               } catch (MetadataStoreException e) { 
        
                   throw new MetadataException(Code.METADATA_SERVICE_ERROR, e); 
        
               } 
        
           } 
        
           return store;

run autorecovery standlone :

./bin/pulsar autorecovery

###### bookkeeper client url config #####
metadataServiceUri=zk+hierarchical://10.206.128.154:2181;10.206.128.155:2181;10.206.128.156:2181;10.206.128.157:2181;10.206.128.158:2181/bk1

###### bookkeeper client rack config #####
ensemblePlacementPolicy=org.apache.bookkeeper.client.RackawareEnsemblePlacementPolicy
reppDnsResolverClass=org.apache.pulsar.bookie.rackawareness.BookieRackAffinityMapping
minNumRacksPerWriteQuorum=2
enforceMinNumRacksPerWriteQuorum=true

##### BookieRackAffinityMapping config #####
metadataStoreUrl=zk:10.206.128.154:2181,10.206.128.155:2181,10.206.128.156:2181,10.206.128.157:2181,10.206.128.158:2181/sf-pulsar-1
zkTimeout=3000

hangc0276

Nice catch!

...r-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java

codelipenghui · 2022-12-16T02:49:01Z

@hangc0276 Do we need to cherry-pick to branch-2.9? I noticed you have added the label release/2.9.5

...r-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java

...mmon/src/test/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMappingTest.java

eolivelli · 2022-12-31T10:34:54Z

...r-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java

+        BookieAddressResolver bookieAddressResolver = getBookieAddressResolver();
+        if (bookieAddressResolver instanceof DefaultBookieAddressResolver) {
+            try {
+                Field field = DefaultBookieAddressResolver.class.getDeclaredField("registrationClient");


Can you please send a PR to BookKeeper in order to make this field accessible with a getter?

That's fine.

apache/bookkeeper#3724.

Dose this pulsar pr wait for the next bk release?

We can use reflection

horizonzy · 2023-01-29T02:49:59Z

ping @eolivelli @codelipenghui @mattisonchao

hangc0276 · 2023-02-03T02:41:19Z

@hangc0276 Do we need to cherry-pick to branch-2.9? I noticed you have added the label release/2.9.5

@codelipenghui This Pr doesn't need to be cherry-picked to branch-2.9. I removed the label.

…ot aware rack info problem. (#18672)

…ot aware rack info problem. (#18672) (cherry picked from commit 43335fb)

…ot aware rack info problem. (apache#18672) (cherry picked from commit 43335fb) (cherry picked from commit 003c186)

Fix autoRecovery not aware rack info problem.

8ed4a29

github-actions bot added the doc-label-missing label Nov 29, 2022

github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Nov 29, 2022

horizonzy changed the title ~~Fix autoRecovery not aware rack info problem.~~ [fix][broker]Fix autoRecovery not aware rack info problem. Nov 29, 2022

Technoboy- added the ready-to-test label Nov 29, 2022

Technoboy- assigned horizonzy Nov 29, 2022

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

Technoboy- closed this Nov 29, 2022

Technoboy- changed the title ~~[fix][broker]Fix autoRecovery not aware rack info problem.~~ [fix][broker] Fix autoRecovery not aware rack info problem. Nov 29, 2022

Technoboy- reopened this Nov 29, 2022

hangc0276 requested review from addisonj, michaeljmarshall, aloyszhang, hangc0276, codelipenghui, Technoboy- and zymap November 30, 2022 01:24

hangc0276 added type/bug The PR fixed a bug or issue reported a bug release/2.11.1 release/2.10.3 release/2.9.5 labels Nov 30, 2022

horizonzy added 3 commits December 1, 2022 00:48

Fix autoRecovery not aware rack info problem.

e919c9b

Fix didn't aware rack info problem after restart.

50eed52

Fix checkstyle.

ba4c4a9

horizonzy changed the title ~~[fix][broker] Fix autoRecovery not aware rack info problem.~~ [fix][broker] Fix PulsarRegistrationClient and ZkRegistrationClient not aware rack info problem. Dec 2, 2022

horizonzy mentioned this pull request Dec 11, 2022

[Bug] cannot get the right rack info since the 2.10 version #18834

Closed

2 tasks

yangl approved these changes Dec 12, 2022

View reviewed changes

hangc0276 approved these changes Dec 14, 2022

View reviewed changes

mattisonchao reviewed Dec 16, 2022

View reviewed changes

...r-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java Show resolved Hide resolved

Code clean.

58ea1ac

codelipenghui reviewed Dec 16, 2022

View reviewed changes

...r-common/src/main/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMapping.java Show resolved Hide resolved

...mmon/src/test/java/org/apache/pulsar/bookie/rackawareness/BookieRackAffinityMappingTest.java Show resolved Hide resolved

Address the comments.

24cff81

eolivelli reviewed Dec 31, 2022

View reviewed changes

horizonzy mentioned this pull request Jan 3, 2023

Expose registrationClient in DefaultBookieAddressResolver. apache/bookkeeper#3724

Merged

codelipenghui approved these changes Jan 31, 2023

View reviewed changes

codelipenghui closed this Jan 31, 2023

codelipenghui reopened this Jan 31, 2023

codelipenghui merged commit 43335fb into apache:master Feb 1, 2023

liangyepianzhou added release/2.10.4 and removed release/2.10.3 labels Feb 1, 2023

This was referenced Feb 1, 2023

[fix][broker] fix the autoRecovery aware of the right rack info. #19382

Closed

[fix][broker] fix the autoRecovery aware of the right rack info. #19382 yangl/pulsar#19

Open

hangc0276 removed the release/2.9.5 label Feb 3, 2023

Technoboy- pushed a commit that referenced this pull request Feb 8, 2023

[fix][broker] Fix PulsarRegistrationClient and ZkRegistrationClient n…

d802a6a

…ot aware rack info problem. (#18672)

Technoboy- added the cherry-picked/branch-2.11 label Feb 8, 2023

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

[fix][broker] Fix PulsarRegistrationClient and ZkRegistrationClient n…

003c186

…ot aware rack info problem. (#18672) (cherry picked from commit 43335fb)

liangyepianzhou added the cherry-picked/branch-2.10 label Feb 9, 2023

horizonzy mentioned this pull request Aug 8, 2023

[Bug] Bookkeeper rackaware placement policy not applying on bookies #20918

Open

2 tasks

lhotari mentioned this pull request May 13, 2024

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

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][broker] Fix PulsarRegistrationClient and ZkRegistrationClient not aware rack info problem. #18672

[fix][broker] Fix PulsarRegistrationClient and ZkRegistrationClient not aware rack info problem. #18672

horizonzy commented Nov 29, 2022 •

edited

Loading

github-actions bot commented Nov 29, 2022

codecov-commenter commented Nov 29, 2022 •

edited

Loading

hangc0276 commented Nov 30, 2022

horizonzy commented Nov 30, 2022

yangl commented Dec 12, 2022

yangl left a comment

yangl commented Dec 13, 2022 •

edited

Loading

hangc0276 left a comment

codelipenghui commented Dec 16, 2022

eolivelli Dec 31, 2022

horizonzy Jan 3, 2023

horizonzy Jan 3, 2023 •

edited

Loading

eolivelli Jan 3, 2023

horizonzy commented Jan 29, 2023

hangc0276 commented Feb 3, 2023

[fix][broker] Fix PulsarRegistrationClient and ZkRegistrationClient not aware rack info problem. #18672

[fix][broker] Fix PulsarRegistrationClient and ZkRegistrationClient not aware rack info problem. #18672

Conversation

horizonzy commented Nov 29, 2022 • edited Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Matching PR in forked repository

github-actions bot commented Nov 29, 2022

codecov-commenter commented Nov 29, 2022 • edited Loading

Codecov Report

hangc0276 commented Nov 30, 2022

horizonzy commented Nov 30, 2022

yangl commented Dec 12, 2022

yangl left a comment

Choose a reason for hiding this comment

yangl commented Dec 13, 2022 • edited Loading

hangc0276 left a comment

Choose a reason for hiding this comment

codelipenghui commented Dec 16, 2022

eolivelli Dec 31, 2022

Choose a reason for hiding this comment

horizonzy Jan 3, 2023

Choose a reason for hiding this comment

horizonzy Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

eolivelli Jan 3, 2023

Choose a reason for hiding this comment

horizonzy commented Jan 29, 2023

hangc0276 commented Feb 3, 2023

horizonzy commented Nov 29, 2022 •

edited

Loading

codecov-commenter commented Nov 29, 2022 •

edited

Loading

yangl commented Dec 13, 2022 •

edited

Loading

horizonzy Jan 3, 2023 •

edited

Loading