Here is the sample code to clean this lookup/urlLookupMap access(by following Option A-1).
https://github.com/heesung-sn/pulsar/pull/60 Plz let me know if this looks good. Then, I will raise a PR, and accordingly, I will apply a similar change to other clients(cpp and golang) when adding the blue-green migration logic. Regards, Heesung On Sun, Jan 28, 2024 at 3:23 PM Heesung Sohn <hees...@apache.org> wrote: > 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 >> >>>> >> >>> >> >
