Hello Lari, On Tue, Feb 13, 2024 at 12:04 PM Lari Hotari <lhot...@apache.org> wrote:
> Thanks for linking to the PIP-95 implementation PR #12056 [1]. I wasn't > aware that it has been implemented. > > That PR is what I thought that was missing. It is the implementation to > achieve what PIP-61 described as "only return the corresponding service > URL" and the PIP-95 design of "use a unique bind address for each > listener". It is possible that there are gaps, but I think that we should > cover the possible gaps. > > > Maybe that's not entirely true. You can configure 100s of listeners for > all > > schemes/protocols, but the code only returns the internal or requested or > > first address for all 4 schemes (pulsar, pulsar+ssl, http, https, and one > > service url, which i am not sure why it is needed, maybe for backward > > compatibility). So while, it's not exactly approach 2, it's also not > purely > > I think that this is expected and what PIP-61 and PIP-95 describe. > The main problem of PIP-61 and PIP-95 is that there isn't documentation of > how to properly configure the solution. The important piece of > "bindAddresses" implemented in PIP-95 was missing from PIP-61 in the first > place and it's likely that PIP-95 isn't fully completed. > > The PR 12056 description is perhaps the best documentation of the feature > and provides an example of how to use the configuration: > ************ > bindAddresses=external:pulsar://0.0.0.0:6652,external:pulsar+ssl:// > 0.0.0.0:6653 > bindAddress=0.0.0.0 > brokerServicePort=6650 > brokerServicePortTls=6651 > > advertisedListeners=cluster:pulsar://broker-1.local:6650,cluster:pulsar+ssl://broker-1.local:6651,external:pulsar:// > broker-1.example.dev:6652,external:pulsar+ssl://broker-1.example.dev:6653 > internalListenerName=cluster > ************ > > Personally, while this may be a much cleaner approach or may be not solve the core issue at all, it is not what we are trying to achieve with our PIP, which is basically only a PIP due to configuration changes involved, but actually is a bug fix for a *bug we are facing in production right now. *The same bug is also highlighted by you via the comment you have linked in your [3] link There are much more things to consider for the multiple bind address approach and it deserves its own PIP. Specifically the comment on GH that I've made showcasing a use case - https://github.com/apache/pulsar/pull/22039#discussion_r1486400014 > As it can be seen, the "external" listener is bound to different ports > than the default "cluster" listener (listener for the brokerServicePort and > brokerServicePortlTls is specified with internalListenerName). > > The clear gaps in the PIP-61 and PIP-95 solution are in the admin API. > There are multiple APIs where a redirect is sent back to the client. The PR > 12072 [2] covers the topic lookup over the admin API, but doesn't cover all > other cases where the redirect happens. This is already pointed out in a > comment [3] on that PR. > > Let's treat this PIP as a bug fix only. If needed, we can skip the PIP and directly send a bug fix PR if that clears things here. > I think that PIP-338 should focus on covering the gaps and providing a > design that is aligned with the current PIP-61 & PIP-95 implementation. > > I disagree here. We have clearly identified that there is a bug in the current code. We are trying to do a bug fix here. The goal is not to deep dive into a much bigger design problem as we want to hotfix this ASAP in our system, but also want an alignment with the community so as to not maintain this patch locally, internally, for every version we upgrade to. > -Lari > > 1 - https://github.com/apache/pulsar/pull/12056 > 2 - https://github.com/apache/pulsar/pull/12072 > 3 - https://github.com/apache/pulsar/pull/12072#issuecomment-921663472 > > -- Girish Sharma
