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 NPE when splitting and unloading bundle #2348

Merged
merged 1 commit into from Aug 10, 2018

Conversation

@massakam
Copy link
Contributor

massakam commented Aug 9, 2018

Motivation

In v1.22.1 or later, NPE often occurs when a bundle is splitted and unloaded.
https://gist.github.com/massakam/39b10ab273771924e8ccc458fa83b7e0

NPE occurs in the following line. It means that the bundle is not included in ownershipCache.
https://github.com/apache/incubator-pulsar/blob/e548aa28505c3e0d40944751c85fbc9360cfcb7d/pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java#L506

I think that what we should pass to updateFuture is not the left element of splittedBundles but the right one.
https://github.com/apache/incubator-pulsar/blob/e548aa28505c3e0d40944751c85fbc9360cfcb7d/pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java#L625

The left element seems to be all bundles under the namespace. Some of them are not owned by the broker and not included in ownershipCache.

cf. #1428

Modifications

Fixed NamespaceService to pass the left element of splittedBundles (list of bundles owened by the broker) to updateFuture.

Result

NPE will no longer occur and the splitted bundle will be unloaded properly.

@massakam massakam added this to the 2.1.1-incubating milestone Aug 9, 2018
@massakam massakam self-assigned this Aug 9, 2018
@massakam

This comment has been minimized.

Copy link
Contributor Author

massakam commented Aug 10, 2018

rerun integration tests

Copy link
Contributor

merlimat left a comment

👍

@merlimat

This comment has been minimized.

Copy link
Contributor

merlimat commented Aug 10, 2018

@massakam the integration tests are having issues right now (unrelated from this pr). In any case they won't prevent from merging this.

@merlimat merlimat merged commit a850934 into apache:master Aug 10, 2018
2 of 3 checks passed
2 of 3 checks passed
Jenkins: Integration Tests Build started for merge commit.
Details
Jenkins: C++ / Python Tests SUCCESS
Details
Jenkins: Java 8 - Unit Tests SUCCESS
Details
@massakam massakam deleted the massakam:bundle-split-npe branch Aug 10, 2018
sijie added a commit that referenced this pull request Aug 27, 2018
@sijie

This comment has been minimized.

Copy link
Contributor

sijie commented Aug 27, 2018

merged 4db7f38 in branch-2.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.