+1, the KIP looks good to me now. Guozhang
On Tue, Jan 5, 2016 at 1:47 AM, Becket Qin <becket....@gmail.com> wrote: > Hi Adi, > > Please see inline comments. And thanks for correcting my typos :) > > > On Jan 5, 2016, at 3:37 AM, Aditya Auradkar > <aaurad...@linkedin.com.INVALID> wrote: > > > > Hey Becket/Anna - > > > > I have a few comments about the KIP. > > > > 1. (Minor) Can we rename the KIP? It's currently "Add CreateTime and > > LogAppendTime etc..". This is actually the title of the now rejected > Option > Done. Please notice the link will change as well. The previous link in the > thread no longer works. > > 1. > > 2. (Minor) Can we rename the proposed option? It isn't really "option 4" > > anymore. > Done. > > 3. I'm not clear on what exactly happens to compressed messages > > when message.timestamp.type=LogAppendTime? Does every batch get > > recompressed because the inner message gets rewritten with the server > > timestamp? Or does the message set on disk have the timestamp set to -1. > In > > that case, what do we use as timestamp for the message? > The current proposal is to keep the inner message timestamp=-1. Broker > only overwrites it if it is not -1, in that case the broker will do > recompression. > > 4. Do message.timestamp.type and max.message.time.difference.ms need to > be > > per-topic configs? It seems that this is really a client config i.e. a > > client is the source of timestamps not a topic. It could also be a > > broker-level config to keep things simple. > I feel the current proposal is more server oriented, i.e, server decides > whether accept, overwrite or reject a timestamp. This ensures the timestamp > on the broker has the same type. I am not sure about the granularity. My > gut feeling is that topic level configuration helps with multi tenant > cluster. Eventually it is the application who'd decide how they want to > define the timestamp and it might be hard to for all the applications have > same timestamp usage in the same cluster. > > 5. The "Proposed Changes" section in the KIP tries to build a time-based > > index for query but that is a separate proposal (KIP-33). Can we more > > crisply identify what exactly will change when this KIP (and 31) is > > implemented? It isn't super clear to me at this point. > Good point. They are not going to be implemented in this KIP. They are > listed because they are closely related to KIP-32. > > Other than the time index and changes on top of it, KIP-31 and KIP-32 will > implement everything else needed for message format change. > > > > Aside from that, I think the "Rejected Alternatives" section of the KIP > is > > excellent. Very good insight into what options were discussed and > rejected. > > > > Aditya > > > >> On Mon, Dec 28, 2015 at 3:57 PM, Becket Qin <becket....@gmail.com> > wrote: > >> > >> Thanks Guozhang, Gwen and Neha for the comments. Sorry for late reply > >> because I only have occasional gmail access from my phone... > >> > >> I just updated the wiki for KIP-32. > >> > >> Gwen, > >> > >> Yes, the migration plan is what you described. > >> > >> I agree with your comments on the version. > >> I changed message.format.version to use the release version. > >> I did not change the internal version, we can discuss this in a separate > >> thread. > >> > >> Thanks, > >> > >> Jiangjie (Becket) Qin > >> > >> > >> > >>> On Dec 24, 2015, at 5:38 AM, Guozhang Wang <wangg...@gmail.com> wrote: > >>> > >>> Also I agree with Gwen that such changes may worth a 0.10 release or > even > >>> 1.0, having it in 0.9.1 would be quite confusing to users. > >>> > >>> Guozhang > >>> > >>>> On Wed, Dec 23, 2015 at 1:36 PM, Guozhang Wang <wangg...@gmail.com> > >> wrote: > >>>> > >>>> Becket, > >>>> > >>>> Please let us know once you have updated the wiki page regarding the > >>>> migration plan. Thanks! > >>>> > >>>> Guozhang > >>>> > >>>>> On Wed, Dec 23, 2015 at 11:52 AM, Gwen Shapira <g...@confluent.io> > >> wrote: > >>>>> > >>>>> Thanks Becket, Anne and Neha for responding to my concern. > >>>>> > >>>>> I had an offline discussion with Anne where she helped me understand > >> the > >>>>> migration process. It isn't as bad as it looks in the KIP :) > >>>>> > >>>>> If I understand it correctly, the process (for users) will be: > >>>>> > >>>>> 1. Prepare for upgrade (set format.version = 0, ApiVersion = 0.9.0) > >>>>> 2. Rolling upgrade of brokers > >>>>> 3. Bump ApiVersion to 0.9.0-1, so fetch requests between brokers will > >> use > >>>>> the new protocol > >>>>> 4. Start upgrading clients > >>>>> 5. When "enough" clients are upgraded, bump format.version to 1 > >> (rolling). > >>>>> > >>>>> Becket, can you confirm? > >>>>> > >>>>> Assuming this is the process, I'm +1 on the change. > >>>>> > >>>>> Reminder to coders and reviewers that pull-requests with user-facing > >>>>> changes should include documentation changes as well as code changes. > >>>>> And a polite request to try to be helpful to users on when to use > >>>>> create-time and when to use log-append-time as configuration - this > is > >> not > >>>>> a trivial decision. > >>>>> > >>>>> A separate point I'm going to raise in a different thread is that we > >> need > >>>>> to streamline our versions a bit: > >>>>> 1. I'm afraid that 0.9.0-1 will be confusing to users who care about > >>>>> released versions (what if we forget to change it before the release? > >> Is > >>>>> it > >>>>> meaningful enough to someone running off trunk?), we need to come up > >> with > >>>>> something that will work for both LinkedIn and everyone else. > >>>>> 2. ApiVersion has real version numbers. message.format.version has > >>>>> sequence > >>>>> numbers. This makes us look pretty silly :) > >>>>> > >>>>> My version concerns can be addressed separately and should not hold > >> back > >>>>> this KIP. > >>>>> > >>>>> Gwen > >>>>> > >>>>> > >>>>> > >>>>> On Tue, Dec 22, 2015 at 11:01 PM, Becket Qin <becket....@gmail.com> > >>>>> wrote: > >>>>> > >>>>>> Hi Anna, > >>>>>> > >>>>>> Thanks for initiating the voting process. I did not start the voting > >>>>>> process because there were still some ongoing discussion with Jun > >> about > >>>>> the > >>>>>> timestamp regarding compressed messages. That is why the wiki page > >>>>> hasn't > >>>>>> reflected the latest conversation as Guozhang pointed out. > >>>>>> > >>>>>> Like Neha said I think we have reached general agreement on this > KIP. > >> So > >>>>>> it is probably fine to start the KIP voting. At least we draw more > >>>>>> attention to the KIP even if there are some new discussion to bring > >> up. > >>>>>> > >>>>>> Regarding the upgrade plan, given we decided to implement KIP-31 and > >>>>>> KIP-32 in the same patch to avoid change binary protocol twice, the > >>>>> upgrade > >>>>>> plan was mostly discussed on the discussion thread of KIP-31. > >>>>>> > >>>>>> Since the voting has been initiated, I will update the wiki with > >> latest > >>>>>> conversation to avoid further confusion. > >>>>>> > >>>>>> BTW, I actually have started coding work on KIP-31 and KIP-32 and > will > >>>>>> focus on the patch before I return from vacation in mid Jan because > I > >>>>> have > >>>>>> no LInkedIn VPN access in China anyway... > >>>>>> > >>>>>> Thanks, > >>>>>> > >>>>>> Jiangjie > >>>>>> > >>>>>>> On Dec 23, 2015, at 12:31 PM, Anna Povzner <a...@confluent.io> > >> wrote: > >>>>>>> > >>>>>>> Hi Gwen, > >>>>>>> > >>>>>>> I just wanted to point out that I just started the vote. Becket > wrote > >>>>> the > >>>>>>> proposal and led the discussions. > >>>>>>> > >>>>>>> What I understood from reading the discussion thread, the migration > >>>>> plan > >>>>>>> was discussed at the KIP meeting, and not much on the mailing list > >>>>>> itself. > >>>>>>> > >>>>>>> My question about the migration plan was same as Guozhang wrote: > The > >>>>> case > >>>>>>> when an upgraded broker receives an old producer request. The > >>>>> proposal is > >>>>>>> for the broker to fill in the timestamp field with the current time > >> at > >>>>>> the > >>>>>>> broker. Cons: it goes against the definition of CreateTime type of > >> the > >>>>>>> timestamp (we are "over-writing" it at the broker). Pros: It looks > >>>>> like > >>>>>>> most of the use-cases would actually want that behavior, because > >>>>>> otherwise > >>>>>>> timestamp is useless and they will need to support an old, > >>>>> pre-timestamp, > >>>>>>> behavior. E.g., if we modify log retention policy to use the > >>>>> timestamp, > >>>>>> we > >>>>>>> would need to support an old implementation (the one that does not > >> use > >>>>>>> timestamps in the message). So I actually have a preference for the > >>>>>>> proposed approach. > >>>>>>> > >>>>>>> Thanks, > >>>>>>> Anna > >>>>>>> > >>>>>>>> On Tue, Dec 22, 2015 at 8:02 PM, Neha Narkhede <n...@confluent.io > > > >>>>>> wrote: > >>>>>>>> > >>>>>>>> Hey Gwen, > >>>>>>>> > >>>>>>>> Migration plan wasn't really discussed a ton in the previous > >> threads. > >>>>>> So it > >>>>>>>> will be great to dive deep and see if there are gaps there. I had > >>>>> some > >>>>>>>> questions, but the details listed on the KIP are great. > >>>>>>>> > >>>>>>>> It is complex, though the plan outlined in the wiki assumes a zero > >>>>>> downtime > >>>>>>>> upgrade assuming that producers and consumers can't be upgraded in > >>>>>> tandem. > >>>>>>>> This is typical for companies that have a significant Kafka > >>>>> footprint, > >>>>>> like > >>>>>>>> LinkedIn. > >>>>>>>> > >>>>>>>> Thanks, > >>>>>>>> Neha > >>>>>>>> > >>>>>>>>> On Tue, Dec 22, 2015 at 7:48 PM, Gwen Shapira <g...@confluent.io > > > >>>>>> wrote: > >>>>>>>>> > >>>>>>>>> Hi Anna, > >>>>>>>>> > >>>>>>>>> Thanks for the KIP, especially for the details on all the > >>>>> alternatives > >>>>>>>> and > >>>>>>>>> how we arrived at the proposal. Its really great! > >>>>>>>>> > >>>>>>>>> Can you point me at where the migration plan was discussed? It > >> looks > >>>>>>>> overly > >>>>>>>>> complex and I have a bunch of questions, but if there was a > >>>>> discussion, > >>>>>>>> I'd > >>>>>>>>> like to read up rather than repeating it :) > >>>>>>>>> > >>>>>>>>> Gwen > >>>>>>>>> > >>>>>>>>>> On Tue, Dec 22, 2015 at 12:34 PM, Anna Povzner < > a...@confluent.io > >>> > >>>>>>>>> wrote: > >>>>>>>>> > >>>>>>>>>> Hi, > >>>>>>>>>> > >>>>>>>>>> I am opening the voting thread for KIP-32: Add CreateTime and > >>>>>>>>>> LogAppendTime to Kafka message. > >>>>>>>>>> > >>>>>>>>>> For reference, here's the KIP wiki: > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-32+-+Add+CreateTime+and+LogAppendTime+to+Kafka+message > >>>>>>>>>> > >>>>>>>>>> And the mailing list threads: > >>>>>>>>>> > >>>>>>>>>> September: > >> > http://mail-archives.apache.org/mod_mbox/kafka-dev/201509.mbox/%3CCAHrRUm6NMg%3DPh4HAJdxr%3DpmZhfFcD5OEV2yxj3fg%2BXnEBTW%2B3w%40mail.gmail.com%3E > >>>>>>>>>> > >>>>>>>>>> October: > >> > http://mail-archives.apache.org/mod_mbox/kafka-dev/201510.mbox/%3CCAHrRUm7RiBAJxwO15s1tztz%3D15oibO-QJ%2B_w8AxafTnuw3jjCw%40mail.gmail.com%3E > >>>>>>>>>> > >>>>>>>>>> December: > >> > http://mail-archives.apache.org/mod_mbox/kafka-dev/201512.mbox/%3CCAHrRUm4ugxDYzyy26MGRGKpK4hsjT4EKTuu18M3wztYq4PE%3DaQ%40mail.gmail.com%3E > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Thanks > >>>>>>>>>> Anna > >>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> -- > >>>>>>>> Thanks, > >>>>>>>> Neha > >>>> > >>>> > >>>> > >>>> -- > >>>> -- Guozhang > >>> > >>> > >>> > >>> -- > >>> -- Guozhang > >> > -- -- Guozhang
