Hi Senthilnathan, Regarding Matthias's point 6, what is the reasoning for choosing the first occurrence of the configured header? I believe this corresponds to the oldest value for given key. If there are multiple values for a key, it seems more intuitive that the newest value is the one that should be used for compaction.
Thanks, Eric On Mon, Nov 4, 2019 at 11:00 AM Guozhang Wang <wangg...@gmail.com> wrote: > Hello Senthilnathan, > > Thanks for revamping on the KIP. I have only one comment about the wiki > otherwise LGTM. > > 1. We should emphasize that the newly introduced config yields to the > existing "log.cleanup.policy", i.e. if the latter's value is `delete` not > `compact`, then the previous config would be ignored. > > > Guozhang > > On Mon, Nov 4, 2019 at 9:52 AM Senthilnathan Muthusamy > <senth...@microsoft.com.invalid> wrote: > > > Hi all, > > > > I will start the vote thread shortly for this updated KIP. If there are > > any more thoughts I would love to hear them. > > > > Thanks, > > Senthil > > > > -----Original Message----- > > From: Senthilnathan Muthusamy <senth...@microsoft.com.INVALID> > > Sent: Thursday, October 31, 2019 3:51 AM > > To: dev@kafka.apache.org > > Subject: RE: [DISCUSS] KIP-280: Enhanced log compaction > > > > Hi Matthias > > > > Thanks for the response. > > > > (1) Yes > > > > (2) Yes, and the config name will be the same (i.e. > > `log.cleaner.compaction.strategy` & > > `log.cleaner.compaction.strategy.header`) at broker level and topic level > > (to override broker level default compact strategy). Please let me know > if > > we need to keep it in different naming convention. Note: Broker level > > (which will be in the server.properties) configuration is optional and > > default it to offset. Topic level configuration will be default to broker > > level config... > > > > (3) By this new way, it avoids another config parameter and also in > > feature if any new strategy like header need addition info, no additional > > config required. As this got discussed already and agreed to have > separate > > config, I will revert it. KIP updated... > > > > (4) Done > > > > (5) Updated > > > > (6) Updated to pick the first header in the list > > > > Please let me know if you have any other questions. > > > > Thanks, > > Senthil > > > > -----Original Message----- > > From: Matthias J. Sax <matth...@confluent.io> > > Sent: Thursday, October 31, 2019 12:13 AM > > To: dev@kafka.apache.org > > Subject: Re: [DISCUSS] KIP-280: Enhanced log compaction > > > > Thanks for picking up this KIP, Senthil. > > > > (1) As far as I remember, the main issue of the original proposal was a > > missing topic level configuration for the compaction strategy. With this > > being addressed, I am in favor of this KIP. > > > > (2) With regard to (1), it seems we would need a new topic level config > > `compaction.strategy`, and `log.cleaner.compaction.strategy` would be the > > default strategy (ie, broker level config) if a topic does not overwrite > it? > > > > (3) Why did you remove `log.cleaner.compaction.strategy.header` > > parameter and change the accepted values of > > `log.cleaner.compaction.strategy` to "header.<key>" instead of keeping > > "header"? The original approach seems to be cleaner, and I think this was > > discussed on the original discuss thread already. > > > > (4) Nit: For the "timestamp" compaction strategy you changed the KIP to > > > > -> `The record [create] timestamp` > > > > This is miss leading IMHO, because it depends on the broker/log > > configuration `(log.)message.timestamp.type` that can either be > > `CreateTime` or `LogAppendTime` what the actual record timestamp is. I > > would just remove "create" to keep it unspecified. > > > > (5) Nit: the section "Public Interfaces" should list the newly introduced > > configs -- configuration parameters are a public interface. > > > > (6) What do you mean by "first level header lookup"? The term "first > > level" indicates some hierarchy, but headers don't have any hierarchy -- > > it's just a list of key-value pairs? If you mean the _order_ of the > > headers, ie, pick the first header in the list that matches the key, > please > > rephrase it to make it clearer. > > > > > > > > @Tom: I agree with all you are saying, however, I still think that this > > KIP will improve the overall situation, because everything you pointed > out > > is actually true with offset based compaction, too. > > > > The KIP is not a silver bullet that solves all issue for interleaved > > writes, but I personally believe, it's a good improvement. > > > > > > > > -Matthias > > > > > > On 10/30/19 9:45 AM, Senthilnathan Muthusamy wrote: > > > Hi, > > > > > > Please let me know if anyone has any questions on this updated > KIP-280... > > > > > > Thanks, > > > > > > Senthil > > > > > > -----Original Message----- > > > From: Senthilnathan Muthusamy <senth...@microsoft.com.INVALID> > > > Sent: Monday, October 28, 2019 11:36 PM > > > To: dev@kafka.apache.org > > > Subject: RE: [DISCUSS] KIP-280: Enhanced log compaction > > > > > > Hi Tom, > > > > > > Sorry for the delayed response. > > > > > > Regarding the fall back to offset decision for both timestamp & header > > value is based on the previous author discuss > > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.apache.org%2Fthread.html%2Ff44317eb6cd34f91966654c80509d4a457dbbccdd02b86645782be67%40%253Cdev.kafka.apache.org%253E&data=02%7C01%7Csenthilm%40microsoft.com%7Cb5c596140be1436e9fb708d75df04714%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637081159484181661&sdata=%2Fap4F2CdPQe02wNDGkzjzIrxOQRTa2KraQE75dpjTzE%3D&reserved=0 > > and as per the discussion, it is really required to avoid duplicates. > > > > > > And the timestamp strategy is from the original KIP author and we are > > keeping it as is. > > > > > > Finally on the sequence order guarantee by the producer, it is not > > feasible on waiting for ack in async / multi-threads/processes scenarios > > and hence the header sequence based compact strategy with producer's > > responsibility to have a unique sequence generation for the > > topic-partition-key level. > > > > > > Hoping this clarifies all your questions. Please let us know if you > have > > any further questions. > > > > > > @Guozhang Wang / @Matthias J. Sax, I see you both had a detail > > discussion on the original KIP with previous author and it would great to > > hear your inputs as well. > > > > > > Thanks, > > > Senthil > > > > > > -----Original Message----- > > > From: Tom Bentley <tbent...@redhat.com> > > > Sent: Tuesday, October 22, 2019 2:32 AM > > > To: dev@kafka.apache.org > > > Subject: Re: [DISCUSS] KIP-280: Enhanced log compaction > > > > > > Hi Senthilnathan, > > > > > > In the motivation isn't it a little misleading to say "On the producer > > > side, we clearly preserve an order for the two messages, <K1, V1> <K1, > > > V2>"? IMHO, the semantics of the producer are clear that having an > > > V2>observed > > > order of sending records from different producers is not sufficient to > > guarantee ordering on the broker. You really need to send the 2nd record > > only after the 1st record is acked. It's the difficultly of achieving > that > > in practice that's the true motivation for your KIP. > > > > > > I can see the attraction of using timestamps, but it would be helpful > to > > explain how that really solves the problem. When the producers are in > > different processes on different machines you're relying on their clocks > > being synchronized, which is a whole subject in itself. Even if they're > > synchronized the resolution of System.currentTimeMillis() is typically > many > > milliseconds. If your producers are in different threads of the same > > process that could be a real problem because it makes ties quite likely. > > > And you don't explain why it's OK to resolve ties using the offset. The > > basis of your argument is that the offset is giving you the wrong answer. > > > So it seems to me that using it as a tiebreaker is just narrowing the > > chances of getting the wrong answer. Maybe none of this matters for your > > use case, but I think it should be spelled out in the KIP, because it > > surely would matter for similar use cases. > > > > > > Using a sequence at least removes the problem of ties, but the > > interesting bit is now in how you deal with races between > threads/processes > > in getting a sequence number allocated (which is out of scope of the > KIP, I > > guess). > > > How is resolving that race any simpler that resolving the motivating > > race by waiting for the ack of the first record sent? > > > > > > Kind regards, > > > > > > Tom > > > > > > On Mon, Oct 21, 2019 at 9:06 PM Senthilnathan Muthusamy < > > senth...@microsoft.com.invalid> wrote: > > > > > >> Hi All, > > >> > > >> We are bring back the KIP-280 to live with small correct for the > > >> discussion & voting. Thanks to previous author Luis Cabral on the > > >> KIP-280 initiation and we are taking over to complete and get it into > > 2.4... > > >> > > >> Below is the correction that we made to the existing KIP-280: > > >> > > >> * Allowing the compact strategy configuration at the topic level > as > > >> the log compaction is at the topic level and a broker can have > > >> multiple topics. This allows the flexibility to have the strategy at > > >> both broker level (i.e. for all topics within the broker) and topic > > >> level (i.e. for a subset of topics within a broker) as well... > > >> > > >> KIP-280: > > >> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwi > > >> k > > >> i.apache.org%2Fconfluence%2Fdisplay%2FKAFKA%2FKIP-280%253A%2BEnhanced > > >> % > > >> 2Blog%2Bcompaction&data=02%7C01%7Csenthilm%40microsoft.com%7C686c > > >> 3 > > >> 2fa4a554d61ae1408d756d409f6%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C > > >> 0 > > >> %7C637073341017520406&sdata=KrRem2KWCBscHX963Ah8wZ%2Fj9dkhCeAa7Gs > > >> 6 > > >> XqJ%2F5SQ%3D&reserved=0 PULL REQUEST: > > >> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > > >> h > > >> ub.com%2Fapache%2Fkafka%2Fpull%2F7528&data=02%7C01%7Csenthilm%40m > > >> i > > >> crosoft.com%7C686c32fa4a554d61ae1408d756d409f6%7C72f988bf86f141af91ab > > >> 2 > > >> d7cd011db47%7C1%7C0%7C637073341017520406&sdata=bt32PgDUjJjpXohEWp > > >> t > > >> Fxv6mPERCwcRFlVROzinBtnk%3D&reserved=0 (unit test coverage in > > >> progress) > > >> > > >> Previous Thread DISCUSS: > > >> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis > > >> t > > >> s.apache.org%2Fthread.html%2F79aa6e50d7c737ddf83455dd8063692a535a1afa > > >> 5 > > >> 58620fe1a1496d3%40%253Cdev.kafka.apache.org%253E&data=02%7C01%7Cs > > >> e > > >> nthilm%40microsoft.com%7C686c32fa4a554d61ae1408d756d409f6%7C72f988bf8 > > >> 6 > > >> f141af91ab2d7cd011db47%7C1%7C0%7C637073341017520406&sdata=XwcUWWY > > >> D > > >> PV1nA%2BbkDGLFNlXZ5bysVblWUTDQEzAaKxM%3D&reserved=0 > > >> Previous Thread VOTE: > > >> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis > > >> t > > >> s.apache.org%2Fthread.html%2Fb2ecd73ce849741f0c40b4f801c3f76505834978 > > >> 1 > > >> 2713e240e1ac2b7%40%253Cdev.kafka.apache.org%253E&data=02%7C01%7Cs > > >> e > > >> nthilm%40microsoft.com%7C686c32fa4a554d61ae1408d756d409f6%7C72f988bf8 > > >> 6 > > >> f141af91ab2d7cd011db47%7C1%7C0%7C637073341017520406&sdata=8cKQcAm > > >> 2 > > >> DDVGVLTKtciYKGMiI%2FgOADW6tam9nem4lsg%3D&reserved=0 > > >> > > >> Appreciate your timely action. > > >> > > >> PS: Initiating a separate thread as I was not able to reply to the > > >> existing threads... > > >> > > >> Thanks, > > >> Senthil > > >> > > > > > > -- > -- Guozhang >
