+1 (binding) The changes are minimal, and the concept is easy for users to understand. I support this PIP.
Thanks, Bo Yubiao Feng <yubiao.f...@streamnative.io.invalid> 于2023年8月23日周三 10:49写道： > > Hi Mattison > > > but anyway. IMO, it's better to split the > > implementation PR into multiple that > > will help the reviewer review this > > PR more easily. > > Good suggestion. > After removing tests, the amount of code lines should be very little. > Mainly changes in the class `ProducerHandler`. > > > IMO, it's better to introduce a public AP > >I to help client support it. but it's fine > > to use it to solve the web socket > > problem now. > > Good suggestion. > We can talk about improve the client design and adding API later > > Thanks > Yubiao Feng > > > On Tue, Aug 22, 2023 at 12:46 PM <mattisonc...@gmail.com> wrote: > > > +1 (binding) > > > > This proposal looks great to me. But I've got several concerns which will > > not affect this PIP voting. > > > > 1. You mixed compression and E2E encryption support in one proposal. I am > > unsure if we should split them into two parts(compression & E2E) to help > > make the proposal not too complex. but anyway. IMO, it's better to split > > the implementation PR into multiple that will help the reviewer review this > > PR more easily. > > 2. Actually, We uses a tricky way to avoid client don't compress the > > compressed data again(We set producer compression type to NONE. but we use > > the ProducerImpl to send a message entity with compressed data). IMO, it's > > better to introduce a public API to help client support it. but it's fine > > to use it to solve the web socket problem now. > > > > > > > > Best, > > Mattison > > On 22 Aug 2023 at 11:16 +0800, PengHui Li <peng...@apache.org>, wrote: > > > +1(binding) > > > > > > - The motivation looks good to me. The proposal will provide a real e2e > > > encryption solution for the WebSocket proxy > > > - The solution looks good to me. It will not introduce break changes and > > > will use public APIs as much as possible. And it will not introduce any > > > extra configuration. The API definition is clear and aligns with the > > > existing naming pattern. > > > - For the public API changes. We already have an encryptionKey field, but > > > it is key names, which not aligned with the existing definition of the > > > encryptionKey in the binary protocol. Instead of introducing a new one > > like > > > encryptionKeyValue, the proposal will use the existing one(encryptionKey) > > > and check the format on the server side. It's not so good, but better > > than > > > adding a new one to confuse users. > > > - The proposal quality looks good to me. It provides enough context about > > > what is the existing solution and what is the new solution. And provides > > a > > > comprehensive example to show what the new way looks like. > > > > > > Regards, > > > Penghui > > > > > > > > > > > > On Mon, Aug 21, 2023 at 5:30 PM Yubiao Feng > > > <yubiao.f...@streamnative.io.invalid> wrote: > > > > > > > Sorry, the PR link in the last email is ambiguous, > > > > https://github.com/apache/pulsar/pull/20923 is the correct one. > > > > > > > > Thanks > > > > Yubiao Feng > > > > > > > > On Mon, Aug 21, 2023 at 4:07 PM Yubiao Feng < > > yubiao.f...@streamnative.io> > > > > wrote: > > > > > > > > > > Hello, Guys > > > > > > > > > > > > Since there are no concerns in the discussion mail, I'd like to > > start > > > > > > voting for this PIP. > > > > > > > > > > > > The PIP link: https://github.com/apache/pulsar/pull/ > > > > > > <https://github.com/apache/pulsar/pull/21033>20923 > > > > > > <https://github.com/apache/pulsar/pull/20923> > > > > > > > > > > > > Thanks > > > > > > Yubiao Feng > > > > > > > > > > > >
