Skip to content

[fix][broker] Fix brokerId npe problem - #22976

Closed
TakaHiR07 wants to merge 1 commit into
apache:masterfrom
TakaHiR07:fix_brokerId_npe
Closed

[fix][broker] Fix brokerId npe problem#22976
TakaHiR07 wants to merge 1 commit into
apache:masterfrom
TakaHiR07:fix_brokerId_npe

Conversation

@TakaHiR07

Copy link
Copy Markdown
Contributor

Motivation

As shown in #22975.

Modifications

generate brokerId when construct PulsarService, instead of in PulsarService.start().

Verifying this change

  • Make sure that the change passes the CI checks.

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 25, 2024

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think that this works for the case when a dynamic port (0) is used on the broker. Dynamic ports are used for tests.

A proper solution would be to postpone any calls until the broker id is available.

@lhotari

lhotari commented Jun 25, 2024

Copy link
Copy Markdown
Member

I'll propose another PR which also makes an improvement to ensure that the broker doesn't start serving requests before it has been fully started. That will also prevent other edge cases.

@lhotari

lhotari commented Jun 25, 2024

Copy link
Copy Markdown
Member

@TakaHiR07 I have created #22977 which addresses the brokerId NPE problem.

@TakaHiR07

Copy link
Copy Markdown
Contributor Author

I'll propose another PR which also makes an improvement to ensure that the broker doesn't start serving requests before it has been fully started. That will also prevent other edge cases.

Ok

@TakaHiR07 TakaHiR07 closed this Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants