+1 On Tue, Jan 5, 2016 at 11:12 AM, Guozhang Wang <wangg...@gmail.com> wrote:
> +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 >
