Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][broker] Fix ModularLoadManagerImpl always delete active bundle-data. sec ver. #20620

Merged

Conversation

lifepuzzlefun
Copy link
Contributor

@lifepuzzlefun lifepuzzlefun commented Jun 20, 2023

Motivation

fix #20544

this problem will cause LoadManager's loadSheding and BundleSplitTask not functioning. because of the lost of real cluster-wide bundle-data in LoadManager.

master code logic in ModularLoadManagerImpl will try to clean up bundle-data if the namespace bundle is splitted.

but the current logic will cause amost all bundle-datas in the cluster to be deleted, each time the updateBundleData method triggered.

Modifications

move bundle-data cleanup logic after iterator all broker to get cluster wide activeBundles in updateBundleData method.

Verifying this change

add unittest to check if the broker number is more than one.

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 20, 2023
@lifepuzzlefun
Copy link
Contributor Author

@heesung-sn I think this version is more easily to understand. please take a look

@@ -603,6 +592,16 @@ private void updateBundleData() {
LoadManagerShared.fillNamespaceToBundlesMap(preallocatedBundleData.keySet(), namespaceToBundleRange);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why this reordering can resolve the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.
If we need to figure out which bundle is not active, we should get cluster wide all bundle data, right ? To get all cluster wide bundle data, we need iterate all brokerData to collect this.
Current we only iterate the first brokerData and check active bundles. so the all the bundleData which is not owned by the first broker will be cleaned at the first broker check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, maybe I missed some logic here.

But, In this func, I don't see bundleData and activeBundles are updated between these lines. https://github.com/apache/pulsar/pull/20620/files#diff-642d3e26ddad51db8fb736b0f5519f4c929814a4f69d85e16829d19b885be0ebR562-R594 .
So, I am curious to know how this reordering can be effective.

To get all cluster wide bundle data, we need iterate all brokerData to collect this.
Current we only iterate the first brokerData and check active bundles. so the all the bundleData which is not owned by the first broker will be cleaned at the first broker check.

Can you provide the code lines which iterate all brokerData and fill activeBundles and bundleData, other than here?
https://github.com/apache/pulsar/pull/20620/files#diff-642d3e26ddad51db8fb736b0f5519f4c929814a4f69d85e16829d19b885be0ebR549-R558

@lifepuzzlefun
Copy link
Contributor Author

lifepuzzlefun commented Jun 21, 2023

before: 5sW4ZOIXa9
after:
Gth0t6gQvh

@heesung-sn if you fold the code lines you can see the logic clearly.

@heesung-sn
Copy link
Contributor

Oh. I mis-read the change. I got it now. This removal has been moved out of the loop, for (Map.Entry<String, BrokerData> brokerEntry : loadData.getBrokerData().entrySet())

            //Remove not active bundle from loadData
            for (String bundle : bundleData.keySet()) {
                if (!activeBundles.contains(bundle)){
                    bundleData.remove(bundle);
                    if (pulsar.getLeaderElectionService().isLeader()){
                        deleteBundleDataFromMetadataStore(bundle);
                    }
                }
            }

LGTM.

@lifepuzzlefun
Copy link
Contributor Author

@Demogorgon314 could you help review this pr?

Copy link
Member

@Demogorgon314 Demogorgon314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@aloyszhang
Copy link
Contributor

Nice cath, LGTM.

@aloyszhang
Copy link
Contributor

@lifepuzzlefun plz resolve the conflict

@lifepuzzlefun
Copy link
Contributor Author

@lifepuzzlefun plz resolve the conflict

Thanks for review. Conflict has been resolved. : -)

@aloyszhang aloyszhang merged commit f2f0bf4 into apache:master Aug 21, 2023
45 checks passed
@Technoboy- Technoboy- changed the title [fix][broker] fix ModularLoadManagerImpl always delete active bundle-data. sec ver. [fix][broker] Fix ModularLoadManagerImpl always delete active bundle-data. sec ver. Aug 21, 2023
Technoboy- pushed a commit that referenced this pull request Aug 21, 2023
Technoboy- pushed a commit that referenced this pull request Aug 21, 2023
shibd pushed a commit that referenced this pull request Oct 21, 2023
…data. sec ver. (#20620)

Co-authored-by: wangjinlong <[email protected]>
(cherry picked from commit f2f0bf4)
nodece pushed a commit to nodece/pulsar that referenced this pull request Jul 23, 2024
…data. sec ver. (apache#20620)

Co-authored-by: wangjinlong <[email protected]>
(cherry picked from commit f2f0bf4)
nodece pushed a commit to ascentstream/pulsar that referenced this pull request Jul 24, 2024
…data. sec ver. (apache#20620)

Co-authored-by: wangjinlong <[email protected]>
(cherry picked from commit f2f0bf4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] LoadManager always delete active bundle-data.
8 participants