[fix] [broker] remove bundle-data in local metadata store. #21078

thetumbled · 2023-08-28T08:47:12Z

Motivation

When deleting a namespace, we will delete znode under the path /loadbalance/bundle-data in local metadata store instead of global metadata store.

Modifications

Delete bundle data znode in local metadata store.

Verifying this change

Make sure that the change passes the CI checks.
This change is a trivial rework / code cleanup without any test coverage.

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: thetumbled#26

thetumbled · 2023-08-28T08:52:53Z

PTAL, thanks. @codelipenghui @hangc0276 @AnonHxy @poorbarcode

AnonHxy · 2023-08-29T02:32:09Z

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/NamespaceResources.java


    public static final String POLICIES_READONLY_FLAG_PATH = "/admin/flags/policies-readonly";
    private static final String NAMESPACE_BASE_PATH = "/namespace";
    private static final String BUNDLE_DATA_BASE_PATH = "/loadbalance/bundle-data";

-    public NamespaceResources(MetadataStore configurationStore, int operationTimeoutSec) {
+    public NamespaceResources(MetadataStore localStore, MetadataStore configurationStore, int operationTimeoutSec) {


How about creating a separate class naming BundleResources to handle the bundle-date creating and deleting. The reason is that:

NamespaceResources will be null if configurationMetadataStore != null is false, so there is a chance to throw NPE when call NamespaceResources.deleteBundleDataAsync(..) or NamespaceResources.deleteBundleDataTenantAsync(...):

pulsar/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/PulsarResources.java

Lines 60 to 64 in d099ac4

if (configurationMetadataStore != null) {

tenantResources = new TenantResources(configurationMetadataStore, operationTimeoutSec);

clusterResources = new ClusterResources(configurationMetadataStore, operationTimeoutSec);

namespaceResources = new NamespaceResources(configurationMetadataStore, operationTimeoutSec);

resourcegroupResources = new ResourceGroupResources(configurationMetadataStore, operationTimeoutSec);

NamespaceResources#BUNDLE_DATA_BASE_PATH and ModularLoadManagerImpl#BUNDLE_DATA_PATH are duplicated. BundleResources could unify them. WDYT

It seems that we can't allow the configurationMetadataStore to be null, or there will be serious issues.

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

Lines 752 to 762 in d099ac4

if (config.isConfigurationStoreSeparated()) {

configMetadataSynchronizer = StringUtils.isNotBlank(config.getConfigurationMetadataSyncEventTopic())

? new PulsarMetadataEventSynchronizer(this, config.getConfigurationMetadataSyncEventTopic())

: null;

configurationMetadataStore = createConfigurationMetadataStore(configMetadataSynchronizer);

shouldShutdownConfigurationMetadataStore = true;

} else {

configurationMetadataStore = localMetadataStore;

shouldShutdownConfigurationMetadataStore = false;

}

pulsarResources = newPulsarResources();

Currently, bundle-data are created/updated by LoadManager, and deleted by LoadManager and NamespaceResources. It is beneficial to introduce a BundleResources to unify the opeartions to bundle-data. But i think that it should be introduced as a code optimization instead of bug fix, which need to be discussed fully.

OK. As a bug fix, this change could fix the problem and it LGTM. Maybe we could refactor it in another PR

poorbarcode · 2023-08-29T08:51:05Z

@heesung-sn @Demogorgon314 Could you take a look? Thanks

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

poorbarcode · 2023-08-30T14:48:19Z

/pulsarbot run-failure-checks

poorbarcode · 2023-08-30T15:35:56Z

/pulsarbot run-failure-checks

codecov-commenter · 2023-08-30T15:38:59Z

Codecov Report

Merging #21078 (cc9bf78) into master (d06cda6) will increase coverage by 38.42%.
Report is 30 commits behind head on master.
The diff coverage is 65.17%.

@@              Coverage Diff              @@
##             master   #21078       +/-   ##
=============================================
+ Coverage     34.70%   73.13%   +38.42%     
- Complexity    12121    32370    +20249     
=============================================
  Files          1698     1887      +189     
  Lines        129914   139862     +9948     
  Branches      14172    15384     +1212     
=============================================
+ Hits          45089   102289    +57200     
+ Misses        78825    29475    -49350     
- Partials       6000     8098     +2098

Flag	Coverage Δ
inttests	`24.06% <7.46%> (?)`
systests	`25.15% <5.97%> (+0.04%)`	⬆️
unittests	`72.41% <65.17%> (+40.32%)`	⬆️

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

Files Changed	Coverage Δ
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java	`81.21% <0.00%> (+34.26%)`	⬆️
...ookkeeper/PulsarLedgerUnderreplicationManager.java	`51.74% <52.85%> (+51.74%)`	⬆️
...etadata/bookkeeper/PulsarLedgerManagerFactory.java	`61.53% <57.14%> (+30.98%)`	⬆️
...ookkeeper/LongHierarchicalLedgerRangeIterator.java	`81.66% <66.66%> (+81.66%)`	⬆️
...metadata/bookkeeper/PulsarRegistrationManager.java	`62.56% <70.96%> (+53.16%)`	⬆️
...kkeeper/LegacyHierarchicalLedgerRangeIterator.java	`72.15% <71.42%> (+72.15%)`	⬆️
...he/pulsar/broker/resources/NamespaceResources.java	`80.28% <75.00%> (+10.28%)`	⬆️
...ulsar/metadata/bookkeeper/PulsarLayoutManager.java	`66.66% <77.77%> (+46.66%)`	⬆️
...ker/service/persistent/PersistentSubscription.java	`75.89% <83.33%> (+26.20%)`	⬆️
...pache/pulsar/broker/resources/PulsarResources.java	`83.72% <100.00%> (ø)`
... and 3 more

... and 1459 files with indirect coverage changes

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/NamespaceResources.java

mattisonchao

Hi, Bro.

I need to confirm something related to this. The namespace-level metadata should store all the data in the configuration store.
You change deleting from the configuration store to the local store. How can we delete it in the configuration store?

thetumbled · 2023-09-04T02:18:49Z

I need to confirm something related to this. The namespace-level metadata should store all the data in the configuration store. You change deleting from the configuration store to the local store. How can we delete it in the configuration store?

There is no such bundle-data in the configuration store. Znodes for bundle-data is created by LoadManager in local store, instead of configuration store.

mattisonchao · 2023-09-04T02:22:51Z

Yep, you are right. I was misunderstood.

mattisonchao · 2023-09-04T02:25:04Z

Btw, I suggest we move this logic out of the namespace resource. Because we didn't create bundle data in NamespaceResource but deleted it there, it's weird.

AnonHxy · 2023-09-04T09:02:02Z

Btw, I suggest we move this logic out of the namespace resource. Because we didn't create bundle data in NamespaceResource but deleted it there, it's weird.

+1. As I mentioned above #21078 (comment). I have created a PR to refactor this #21119

Motivation: When deleting a namespace, we will delete znode under the path `/loadbalance/bundle-data` in `local metadata store` instead of `global metadata store`. Modifications: Delete bundle data znode in local metadata store.

shibd · 2023-10-22T02:59:01Z

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

thetumbled · 2023-10-22T03:30:29Z

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

Sure. The PR for cherry-pick is #21413.

remove bundle-data in local metadata store.

265bfe7

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

thetumbled mentioned this pull request Aug 28, 2023

remove bundle-data in local metadata store. thetumbled/pulsar#26

Merged

fix checkstyle.

76dc273

poorbarcode assigned thetumbled Aug 28, 2023

poorbarcode added type/bug The PR fixed a bug or issue reported a bug release/3.0.2 release/2.11.3 release/2.10.6 labels Aug 28, 2023

poorbarcode requested review from poorbarcode, coderzc and congbobo184 August 28, 2023 09:57

poorbarcode added this to the 3.2.0 milestone Aug 28, 2023

thetumbled added 2 commits August 28, 2023 18:15

add test code.

73e86e5

fix test code.

23e42c5

AnonHxy reviewed Aug 29, 2023

View reviewed changes

thetumbled added 2 commits August 29, 2023 10:35

add test code.

e656139

add test code.

8e4473d

poorbarcode requested review from Demogorgon314 and heesung-sn and removed request for coderzc and congbobo184 August 29, 2023 08:50

poorbarcode reviewed Aug 30, 2023

View reviewed changes

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApi2Test.java Outdated Show resolved Hide resolved

add test code.

a9472bc

thetumbled requested a review from poorbarcode August 30, 2023 08:30

fix checkstyle.

efb87b1

poorbarcode approved these changes Aug 30, 2023

View reviewed changes

AnonHxy approved these changes Aug 30, 2023

View reviewed changes

poorbarcode added the ready-to-test label Aug 30, 2023

Demogorgon314 reviewed Aug 30, 2023

View reviewed changes

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApi2Test.java Outdated Show resolved Hide resolved

fix.

cc9bf78

thetumbled requested a review from Demogorgon314 August 30, 2023 09:34

Demogorgon314 approved these changes Aug 30, 2023

View reviewed changes

heesung-sn reviewed Aug 30, 2023

View reviewed changes

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/NamespaceResources.java Show resolved Hide resolved

heesung-sn approved these changes Aug 30, 2023

View reviewed changes

poorbarcode merged commit 4a87c64 into apache:master Aug 31, 2023
45 checks passed

Technoboy- added the release/3.1.1 label Sep 4, 2023

mattisonchao reviewed Sep 4, 2023

View reviewed changes

AnonHxy mentioned this pull request Sep 4, 2023

[refactor][broker] PIP-301 Part-1: Add BundleDataResources #21119

Merged

5 tasks

Technoboy- added the cherry-picked/branch-3.1 label Sep 5, 2023

Technoboy- added the cherry-picked/branch-3.0 label Sep 5, 2023

thetumbled mentioned this pull request Oct 22, 2023

[cherry-pick][branch-2.11] [fix] [broker] remove bundle-data in local metadata store. #21413

Merged

4 tasks

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

lhotari added release/2.10.7 and removed release/2.10.6 labels Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] [broker] remove bundle-data in local metadata store. #21078

[fix] [broker] remove bundle-data in local metadata store. #21078

thetumbled commented Aug 28, 2023 •

edited

thetumbled commented Aug 28, 2023 •

edited

AnonHxy Aug 29, 2023

thetumbled Aug 29, 2023 •

edited

AnonHxy Aug 30, 2023 •

edited

poorbarcode commented Aug 29, 2023

poorbarcode commented Aug 30, 2023

poorbarcode commented Aug 30, 2023

codecov-commenter commented Aug 30, 2023

mattisonchao left a comment

thetumbled commented Sep 4, 2023

mattisonchao commented Sep 4, 2023

mattisonchao commented Sep 4, 2023

AnonHxy commented Sep 4, 2023 •

edited

shibd commented Oct 22, 2023

thetumbled commented Oct 22, 2023 •

edited

	if (configurationMetadataStore != null) {
	tenantResources = new TenantResources(configurationMetadataStore, operationTimeoutSec);
	clusterResources = new ClusterResources(configurationMetadataStore, operationTimeoutSec);
	namespaceResources = new NamespaceResources(configurationMetadataStore, operationTimeoutSec);
	resourcegroupResources = new ResourceGroupResources(configurationMetadataStore, operationTimeoutSec);

	if (config.isConfigurationStoreSeparated()) {
	configMetadataSynchronizer = StringUtils.isNotBlank(config.getConfigurationMetadataSyncEventTopic())
	? new PulsarMetadataEventSynchronizer(this, config.getConfigurationMetadataSyncEventTopic())
	: null;
	configurationMetadataStore = createConfigurationMetadataStore(configMetadataSynchronizer);
	shouldShutdownConfigurationMetadataStore = true;
	} else {
	configurationMetadataStore = localMetadataStore;
	shouldShutdownConfigurationMetadataStore = false;
	}
	pulsarResources = newPulsarResources();

[fix] [broker] remove bundle-data in local metadata store. #21078

[fix] [broker] remove bundle-data in local metadata store. #21078

Conversation

thetumbled commented Aug 28, 2023 • edited

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

thetumbled commented Aug 28, 2023 • edited

AnonHxy Aug 29, 2023

Choose a reason for hiding this comment

thetumbled Aug 29, 2023 • edited

Choose a reason for hiding this comment

AnonHxy Aug 30, 2023 • edited

Choose a reason for hiding this comment

poorbarcode commented Aug 29, 2023

poorbarcode commented Aug 30, 2023

poorbarcode commented Aug 30, 2023

codecov-commenter commented Aug 30, 2023

Codecov Report

mattisonchao left a comment

Choose a reason for hiding this comment

thetumbled commented Sep 4, 2023

mattisonchao commented Sep 4, 2023

mattisonchao commented Sep 4, 2023

AnonHxy commented Sep 4, 2023 • edited

shibd commented Oct 22, 2023

thetumbled commented Oct 22, 2023 • edited

thetumbled commented Aug 28, 2023 •

edited

thetumbled commented Aug 28, 2023 •

edited

thetumbled Aug 29, 2023 •

edited

AnonHxy Aug 30, 2023 •

edited

AnonHxy commented Sep 4, 2023 •

edited

thetumbled commented Oct 22, 2023 •

edited