Thanks for your proposal, Heesung. Sorry for my late response. Just want to make a few points to hopefully improve the implementation. Overall, I think this feature is absolutely the right direction for Pulsar and will be a great improvement. I remember discussing this at some community meetings last year. See the last paragraph of [0].
In general, my primary question is about the protocol changes you're proposing, both the commands and the way the commands are sent. > I am worried about the backward compatibility if we add a new proto message > -- how can the old versions of clients handle this new proto message? The protocolVersion [1] object in the proto definition is how we guarantee backwards and forwards compatibility for all combinations of brokers/clients. The client and the broker each share their version during the handshake and then they only use commands known to both peers. We can introduce a new command here if we want/need. The nuance is that the server logic will branch depending on the client's protocolVersion. Next, I am wondering if there are any opportunities to decrease the latency even more than are proposed in the PIP by introducing some concurrency to decouple operations like creating new connections and initializing resources used by each topic (like a managed ledger). Because I used a mermaid diagram, I put the majority of this point in a PR comment [2]. Thanks, Michael [0] https://lists.apache.org/thread/ghlsprmqgkt8lfv634groy93c5dm1k1g [1] https://github.com/apache/pulsar/blob/15453964f6e6086b0c5e34736958cf749f16f5e1/pulsar-common/src/main/proto/PulsarApi.proto#L242-L268 [2] https://github.com/apache/pulsar/pull/20748#discussion_r1287927655 On Fri, Aug 4, 2023 at 3:41 PM Heesung Sohn <heesung.s...@streamnative.io.invalid> wrote: > > Hi, > > I updated the PIP to add more details about the recent discussion. > > Thanks, > Heesung > > On Thu, Aug 3, 2023 at 6:46 PM Heesung Sohn <heesung.s...@streamnative.io> > wrote: > > > > > > > On Wed, Aug 2, 2023 at 8:02 PM PengHui Li <peng...@apache.org> wrote: > > > >> > One of the advantages of this proposal (add these optional dstBrokerUrl > >> fields in the existing CommandCloseProducer and CommandCloseConsumer) is > >> the backward compatibility > >> > >> Ah, yes. I missed this point. It's a good convince. > >> > >> > After the client connects to the destination broker, the next > >> command(e.g. > >> > ProducerCommand) requires > >> > the destination broker to check the ownership again against its local > >> table > >> > view of the bundle state channel. > >> > >> > Upon this local ownership check, there could be the following scenarios: > >> > >> > Happy case: > >> > - If the ownership state is `owned ` and the current broker is indeed > >> the > >> > owner, the command completes. > >> > Unhappy cases: > >> > - If the ownership state is `owned ` and the current broker is not the > >> > owner, the command fails > >> > (the broker returns an error to the client), and the client tries to > >> find > >> > the true new owner by lookups. > >> > The global bundle state channel is eventually consistent, and the > >> lookups > >> > should eventually converge. > >> > - if the ownership change is still in progress(`releasing`, > >> `assigning`), > >> > this check will be deferred > >> > until the state becomes `owned` with a timeout. > >> > >> Could you please also update the explanation in the proposal? > >> It looks like what is guaranteed and what is not in the new proposal. > >> And why the mechanism can work with the unhappy case. > >> > >> Both the existing mechanism and the new mechanism follow the > >> eventual consistency. The nuance is that the new mechanism will change > >> the bundle owner first and then unload the topics. For the broker logs, > >> will more error messages related to the fenced ledger be introduced? > >> Or the client reconnects will happen after the owner cache is updated? > >> > >> > > As described in the protocol change in the PIP, the goal is to have fewer > > fenced ledger errors because the protocol change helps the client > > reconnection(open ledger) > > happen at the very end. > > > > > >> If the load manager crashed before unloading the topics (close producers > >> and consumers). > >> Will the old broker provide service for this topic until the client > >> triggers a > >> new lookup? > >> > > > > Sorry, I am not sure if I understood your question here, but > > When the destination or source broker crashes in the middle, > > the leader will find the orphan state and clean the ownership by selecting > > a new owner, and the client will reconnect to it. > > During this transfer process, if alive, the source broker will serve the > > topic according to the protocol described in the PIP. > > > > > >> Sorry, Another question about PIP-192. Is it required by the new > >> mechanism? > >> Will the situation also be improved without enabling PIP-192? > >> We have added the new owner broker to the close producer and consumer > >> command. > >> We can also update the owner and then close the producer and consumers > >> with > >> the new owner. > >> > > > > This PIP is based on PIP-192 because the proposed protocol change is based > > on the bundle states from PIP-192. > > I assume the community could raise other PIPs to utilize the close > > producer and consumer command changes for other logic too. > > > > > >> > >> Regards, > >> Penghui > >> > >> On Thu, Aug 3, 2023 at 4:38 AM Heesung Sohn > >> <heesung.s...@streamnative.io.invalid> wrote: > >> > >> > Hi PengHui, > >> > Thank you for your questions. Please find my answers inline. > >> > > >> > On Wed, Aug 2, 2023 at 3:47 AM PengHui Li <peng...@apache.org> wrote: > >> > > >> > > Hi Heesung, > >> > > > >> > > I'm sorry for not getting back to you sooner. > >> > > The motivation and the solution look good to me. > >> > > > >> > > I just want to leave some comments to make the proposal clear > >> > > for users and developers. > >> > > > >> > > > >> > > > >> > > >> ---------------------------------------------------------------------------------------------------- > >> > > > >> > > We should have a section to describe the values that will be added > >> > > by this proposal. > >> > > > >> > > - Reduced the publish latency spike and e2e latency spike during the > >> > > reconnecting > >> > > - Mitigated bottleneck(lookup) of a large number of topics in a > >> cluster > >> > > > >> > > Maybe more, I can only think about the above two values. > >> > > > >> > > > >> > Good point. I will clarify this part in the PIP. > >> > > >> > > >> > > >> ---------------------------------------------------------------------------------------------------- > >> > > > >> > > For the proto changes, the client side can also use the HTTP lookup > >> > > service. > >> > > We should also mention it in this proposal. Instead of adding four new > >> > > fields to > >> > > the CommandCloseProducer and CommandCloseConsumer, maybe adding a new > >> > > message BrokerIdentifier, or others is better. > >> > > > >> > > >> > I am worried about the backward compatibility if we add a new proto > >> message > >> > -- how can the old versions of clients handle this new proto message? > >> > > >> > One of the advantages of this proposal (add these optional dstBrokerUrl > >> > fields in the existing CommandCloseProducer and CommandCloseConsumer) is > >> > the backward compatibility -- older clients can still work in the older > >> way > >> > by going through the lookup requests. > >> > > >> > Ref: https://protobuf.dev/overview/#updating-defs > >> > Updating Proto Definitions Without Updating Code > >> > <https://protobuf.dev/overview/#updating-defs> > >> > > >> > It’s standard for software products to be backward compatible, but it is > >> > less common for them to be forward compatible. As long as you follow > >> > some simple > >> > practices <https://protobuf.dev/programming-guides/proto3/#updating> > >> when > >> > updating .proto definitions, old code will read new messages without > >> > issues, ignoring any newly added fields. ... > >> > > >> > > >> > > > >> > > > >> > > >> ---------------------------------------------------------------------------------------------------- > >> > > > >> > > There are two points we need to clarify in the proposal. > >> > > > >> > > - If the client reconnects before the new owner takes ownership. If > >> the > >> > > producer closed > >> > > after the new broker took ownership, would it be a potential > >> > consistency > >> > > issue? > >> > > >> > - If the owner changed again before the client reconnection. How did > >> Pulsar > >> > > handle this case? > >> > > As I understand, it's fine because the broker will check the > >> ownership > >> > > again when handling > >> > > producer and consumer creation. If it can be explained in the > >> > proposal, > >> > > it should be great > >> > > for developers to understand it deeply. > >> > > > >> > > >> > After the client connects to the destination broker, the next > >> command(e.g. > >> > ProducerCommand) requires > >> > the destination broker to check the ownership again against its local > >> table > >> > view of the bundle state channel. > >> > > >> > Upon this local ownership check, there could be the following scenarios: > >> > > >> > Happy case: > >> > - If the ownership state is `owned ` and the current broker is indeed > >> the > >> > owner, the command completes. > >> > Unhappy cases: > >> > - If the ownership state is `owned ` and the current broker is not the > >> > owner, the command fails > >> > (the broker returns an error to the client), and the client tries to > >> find > >> > the true new owner by lookups. > >> > The global bundle state channel is eventually consistent, and the > >> lookups > >> > should eventually converge. > >> > - if the ownership change is still in progress(`releasing`, > >> `assigning`), > >> > this check will be deferred > >> > until the state becomes `owned` with a timeout. > >> > > >> > > >> > > Regards, > >> > > Penghui > >> > > > >> > > On Wed, Aug 2, 2023 at 1:18 AM Heesung Sohn > >> > > <heesung.s...@streamnative.io.invalid> wrote: > >> > > > >> > > > Hi, > >> > > > > >> > > > It has been over three weeks since this discussion started, and I > >> > replied > >> > > > to the questions in the PIP. > >> > > > I will send out a vote email tomorrow if looking good. > >> > > > > >> > > > Thanks, > >> > > > Heesung > >> > > > > >> > > > On Tue, Jul 25, 2023 at 1:12 PM Heesung Sohn < > >> > > heesung.s...@streamnative.io > >> > > > > > >> > > > wrote: > >> > > > > >> > > > > Hi, > >> > > > > > >> > > > > I replied to the comments from Kai and Ran in the PIP. > >> > > > > > >> > > > > Please review the PIP by any chance. > >> > > > > > >> > > > > Thank you, > >> > > > > Heesung > >> > > > > > >> > > > > On Thu, Jul 6, 2023 at 4:32 PM Heesung Sohn < > >> > > > heesung.s...@streamnative.io> > >> > > > > wrote: > >> > > > > > >> > > > >> Hi dev, > >> > > > >> > >> > > > >> I proposed this PIP, https://github.com/apache/pulsar/pull/20748 > >> , > >> > to > >> > > > >> make unloaded clients directly and gracefully connect to new > >> owner > >> > > > brokers > >> > > > >> without lookups. > >> > > > >> > >> > > > >> Please take a look and let me know what you think. > >> > > > >> > >> > > > >> Regards, > >> > > > >> Heesung > >> > > > >> > >> > > > > > >> > > > > >> > > > >> > > >> > >
