Hi Luke Thank you for your participation in reviewing this KIP.
#1 Updated the KIP with correct configuration names and hyperlinks. #2 Yes, the semantics change from a perspective that the difference is always in the past (or at most 1 hour into the future). Updated the compatibility section to represent the same. Will update again after #3 is resolved. #3 I'd propose to introduce 2 new configs, one is " log.message.timestamp.before.max.ms", and the other one is " log.message.timestamp.after.max.ms". This is a great idea because with your proposal, 1\ we can ensure backward compatibility (hence safety of this breaking change) in 3.x by keeping the defaults of these configurations in line with what " log.message.timestamp.difference.max.ms" provides today 2\ and when 4.x has modified logic to perform segment rotation based on append time which will always be available, we can still re-use these configurations to reject messages based on validation of create timestamps. Mehari, thoughts? -- Divij Vaidya On Tue, Jun 6, 2023 at 11:43 AM Luke Chen <show...@apache.org> wrote: > Hi Beyene, > > Thanks for the KIP. > > Questions: > 1. We don't have "*max.message.time.difference.ms > <http://max.message.time.difference.ms>*" config, I think you're referring > to "log.message.timestamp.difference.max.ms"? > 2. After this KIP, the semantics of " > log.message.timestamp.difference.max.ms" > will be changed, right? > We should also mention it in this KIP, maybe compatibility section? > And I think the description of "log.message.timestamp.difference.max.ms" > will also need to be updated. > 3. Personally I like to see the "TIME_DRIFT_TOLERANCE" to be exposed as a > config, since after this change, the root issue is still not completed > resolved. > After this KIP, the 1 hour later of timestamp can still be appended > successfully, which might still be an issue for some applications. > I'd propose to introduce 2 new configs, one is " > log.message.timestamp.before.max.ms", and the other one is " > log.message.timestamp.after.max.ms". > And then we deprecate "log.message.timestamp.difference.max.ms". WDYT? > > Thank you. > Luke > > On Tue, Jun 6, 2023 at 8:02 AM Beyene, Mehari <meh...@amazon.com.invalid> > wrote: > > > Hey Justine and Divij, > > > > Thank you for the recommendations. > > I've made the updates to the KIP and added a new section called "Future > > Work: Update Message Format to Include Both Client Timestamp and > LogAppend > > Timestamp." > > > > Please take a look when get some time and let me know if there's anything > > else you'd like me to address. > > > > Thanks, > > Mehari > > > > On 6/5/23, 10:16 AM, "Divij Vaidya" <divijvaidy...@gmail.com <mailto: > > divijvaidy...@gmail.com>> wrote: > > > > > > CAUTION: This email originated from outside of the organization. Do not > > click links or open attachments unless you can confirm the sender and > know > > the content is safe. > > > > > > > > > > > > > > Hey Justine > > > > > > Thank you for bringing this up. We had a discussion earlier in this [1] > > thread and concluded that bumping up the message version is a very > > expensive operation. Hence, we want to bundle together a bunch of > > impactful changes that we will perform on the message version and change > it > > in v4.0. We are currently capturing the ideas here [2]. The idea of > always > > having a log append time is captured in point 4 in the above wiki of > ideas. > > > > > > As you suggested, we will add a new section called "future work" and add > > the idea of two timestamps (& why not do it now) over there. Meanwhile, > > does the above explanation answer your question on why not to do it right > > now? > > > > > > [1] https://lists.apache.org/thread/rxnps10t4vrsor46cx6xdj6t03qqxosh < > > https://lists.apache.org/thread/rxnps10t4vrsor46cx6xdj6t03qqxosh> > > [2] > > > > > https://cwiki.apache.org/confluence/display/KAFKA/ideas+for+kafka+message+format+v.3 > > < > > > https://cwiki.apache.org/confluence/display/KAFKA/ideas+for+kafka+message+format+v.3 > > > > > > > > > > > > > -- > > Divij Vaidya > > > > > > > > > > > > > > On Mon, Jun 5, 2023 at 6:42 PM Justine Olshan <jols...@confluent.io.inva > > <mailto:jols...@confluent.io.inva>lid> > > wrote: > > > > > > > Hey Mehari, > > > Thanks for adding that section. I think one other thing folks have > > > considered is including two timestamps in the message format -- one for > > the > > > client side timestamp and one for the server side. Of course, this > would > > > require a bump to the message format, and that hasn't happened in a > > while. > > > Could we include some information on this approach and why we aren't > > > pursuing it? I think message format bumps are tricky, but it is worth > > > calling out that this is also an option. > > > > > > Thanks, > > > Justine > > > > > > On Fri, Jun 2, 2023 at 4:51 PM Beyene, Mehari <meh...@amazon.com.inva > > <mailto:meh...@amazon.com.inva>lid> > > > wrote: > > > > > > > Hi Justine, > > > > > > > > I added a section under Proposed Changes -> Timestamp Validation > Logic > > to > > > > capture how the INVALID_TIMESTAMP is handled in this KIP. > > > > Please let me know if there are any additional areas you would like > me > > to > > > > address. > > > > > > > > Thanks, > > > > Mehari > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
