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 get owned service units NPE #20625

Conversation

Demogorgon314
Copy link
Member

Motivation

When using KoP protocol, the KoP will NamespaceService#addNamespaceBundleOwnershipListener before the broker start, however, at this time, the ExtensibleLoadManagerImpl not start yet. So it will cause NPE.

We should return an empty set when ExtensibleLoadManagerImpl does not start.

Modifications

  • Add a check when getOwnedServiceUnits.
  • Add units test to cover this case.

Documentation

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

@Demogorgon314 Demogorgon314 added type/bug The PR fixed a bug or issue reported a bug area/broker release/3.0.2 labels Jun 21, 2023
@Demogorgon314 Demogorgon314 self-assigned this Jun 21, 2023
@Demogorgon314 Demogorgon314 changed the title [bug][broker] Fix get owned service units NPE [fix][broker] Fix get owned service units NPE Jun 21, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 21, 2023
@Demogorgon314
Copy link
Member Author

@heesung-sn Please help review this PR

@Technoboy- Technoboy- added this to the 3.1.0 milestone Jun 21, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #20625 (dfaa8f9) into master (677d160) will increase coverage by 41.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #20625       +/-   ##
=============================================
+ Coverage     31.93%   73.04%   +41.11%     
- Complexity    11779    31998    +20219     
=============================================
  Files          1498     1867      +369     
  Lines        114595   138663    +24068     
  Branches      12428    15237     +2809     
=============================================
+ Hits          36591   101290    +64699     
+ Misses        73158    29354    -43804     
- Partials       4846     8019     +3173     
Flag Coverage Δ
inttests 24.13% <0.00%> (?)
systests 24.87% <0.00%> (?)
unittests 72.34% <100.00%> (+40.41%) ⬆️

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

Impacted Files Coverage Δ
...dbalance/extensions/ExtensibleLoadManagerImpl.java 78.62% <100.00%> (+76.50%) ⬆️

... and 1552 files with indirect coverage changes

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Looks good.

... while I wonder what the caller does after get an empty set from getOwnedServiceUnits? Retry?

@tisonkun
Copy link
Member

Merging...

@tisonkun tisonkun merged commit 7bb3531 into apache:master Jun 21, 2023
61 of 64 checks passed
@Demogorgon314
Copy link
Member Author

@tisonkun

    public void addNamespaceBundleOwnershipListener(NamespaceBundleOwnershipListener... listeners) {
        Objects.requireNonNull(listeners);
        for (NamespaceBundleOwnershipListener listener : listeners) {
            if (listener != null) {
                bundleOwnershipListeners.add(listener);
            }
        }
        getOwnedServiceUnits().forEach(bundle -> notifyNamespaceBundleOwnershipListener(bundle, listeners)); // <-- caller
    }

If the load manager has not started yet, it means we don’t have any ownership in the current broker, so it is fine to get an empty set here. After the load manager starts and has owned the bundle, it will notify all the listeners when they have ownership acquired by the current broker.

@Demogorgon314 Demogorgon314 deleted the Demogorgon314/fix-get-owned-service-units-npe branch June 21, 2023 05:42
@tisonkun
Copy link
Member

@Demogorgon314 Thanks for your explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker cherry-picked/branch-3.0 doc-not-needed Your PR changes do not impact docs ready-to-test release/3.0.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants