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
