+1 on the KIP. Thanks for the hard work, Becket! On Tue, Jan 5, 2016 at 11:24 AM, Jun Rao <j...@confluent.io> wrote:
> Jiangjie, > > Thanks for the updated KIP. Overall, a +1 on the proposal. A few minor > comments on the KIP. > > KIP-32: > 50. 6.c says "The log rolling has to depend on the earliest timestamp", > which is inconsistent with KIP-33. > > 51. 8.b "If the time difference threshold is set to 0. The timestamp in the > message is equivalent to LogAppendTime." If the time difference is 0 and > CreateTime is used, all messages will likely be rejected in this proposal. > So, it's not equivalent to LogAppendTime. > > 52. Could you include the new value of magic byte in message format change? > Also, do we have a single new message format that includes both the offset > change (relative offset for inner messages) and the addition of timestamp? > > 53. Could you document the changes in ProducerRequest V2 and FetchRequest > V2 (and the responses)? > > 54. In migration phase 1, step 2, does internal ApiVersion mean > inter.broker.protocol.version? > > 55. In canary step 2.b, it says "It will only see > ProduceRequest/FetchRequest V1 from other brokers and clietns.". But in > phase 2, a broker will receive FetchRequest V2 from other brokers. > > > KIP-33: > 60. The KIP still says maintaining index at "at minute granularity" even > though the index interval is configurable now. > > 61. In this design, it's possible for a log segment to have an empty time > index. In the worse case, we may have to scan more than the active segment > to recover the latest timestamp. > > Thanks, > > Jun > > On Mon, Jan 4, 2016 at 11: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 > > 1. > > 2. (Minor) Can we rename the proposed option? It isn't really "option 4" > > anymore. > > 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? > > 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. > > 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. > > > > 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 > > > > > > -- Thanks, Neha
