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
> >>>
> >>
> >
>
>
