Thanks all, for your comments!
I'm on board with the consensus, so it sounds like your KIP is fine as-is,
Maarten. Would you like to start the vote thread?
(just look at the mailing list for other examples)
Thanks,
-John
On Sat, Apr 13, 2019 at 5:26 PM Matthias J. Sax <matth...@confluent.io>
wrote:
> > 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
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>>
> >>
> >
>
>
