I see. Thank you for clarifying this requirement. Then, I am wondering if we need to refactor the current `lookup/urlLookupMap` access code to avoid using `lookup` when the `redirectedClusterURI` is set at the consumers/producer layer. (it appears that we still use `lookup` in the following code at the PulsarClientImpl even when the consumers/producer might have migrated already(set redirectedClusterURI already). Maybe I missed some other logic here, and please also correct me if this behavior is fine.
Some of the `lookup` access examples: https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java#L574 https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java#L951 https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java#L989 https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java#L999 If we need to make the clients use `urlLookupMap` after producers/customers are migrated, then I guess we may need to consider the following options. Option A-1: Pass `optional redirectedClusterURI` var to the above PulsarClientImpl funcs to select the lookup or urlLookupMap inside. Option A-2: - Remove urlLookupMap from PulsarClientImpl - Instead, add serviceNameResolverMap in LookupService - Pass `optional redirectedClusterURI` var to the above PulsarClientImpl funcs - Pass `optional redirectedClusterURI` var to the above LookupService funcs Regards, Heesung On Sat, Jan 27, 2024 at 10:33 AM Rajan Dhabalia <rdhaba...@yahooinc.com.invalid> wrote: > Hi, > > Updating ServiceUrl in PulsarClient will make all producers and > consumers of all topics to use that new url which is incorrect because > redirection state can be different for each topic and even producer > and consumer of the same topic. So, we can not update main serviceUrl > but client should maintain redirection url for each producer and > consumer based on its state decided by the server. > > > Sent from my iPhone > > > On Jan 26, 2024, at 9:12 AM, Heesung Sohn <hees...@apache.org> wrote: > > > > To add more context, I am trying to add this blue-green migration logic > in > > the cpp and golang client, and before copying the current logic, I am > > wondering if we can simplify it. > > > > Again, since all of the producers and consumers from the same client and > > topic need to migrate eventually, > > I think directly updating the lookup service of that shared client to the > > new cluster seems harmless. > > I might be missing some other requirements here, so please correct me. > > If we conclude keeping this redirectedClusterURI in HandlerState here, I > > will go ahead and copy the current java client logic to other clients. > > > > Also, I updated the sample code for better synchronization and dedup. > > > https://urldefense.com/v3/__https://github.com/heesung-sn/pulsar/pull/59/__;!!Op6eflyXZCqGR5I!GOoO9eJK-kfDowteNFB-KOZpNcOrlY32vOe5Z86oxNS9wuxZjjqfwunkBYgxHjtBLEpnjga7rArkuM1fbw$ > > > > Thanks, > > Heesung > > > > > > > >> On Thu, Jan 25, 2024 at 10:51 PM Heesung Sohn <hees...@apache.org> > wrote: > >> > >> I am sorry, but the current implementation of blue-green migration is > for > >> all topics (producers and customers), not topic-specific. Could you > correct > >> me if I am wrong here? > >> So, for the current requirement(all topics migration), can't we just > >> update the original lookup service for that client because all of the > >> topics (of consumers and producers) will eventually migrate to the new > >> cluster? > >> Having said this, having redirectedClusterURI in HandlerState appears to > >> be futuristic. > >> > >> Heesung > >> > >> > >> > >> On Thu, Jan 25, 2024 at 10:36 PM Heesung Sohn <hees...@apache.org> > wrote: > >> > >>>> This might not work because same PulsarClient can be used to create > >>> multiple topics and producer/consumer and migration state can be > different > >>> per topic and even for produce/consumer (of same topic) so, changing > >>> service url will not work for blue-green migration redirection for all > >>> topics served by that PulsarClient. > >>> > >>> Good point on this topic (and producer and consumer) level migration > >>> requirement. I was under the impression that the migration happens for > all > >>> topics from the old to the new cluster. > >>> > >>> For this requirement, I think we should keep redirectedClusterURI in > >>> HandlerState. > >>> > >>> Thanks for this quick response. > >>> Heesung > >>> > >>> > >>> On Thu, Jan 25, 2024 at 10:05 PM Rajan Dhabalia <dhabalia...@gmail.com > > > >>> wrote: > >>> > >>>> This might not work because same PulsarClient can be used to create > >>>> multiple topics and producer/consumer and migration state can be > different > >>>> per topic and even for produce/consumer (of same topic) so, changing > >>>> service url will not work for blue-green migration redirection for all > >>>> topics served by that PulsarClient. > >>>> > >>>> Sent from my iPhone > >>>> > >>>>> On Jan 25, 2024, at 6:57 PM, Heesung Sohn <hees...@apache.org> > wrote: > >>>>> > >>>>> Hi dev, > >>>>> > >>>>> I would like to discuss this change with the community. > >>>>> > >>>>> Motivation: > >>>>> By this BlueGreenClusterMigration PIP-188( > >>>>> > https://urldefense.com/v3/__https://github.com/apache/pulsar/pull/17962__;!!Op6eflyXZCqGR5I!GOoO9eJK-kfDowteNFB-KOZpNcOrlY32vOe5Z86oxNS9wuxZjjqfwunkBYgxHjtBLEpnjga7rAom047YrQ$ > ), `redirectedClusterURI` > >>>> member > >>>>> var has been introduced in the pulsar client to hold the new > cluster's > >>>>> endpoint. Accordingly, pulsar client also added a state of a map of > >>>>> LookupService to lookup the migrated clusters. If > redirectedClusterURI > >>>> is > >>>>> not null, then clients look up the mapped, > >>>>> redirectedClusterURI->LookupService, when clients connect to migrated > >>>>> brokers. > >>>>> > >>>>> Proposal: > >>>>> In fact, this logic can be simplified by directly updating the > >>>>> existing lookup service's service url. Then, we don't need to track > >>>> the new > >>>>> lookup services in a separate map and remove `redirectedClusterURI`, > >>>>> either. > >>>>> > >>>>> Hers is the sample code for this proposal: > >>>>> > https://urldefense.com/v3/__https://github.com/heesung-sn/pulsar/pull/59/__;!!Op6eflyXZCqGR5I!GOoO9eJK-kfDowteNFB-KOZpNcOrlY32vOe5Z86oxNS9wuxZjjqfwunkBYgxHjtBLEpnjga7rArkuM1fbw$ > >>>>> > >>>>> Pros: > >>>>> - The removal of redirectedClusterURI and the lookup service map can > >>>>> simplify the client code > >>>>> - It works better with other logics: Bundle Transfer and > >>>>> AutoClusterFailover, as these features currently only use the > original > >>>>> lookup service. > >>>>> > >>>>> Cons: > >>>>> - To consume backlog from the blue (old) cluster, new clients need to > >>>> be > >>>>> created with the old service url(pointing to the old cluster) because > >>>> the > >>>>> existing clients' lookup service will be already updated, pointing to > >>>> the > >>>>> new cluster. > >>>>> > >>>>> However, I think this(requiring a new client with old service url, > old > >>>>> cluster) makes sense because all of the existing pub/sub will anyway > be > >>>>> reconnected to the new clusters as per the broker's close commands. > >>>> So, to > >>>>> consume the backlog from the old cluster, new client applications > need > >>>> to > >>>>> start with the old service url anyway. > >>>>> > >>>>> Example: > >>>>> > >>>> > https://urldefense.com/v3/__https://github.com/heesung-sn/pulsar/pull/59/files*diff-3c745333eaeae9e8a87e92495fbf97b7c952c034b8c18ca92156f07cd5b1f5afR308-R312__;Iw!!Op6eflyXZCqGR5I!GOoO9eJK-kfDowteNFB-KOZpNcOrlY32vOe5Z86oxNS9wuxZjjqfwunkBYgxHjtBLEpnjga7rAoXqs6_Dg$ > >>>>> > >>>>> Thanks, > >>>>> Heesung > >>>> > >>> >
