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 the inconsistency of AdvertisedAddress #10312

Merged
merged 9 commits into from
May 10, 2021
Merged

Conversation

315157973
Copy link
Contributor

@315157973 315157973 commented Apr 21, 2021

Motivation

There is an advertisedAddress in PusarService and an advertisedAddress in conf. When advertisedListener is set, the values of the two advertisedAddress will be inconsistent.
In the current code, PusarService.getAdvertisedAddress is used in some places, and conf.getAdvertisedAddress is used in other places.

Modifications

Put this logic into the config to keep the values in all places consistent.

Verifying this change

@sijie sijie added this to the 2.8.0 milestone May 4, 2021
Copy link
Member

@wolfstudy wolfstudy left a comment

Choose a reason for hiding this comment

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

LGTM +1

@wolfstudy
Copy link
Member

@codelipenghui @jiazhai @eolivelli PTAL again. thanks.

@315157973
Copy link
Contributor Author

Now this bug will cause advertisedListeners to be set but invalid, because some places still get advertisedAddress

@wolfstudy
Copy link
Member

ping @eolivelli Can you help review this change again.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Overall lgtm

I left a comment, we should state it clear that that method is only internal.

We could also annotate it with our annotations that describe private API

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

very good

@codelipenghui codelipenghui merged commit d39e5e4 into apache:master May 10, 2021
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request May 11, 2021
### Motivation
There is an `advertisedAddress` in PusarService and an `advertisedAddress` in conf. When `advertisedListener` is set, the values of the two `advertisedAddress` will be inconsistent. 
In the current code, `PusarService.getAdvertisedAddress` is used in some places, and `conf.getAdvertisedAddress` is used in other places.

### Modifications
Put this logic into the config to keep the values in all places consistent.
codelipenghui pushed a commit that referenced this pull request Jun 26, 2021
There is an `advertisedAddress` in PusarService and an `advertisedAddress` in conf. When `advertisedListener` is set, the values of the two `advertisedAddress` will be inconsistent.
In the current code, `PusarService.getAdvertisedAddress` is used in some places, and `conf.getAdvertisedAddress` is used in other places.

Put this logic into the config to keep the values in all places consistent.

(cherry picked from commit d39e5e4)
@codelipenghui codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Jun 26, 2021
@315157973 315157973 deleted the ad branch July 19, 2021 11:12
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.

None yet

5 participants