[fix][broker] Fix PulsarService.getLookupServiceAddress returns wrong port if TLS is enabled

[Technoboy-]

Fixes #21012

Documentation

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

[alpreu]

Thanks for being so quick with providing a fix 👍
The only question I had when looking at the code is that getLookupServiceAddress() was used in the BrokerRegistry to set the broker id, which will change now - Does this mean when we apply the change that there is stale metadata that has to be cleaned up?

[Technoboy-]

Thanks for being so quick with providing a fix 👍 The only question I had when looking at the code is that getLookupServiceAddress() was used in the BrokerRegistry to set the broker id, which will change now - Does this mean when we apply the change that there is stale metadata that has to be cleaned up?

No need to clean-up

[lhotari]

#21894 makes further improvements in this area and replace "lookupServiceAddress" with "brokerId" and uses it consistenly across the code base.

@Technoboy- what was the reason to not cherry-pick this fix to branch-3.0 and branch-3.1 ? I noticed that you had reverted the change to branch-3.1 . The reason I'm asking is that I am cherry-picking #21894 to branch-3.0 and branch-3.1 and I noticed that tests fail on branch-3.0 . The reason for this is that in master branch, there are different test configuration for tests due to changes made in this PR.
I already cherry-picked #21633 to branch-3.0 and branch-3.1 since that's a bug fix too and without the changes there are some additional merge conflicts when cherry-picking #21894 .

[lhotari]

I think that we need to reverse the change made to "fix" #21012 since this change doesn't make sense to me. It will cause confusion if it is changed for no proper reason.

[Technoboy-]

what was the reason to not cherry-pick this fix to branch-3.0 and branch-3.1

I’m afraid there are break changes or the fix may miss some places, so keep it on the master. Then #21633 found a missing place. As you know, our tests don't cover all the places; this kind of fix impacts many functions.

[lhotari]

I'm cherry-picking this to branch-3.0 and branch-3.1 so that that sequent later changes will merge more cleanly when the changes are cherry-picked. The fixes that will be cherry-picked in this order: #21015, #21633, #21842, #21894, #21937 .
Another reason to cherry-pick is the changes in test configuration. Tests cherry-picked from master might fail in branch-3.1 and branch-3.0 unless this PR is cherry-picked. The reason is that this test removes the TLS setup from most unit tests. That's fine, but it changes behavior and it's better to have the same behavior in the LTS branch too.