Hi all, A few comments follow:
1. The document states "inter-broker communications will be increased by M * N * P round trips per sec. We need to conduct some system performance test to make sure this additional inter-broker traffic would not largely impact the broker cluster". Has this testing been done? And if not, are we planning to do it soon? It seems important to validate this sooner rather than later. This applies more generally too, it would be great to understand how the new message format affects the producer with small messages, for example. 2. `ProducerAppId` is a new authorization resource type. This introduces a compatibility issue with regards to existing third-party authorizers. It would be good to highlight this in the migration/compatibility section. 3. I was happy to see that default values for the new configs have been added to the document since I last checked it. It would be good to explain the motivation for the choices. 4. The Migration plan is relatively brief at the moment. Have we considered if there's any additional work required due to KIP-97 (introduced in 0.10.2.0)? 5. transactional.id sounds good 6. Since we are keeping per message CRCs for auditing apps, have we considered mitigating the performance cost by using the more performant CRC32c in the new message format version? Nits: 7. It seems like there is a bit of inconsistency when it comes to naming convention. For example, we have `InitPIDRequest`, `PidSnapshot` and `InvalidPidMapping`. The latter two match Kafka's naming conventions. There are a few other examples like that and it would be good to clean them up. 8. The document states "The first four fields of a message set in this format must to be the same as the existing format because any fields before the magic byte cannot be changed in order to provide a path for upgrades following a similar approach as was used in KIP-32". This makes things easier, but it seems to me that the only strict requirement is that the magic byte remains in the same offset and with the same size. On Mon, Jan 30, 2017 at 10:37 PM, Guozhang Wang <wangg...@gmail.com> wrote: > Hello Folks, > > We have addressed all the comments collected so far, and would like to > propose a voting thread this Wednesday. If you have any further comments on > this KIP, please feel free to continue sending them on this thread before > that. > > > Guozhang > > > On Mon, Jan 30, 2017 at 1:10 PM, Jason Gustafson <ja...@confluent.io> > wrote: > > > +1 for transactional.id. > > > > -Jason > > > > On Mon, Jan 30, 2017 at 1:05 PM, Guozhang Wang <wangg...@gmail.com> > wrote: > > > > > If I have to choose between app.id and transactional.instance.id, I'd > > > choose the latter. > > > > > > Renaming transactional.instance.id to transactional.id sounds even > > better. > > > > > > > > > Guozhang > > > > > > > > > On Mon, Jan 30, 2017 at 11:49 AM, Apurva Mehta <apu...@confluent.io> > > > wrote: > > > > > > > > Bumping one suggestion from Apurva above. The name "AppID" has > caused > > > > some > > > > > confusion. We're considering the following renaming: > > > > > > > > > > 1. AppID -> ProducerId (transaction.app.id -> producer.id) > > > > > 2. PID -> IPID (internal producer ID) > > > > > > > > > > > > > > > > > How about AppId -> TransactionalId (transaction.app.id -> > > > transactional.id > > > > ) > > > > > > > > This makes it clear that this id just needs to be set when the > > > application > > > > wishes to use transactions. I also think it is more intuitive in the > > > > context of how this id is used, viz. to maintain transactions across > > > > producer sessions. > > > > > > > > Thanks, > > > > Apurva > > > > > > > > > > > > > > > > -- > > > -- Guozhang > > > > > > > > > -- > -- Guozhang >
