Ismael, Thanks for the comments. A few responses below:
> 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. Ack. I added a note in the migration section. 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)? Thanks, I added a few notes about client compatibility to the migration section. I covered the main issues that come to mind, but let me know if you think of others. 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. Let's go with InitPidRequest for consistency. Haha, "InitPIdRequest" seems like a compromise which satisfies no one. -Jason On Wed, Feb 1, 2017 at 4:14 PM, Guozhang Wang <wangg...@gmail.com> wrote: > Ismael, thanks for your feedbacks. Replied inline. > > On Wed, Feb 1, 2017 at 8:03 AM, Ismael Juma <ism...@juma.me.uk> wrote: > > > 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. > > > > > Yes we are conducting the perf tests with the message format changes in the > first stage; then the inter-broker communication with minimal transaction > coordinator implementations in the second stage. > > > > 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. > > > > > Updated doc. > > > > 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? > > > > > We have not discussed about this before. But I think it should be doable as > long as we can include the additional conversion logic in the migration > plan. > > > > 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. > > > > > I agree with the inconsistency issue. About the name itself though, should > it be "InitPIdRequest", "PIdSnapshot" "InvalidPIdMapping" though, since we > need to capitalize "I" right? > > > > 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. > > > > > I agree theoretically it is not required, but I think in practice it is > actually better to make it more restrict: the three fields before magic > byte are offset, length, and crc. Among them, crc needs to be before magic > byte if it wants to cover the magic byte fields; length would better be > before the magic byte as well for pre-allocate memory to deser/decompress > the message set, and the only field that does not matter too much to be > after magic byte is offset, but in KIP-98 we will use it as the base offset > for message set and some validation checks can be optimized to not scan > through the whole message with this field in front of the format. > > > > 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 > > > > > > > > > -- > -- Guozhang >
