Minor correction: the OutOfOrderSequenceException is not fatal for the idempotent producer and it is not necessarily tied to the acks setting (though it is more likely to be thrown with acks=1). It is used to signal the user that there was a gap in the delivery of messages. You can hit this if there is a pause on the producer and the topic retention kicks in and deletes the last records the producer had written. However, it is possible for the user to catch it and simply keep producing (internally the producer will generate a new ProducerId). Nevertheless, pre-idempotent-producer code won't be expecting this exception, and that may cause it to break in cases where it previously wouldn't. This is probably the biggest risk of the change.
-Jason On Wed, Aug 9, 2017 at 6:33 AM, Ismael Juma <ism...@juma.me.uk> wrote: > Thanks for the KIP, Apurva. In general, I think it's a good idea to > strengthen the guarantees we provide by default. And people who are willing > to trade correctness for performance can then change the configs to suit > them. I will comment on the KIP specifics in more detail later, but one > additional comment inline: > > On Wed, Aug 9, 2017 at 7:11 AM, Ewen Cheslack-Postava <e...@confluent.io> > wrote: > > > 3. The acks=all change is actually unrelated to the title of the KIP and > > orthogonal to all the other changes. It's also the most risky since > > acks=all needs more network round trips. And while I think it makes sense > > to have the more durable default, this seems like it's actually fairly > > likely to break things for some people (even if a minority of people). > This > > one seems like a setting change that needs more sensitive handling, e.g. > > both release notes and log notification that the default is going to > > change, followed by the actual change later. > > > > The issue is that with acks=1 and idempotence, OutOfOrderSequenceException > may be thrown, which is a fatal error for the Producer (it needs to be > closed and restarted). I'll leave it to Apurva to explain this in more > detail. > > I wanted to comment on the "log notification" suggestion. Why do you think > this is needed since users can just change the config back to `acks=1` (or > 0)? We haven't done this in the past when changing defaults, so it would be > good to understand it. Given that the next release is 1.0.0, I think it > would be OK to just change it and advertise it well. Logging warnings for > deprecated configs makes sense because: > > 1. The config will go away and there may not be an exact replacement, so > good to give some time for users to transition > 2. Users should not be using the config, so it's OK to spam their logs > > Neither of those is true when we change defaults. Having said that, Git > does what you are suggesting and I agree that the impact can be negative if > people don't read the upgrade notes. Not sure what's the best way to solve > that. > > Ismael >
