+1 (binding) A couple of comments on the details:
- I think this might be the first time we're adding a new message type to the config topic. Might be worth noting that (somewhat unfortunately) we currently log unknown keys as an error. Kinda unclear whether it should be an error or warn (since it could legitimately indicate bad data in the config topic). During upgrade/downgrade this means we could log errors (given all nodes will have had to upgrade first, I think probably only in case of reverting). - I think it would be worthwhile considering a more flexible, extensible solution to the connect subprotocol. In particular, we're not even changing the protocol, but are just using the version number to indicate feature support. Another way of doing this which would still require a protocol version bump but would allow us more flexibility moving forward *without* bumping versions would be to introduce a field that does something like KIP-35, where the field is a flexible list of "features" supported by the client. We lose a bit of automatic resolution of the best protocol to use, but it avoids having to bump versions unnecessarily. In this case, we'd just include something like "internal_security" as a supported feature, the leader would determine if everyone supports it, and if so make use of it. The main reason I bring this up is that bumping versions for every small semantic change gets expensive given the defaults we want to support. We can eventually retire some of the older protocols on a major version bump, but given this new proposal we'll be sending 3x the subprotocol data and any further changes will only make that cost worse. - There is a seemingly arbitrary 1 minute timeout for picking up renewed session ids. It seems like this should probably be tied to some corresponding group timeout, e.g. maybe the session.timeout. -Ewen On Tue, Sep 24, 2019 at 11:37 AM Konstantine Karantasis < konstant...@confluent.io> wrote: > Nicely done! > > +1 (non-binding) > > Konstantine > > On Tue, Sep 24, 2019 at 9:10 AM Rajini Sivaram <rajinisiva...@gmail.com> > wrote: > > > Thanks for the KIP, Chris! > > > > +1 (binding) > > > > Regards, > > > > Rajini > > > > On Tue, Sep 24, 2019 at 3:12 PM Randall Hauch <rha...@gmail.com> wrote: > > > > > Thanks, Chris! > > > > > > +1 (binding) > > > > > > On Thu, Sep 19, 2019 at 7:43 PM Chris Egerton <chr...@confluent.io> > > wrote: > > > > > > > Hi all, > > > > > > > > I'd like to begin voting on KIP-507: > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-507%3A+Securing+Internal+Connect+REST+Endpoints > > > > > > > > Thanks to Ryanne, Magesh, Konstantine, Greg, and Randall for the > > fruitful > > > > discussion! > > > > > > > > Cheers, > > > > > > > > Chris > > > > > > > > > >
