> Are you sure the users are aware that with `withLoggingDisabled()`, they > might lose data during failover?
I hope so :D
Of course, we can always improve the JavaDocs.
-Matthias
On 4/12/19 2:48 PM, Bill Bejeck wrote:
> Thanks for the KIP Maarten.
>
> I also agree that keeping the `withLoggingDisabled()` and
> `withLoggingEnabled(Map)` methods is the better option.
>
> When it comes to educating the users on the downside of disabling logging,
> IMHO I think a comment in the JavaDoc should be sufficient.
>
> -Bill
>
> On Fri, Apr 12, 2019 at 3:59 PM Bruno Cadonna <br...@confluent.io> wrote:
>
>> Matthias,
>>
>> Are you sure the users are aware that with `withLoggingDisabled()`, they
>> might lose data during failover?
>>
>> OK, we maybe do not necessarily need a WARN log. However, I would at least
>> add a comment like in `StoreBuilder`,ie,
>>
>> /**
>> * Disable the changelog for store built by this {@link StoreBuilder}.
>> * This will turn off fault-tolerance for your store.
>> * By default the changelog is enabled.
>> * @return this
>> */
>> StoreBuilder<T> withLoggingDisabled();
>>
>> What do you think?
>>
>> Best,
>> Bruno
>>
>> On Thu, Apr 11, 2019 at 12:04 AM Matthias J. Sax <matth...@confluent.io>
>> wrote:
>>
>>> I think that the current proposal to add `withLoggingDisabled()` and
>>> `withLoggingEnabled(Map)` should be the best option.
>>>
>>> IMHO there is no reason to add a WARN log. We also don't have a WARN log
>>> when people disable logging on regular stores. As Bruno mentioned, this
>>> might also lead to data loss, so I don't see why we should treat
>>> suppress() different to other stores.
>>>
>>>
>>> -Matthias
>>>
>>> On 4/10/19 3:36 PM, Bruno Cadonna wrote:
>>>> Hi Marteen and John,
>>>>
>>>> I would opt for option 1 with an additional log message on INFO or WARN
>>>> level, since the log file is the place where you would look first to
>>>> understand what went wrong. I would also not adjust it when persistence
>>>> stores are available for suppress.
>>>>
>>>> I would not go for option 2 or 3, because IIUC, with
>>>> `withLoggingDisabled()` also persistent state stores do not guarantee
>> not
>>>> to loose records. Persisting state stores is basically a way to
>> optimize
>>>> recovery in certain cases. The changelog topic is the component that
>>>> guarantees no data loss. So regarding data loss, in my opinion,
>> disabling
>>>> logging on the suppression buffer is not different from disabling
>> logging
>>>> on other state stores. Please correct me if I am wrong.
>>>>
>>>> Best,
>>>> Bruno
>>>>
>>>> On Wed, Apr 10, 2019 at 12:12 PM John Roesler <j...@confluent.io>
>> wrote:
>>>>
>>>>> Thanks for the update and comments, Maarten. It would be interesting
>> to
>>>>> hear what others think as well.
>>>>> -John
>>>>>
>>>>> On Thu, Apr 4, 2019 at 2:43 PM Maarten Duijn <maartendu...@msn.com>
>>> wrote:
>>>>>
>>>>>> Thank you for the explanation regarding the internals, I have edited
>>> the
>>>>>> KIP accordingly and updated the Javadoc. About the possible data loss
>>>>> when
>>>>>> altering changelog config, I think we can improve by doing (one of)
>> the
>>>>>> following.
>>>>>>
>>>>>> 1) Add a warning in the comments that clearly states what might
>> happen
>>>>>> when change logging is disabled and adjust it when persistent stores
>>> are
>>>>>> added.
>>>>>>
>>>>>> 2) Change `withLoggingDisabled` to `minimizeLogging`. Instead of
>>>>> disabling
>>>>>> logging, a call to this method minimizes the topic size by
>> aggressively
>>>>>> removing the records emitted downstream by the suppress operator. I
>>>>> believe
>>>>>> this can be achieved by setting `delete.retention.ms=0` in the topic
>>>>>> config.
>>>>>>
>>>>>> 3) Remove `withLoggingDisabled` from the proposal.
>>>>>>
>>>>>> 4) Leave both methods as-proposed, as you indicated, this is in line
>>> with
>>>>>> the other parts of the Streams API
>>>>>>
>>>>>> A user might want to disable logging when downstream is not a Kafka
>>> topic
>>>>>> but some other service that does not benefit from
>> atleast-once-delivery
>>>>> of
>>>>>> the suppressed records in case of failover or rebalance.
>>>>>> Seeing as it might cause data loss, the methods should not be used
>>>>> lightly
>>>>>> and I think some comments are warranted. Personally, I rely purely on
>>>>> Kafka
>>>>>> to prevent data loss even when a store persisted locally, so when
>>> support
>>>>>> is added for persistent suppression, I feel the comments may stay.
>>>>>>
>>>>>> Maarten
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
signature.asc
Description: OpenPGP digital signature
