Skip to content

Fix potential npe bug of #14384 #14595

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.

Sign up for GitHub

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

Jump to bottom
Merged
codelipenghui merged 2 commits into from
Mar 9, 2022
Merged

Fix potential npe bug of #14384 #14595

codelipenghui merged 2 commits into from
Mar 9, 2022

Conversation

@gaozhangmin
Copy link
Contributor

@gaozhangmin gaozhangmin commented Mar 8, 2022 •
edited
Loading

Motivation

Fix potential npe bug

Modifications

Fix potential npe bug

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

@github-actions
Copy link

github-actions bot commented Mar 8, 2022

@gaozhangmin:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@github-actions
Copy link

github-actions bot commented Mar 8, 2022

@gaozhangmin:Thanks for providing doc info!

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Mar 8, 2022
@lhotari
Copy link
Member

lhotari commented Mar 8, 2022

@gaozhangmin Please add some tests to cover this change.

nodece
nodece approved these changes Mar 8, 2022
View reviewed changes
Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

Good catch!

@gaozhangmin
Copy link
Contributor Author

gaozhangmin commented Mar 8, 2022

@gaozhangmin Please add some tests to cover this change.

The tests were already covered by BookKeeperPackagesStorageTest.java. But need to change zookeeperServers to metadataStoreUrl @lhotari

@gaozhangmin
Copy link
Contributor Author

gaozhangmin commented Mar 8, 2022

/pulsarbot run-failure-checks

@lhotari
Copy link
Member

lhotari commented Mar 8, 2022

@gaozhangmin Please add some tests to cover this change.

The tests were already covered by BookKeeperPackagesStorageTest.java. But need to change zookeeperServers to metadataStoreUrl @lhotari

ok, that explains. At the time I checked the PR there was a fix, but no test changes and that's why I made the comment.

@lhotari lhotari requested a review from zymap March 8, 2022 10:15
@codelipenghui codelipenghui merged commit 7c1f17a into apache:master Mar 9, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone Mar 9, 2022
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
@gaozhangmin
Fix potential npe bug of apache#14384 (apache#14595)
f525775
@horizonzy horizonzy mentioned this pull request Aug 26, 2022
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lhotari lhotari lhotari approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@nodece nodece nodece approved these changes

@zymap zymap Awaiting requested review from zymap

Assignees

@gaozhangmin gaozhangmin

Labels

area/broker doc-not-needed Your PR changes do not impact docs

Projects

None yet

Milestone

2.11.0

Development

Successfully merging this pull request may close these issues.

5 participants

@gaozhangmin @lhotari @codelipenghui @nodece @gaoran10