Re: [VOTE] PIP-281: Add notifyError method on PushSource

[Baodi Shi]

Close this vote by 3 binding +1s

   - Penghui
   - Yunze
   - Jiwei


And 2 non-binding +1s

   - Zike
   - Rui


Thanks,
Baodi Shi


On Aug 2, 2023 at 10:47:32, guo jiwei  wrote:

> +1 (binding)
>
>
> Regards
> Jiwei Guo (Tboy)
>
>
> On Tue, Aug 1, 2023 at 10:24 AM Rui Fu  wrote:
>
> +1
>
>
> Best,
>
>
> Rui Fu
>
> On Jul 25, 2023 at 18:10 +0800, Baodi Shi , wrote:
>
> > Hello,
>
> >
>
> > This is the vote thread for PIP 281.
>
> >
>
> > PR: https://github.com/apache/pulsar/pull/20807
>
> > Discussion:
>
> https://lists.apache.org/thread/dnmvcb5drvwhw4bpyq6x572hcdtyydfg
>
> >
>
> > The vote will be open for at least 48 hours.
>
> >
>
> >
>
> > Thanks,
>
> > Baodi Shi
>
>
>

Re: [DISCUSS] PIP-281: Optimize Bundle Unload(Transfer) Protocol for ExtensibleLoadManager

[PengHui Li]

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.



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.



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.

Regards,
Penghui

On Wed, Aug 2, 2023 at 1:18 AM Heesung Sohn
 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  >
> 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
> >>
> >
>

[DISCUSS] PIP-290 Support WSS E2E encryption and not need to expose the private key to the WebSocket Proxy

[Yubiao Feng]

[DISCUSS] PIP-290 Provide a way to implement WSS E2E encryption and not
need to expose the private key to the WebSocket ProxyHi dev, I proposed
this PIP, https://github.com/apache/pulsar/pull/20923, to provide a way to
implement WSS E2E encryption and does not need to expose the private key to
the WebSocket Proxy. Please take a look and let me know what you think.
Thanks
Yubiao Feng

Re: [DISCUSS] PIP-281: Optimize Bundle Unload(Transfer) Protocol for ExtensibleLoadManager

[Heesung Sohn]

Hi PengHui,
Thank you for your questions. Please find my answers inline.

On Wed, Aug 2, 2023 at 3:47 AM PengHui Li  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


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  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
>  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
> > >>
> > >
> >
>

Re: [VOTE] PIP-268: Add support of topic stats/stats-internal using

[Rajan Dhabalia]

>> If so, I think it could be a good reason for introducing binary
protocol support here. For the security sensitive users like financial
application. Usually they will try to reduce the dependencies (less
dependencies, less
CVEs and the exposed service endpoints. For example, the flink connector
also have pulsar-admin dependency but some of the users want to remove it.

Yes, that could be another usecase to have such API available for users.
So, I would like to bump up this discussion again and see if we have any
other suggestions or concerns as we have multiple users who need it and
would like to move forward with this API to serve those usecases.

Thanks,
Rajan

On Mon, Jun 26, 2023 at 9:03 PM PengHui Li  wrote:

> Hi Rajan,
>
> Thanks for the explanation
>
> > This feature helps them to avoid multiple different extra
> efforts
>
> If I understand correctly. You want to say users don't want to
> add the pulsar-admin dependency or the the cluster don't want
> to expose the REST API to external (not the cluster admin or
> tenant admin)?
>
> If so, I think it could be a good reason for introducing binary
> protocol support here. For the security sensitive users like financial
> application.
>
> Usually they will try to reduce the dependencies (less dependencies, less
> CVEs)
> and the exposed service endpoints. For example, the flink connector
> also have pulsar-admin dependency but some of the users want to
> remove it.
>
> I want to say that from the perspective of improving performance,
> it may not be more convincing than the above reason.
>
> Thanks,
> Penghui
>
>
> On Mon, Jun 26, 2023 at 3:37 PM Rajan Dhabalia 
> wrote:
>
> > > I do not deny that binary protocol has performance advantages. But
> maybe
> > the bottleneck is not the protocol level for now.
> >
> > Well, sure. We had serious issue with performance in past over http but
> > this feature we would mainly like to introduce it now for the
> applications
> > to enhance api user accessibility experience where we have these multiple
> > usecases where applications with large number of topics and high fanout
> > consumers would like to fetch stats and stats-internal to retrieve
> various
> > metadata for application startup and managing application state based on
> > managed-ledge states. You can think of producer/consumer stats api which
> is
> > used by many usecases in different scenarios of monitoring or state
> > management. This feature helps them to avoid multiple different extra
> > efforts and performance considerations, which helps to give clean and
> easy
> > experience for their application.
> > I am open to hear about alternative of json string and response schema
> > definition but keeping similar response-schema as admin-api response for
> > stats/stats-internal helps to give consistent view of stats across all
> APIs
> > and transferring json format helps to skip transformation stats
> definition
> > and we don't have to make any wire protocol changes whenever we add any
> new
> > field or state in response which makes this protocol response agnostic
> and
> > do not require maintenance for any changes in stats. So, we can consider
> > any suggestion but we have multiple usecaes which need this API and we
> > should implement it in a way where it can provide a consistent definition
> > view across other APIs without maintenance efforts while making any stat
> > response changes.
> >
> > Thanks,
> > Rajan
> >
> > On Sat, Jun 24, 2023 at 5:51 PM PengHui Li  wrote:
> >
> > > > >> Main reasons are performance (you will see improvement due to http
> > > overhead compare to tcp, limited threads on http-server side and
> > definitely
> > > performance compare to netty-tcp vs http-jetty server), user
> feasibility
> > > (same reason why we have producer/consumer stats using binary protocol
> > > where user can get the stats from the same producer/consumer entity
> > without
> > > having additional client creation for the stats), scale (allowing large
> > > number of requests over binary protocol compare to http is more
> > scalable).
> > >
> > > I do not deny that binary protocol has performance advantages.
> > > But maybe the bottleneck is not the protocol level for now.
> > >
> > > IMO, we should have a clear performance expectation for getting the
> topic
> > > stats
> > > If you want to achieve 10k of requests per second.
> > > Maybe the protocol is not the bottleneck. Of course, if you want to
> reach
> > > 100k or
> > > even millions of requests on a single server per second.
> > > Exactly, the binary protocol will help. I'm curious whether it is a
> real
> > > case.
> > > Unless you have to query topic stats every second and the broker owned
> > > 100k topics.
> > >
> > > And I also saw some benchmark results for the HTTP servers.
> > >
> > >
> >
> https://cwiki.apache.org/confluence/display/HTTPCOMPONENTS/HttpCoreBenchmark
> > > It's not refreshed, but it's instructive. Try other HTTP servers should
> > > also be considered.
> >

Re: [DISCUSS] PIP-281: Optimize Bundle Unload(Transfer) Protocol for ExtensibleLoadManager

[PengHui Li]

> 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?

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, 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.

Regards,
Penghui

On Thu, Aug 3, 2023 at 4:38 AM Heesung Sohn
 wrote:

> Hi PengHui,
> Thank you for your questions. Please find my answers inline.
>
> On Wed, Aug 2, 2023 at 3:47 AM PengHui Li  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
> 
>
> 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  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 w