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