Hi Guozhang,
You are right that it is not straightforward to add a dependent property
validation.
Though it is possible to re-design it to allow for this, that effort would be
better placed under its own KIP, if it really becomes useful for other
properties as well.
Given this, the fallback-to-offset behaviour currently documented will be used.
Also, while analyzing this, I noticed that the existing compaction properties
only exist globally, and not per topic.
I don't understand why this is, but it again feels like something out of scope
for this KIP.
Given this, the KIP was updated to only include the global configuration
properties, removing the per-topic configs.
I'll soon update the PR according to the documentation, but I trust the KIP
doesn't need that to close, right?
Cheers,
Luis
On Monday, July 2, 2018, 2:00:08 PM GMT+2, Luís Cabral
<[email protected]> wrote:
Hi Guozhang,
At the moment the KIP has your vote, Matthias' and Ted's.
Should I ask someone else to have a look?
Cheers,
Luis
On Monday, July 2, 2018, 12:16:48 PM GMT+2, Mickael Maison
<[email protected]> wrote:
+1 (non binding). Thanks for the KIP!
On Sat, Jun 30, 2018 at 12:26 AM, Guozhang Wang <[email protected]> wrote:
> Hi Luis,
>
> Regarding the minor suggest, I agree it would be better to make it as
> mandatory, but it might be a bit tricky because it is a conditional
> mandatory one depending on the other config's value. Would like to see your
> updated PR.
>
> Regarding the KIP itself, both Matthias and myself can recast our votes to
> the updated wiki, while we still need one more committer to vote according
> to the bylaws.
>
>
> Guozhang
>
> On Thu, Jun 28, 2018 at 5:38 AM, Luís Cabral <[email protected]>
> wrote:
>
>> Hi,
>>
>> Thank you all for having a look!
>>
>> The KIP is now updated with the result of these late discussions, though I
>> did take some liberty with this part:
>>
>>
>> - If the "compaction.strategy.header" configuration is not set (or is
>> blank), then the compaction strategy will fallback to "offset";
>>
>>
>> Alternatively, we can also set it to be a mandatory property when the
>> strategy is "header" and fail the application to start via a config
>> validation (I would honestly prefer this, but its up to your taste).
>>
>> Anyway, this is now a minute detail that can be adapted during the final
>> stage of this KIP, so are you all alright with me changing the status to
>> [ACCEPTED]?
>>
>> Cheers,
>> Luis
>>
>>
>> On Thursday, June 28, 2018, 2:08:11 PM GMT+2, Ted Yu <
>> [email protected]> wrote:
>>
>> +1
>>
>> On Thu, Jun 28, 2018 at 4:56 AM, Luís Cabral <[email protected]
>> >
>> wrote:
>>
>> > Hi Ted,
>> > Can I also get your input on this?
>> >
>> > bq. +1 from my side for using `compaction.strategy` with values
>> > "offset","timestamp" and "header" and `compaction.strategy.header`
>> > -Matthias
>> >
>> > bq. +1 from me as well.
>> > -Guozhang
>> >
>> >
>> > Cheers,
>> > Luis
>> >
>> >
>> >
>>
>
>
>
> --
> -- Guozhang
