Thanks for the comment Matthias.
In fact, I cannot think of why we cannot close the producer no matter what.
On the other hand, it is also okay to reuse the producer when the error is
not fatal. @Guozhang Wang <wangg...@gmail.com> @Boyang Chen
<boy...@confluent.io>
Matthias J. Sax <mj...@apache.org> 于2020年5月1日周五 上午7:52写道：
> Thanks for the KIP. Make sense to me. I think you can start a vote.
>
> One minor comment about the code example: From my understanding, a
> producer should always be closed (independent if there was no error, a
> transient error, or a fatal error). If that is correct, than the code
> example seems to be miss-leading?
>
>
> -Matthias
>
> On 4/25/20 6:08 PM, 张祥 wrote:
> > Sorry, but this KIP is still open to discussion, any comments and ideas
> > would be appreciated, Thanks.
> >
> > 张祥 <xiangzhang1...@gmail.com> 于2020年4月17日周五 下午1:04写道：
> >
> >> Guozhang, thanks for the valuable suggestion.
> >>
> >> A new part called "suggested coding pattern" has been added and I copy
> the
> >> core code here:
> >>
> >> try {
> >> producer.beginTransaction();
> >> for (int i = 0; i < 100; i++)
> >> producer.send(new ProducerRecord<>("my-topic",
> >> Integer.toString(i), Integer.toString(i)));
> >> producer.commitTransaction();
> >> } catch (Exception e) {
> >> producer.abortTransaction();
> >> if(e instanceof IllegalStateException ||
> >> e instanceof ProducerFencedException ||
> >> e instanceof UnsupportedVersionException ||
> >> e instanceof AuthorizationException ||
> >> e instanceof OutOfOrderSequenceException) {
> >> producer.close();
> >> }
> >> }
> >>
> >> As you can see, in the catch block, all fatal exceptions need to be
> >> listed, I am not sure I have listed all of them and I wonder if there
> is a
> >> better way to do this.
> >>
> >>
> >> Guozhang Wang <wangg...@gmail.com> 于2020年4月17日周五 上午8:50写道：
> >>
> >>> Xiang, thanks for the written KIP. I just have one meta comment and
> >>> otherwise it looks good to me: could you also add a section about
> >>> suggested
> >>> coding patterns (especially how try - catch should be implemented) as
> we
> >>> discussed on the JIRA to the wiki page as well?
> >>>
> >>> And please also note that besides the javadoc of the function, on top
> of
> >>> the KafkaProducer class there are also comments regarding example
> snippet:
> >>>
> >>> ```
> >>>
> >>> * <pre>
> >>> * {@code
> >>> * Properties props = new Properties();
> >>> * props.put("bootstrap.servers", "localhost:9092");
> >>> * props.put("transactional.id", "my-transactional-id");
> >>> * Producer<String, String> producer = new KafkaProducer<>(props, new
> >>> StringSerializer(), new StringSerializer());
> >>> *
> >>> * producer.initTransactions();
> >>> *
> >>> * try {
> >>> * producer.beginTransaction();
> >>> * for (int i = 0; i < 100; i++)
> >>> * producer.send(new ProducerRecord<>("my-topic",
> >>> Integer.toString(i), Integer.toString(i)));
> >>> * producer.commitTransaction();
> >>> * } catch (ProducerFencedException | OutOfOrderSequenceException |
> >>> AuthorizationException e) {
> >>> * // We can't recover from these exceptions, so our only option is
> >>> to close the producer and exit.
> >>> * producer.close();
> >>> * } catch (KafkaException e) {
> >>> * // For all other exceptions, just abort the transaction and try
> >>> again.
> >>> * producer.abortTransaction();
> >>> * }
> >>> * producer.close();
> >>>
> >>> * } </pre>
> >>> ```
> >>>
> >>> I think with this change we do not need to educate users that they
> should
> >>> distinguish the types of exceptions when calling `abortTxn`, instead
> they
> >>> only need to depend on the exception to decide whether to `close` the
> >>> producer, so the above recommendation could look like:
> >>>
> >>> try {
> >>>
> >>> } catch {Exception e} {
> >>>
> >>> producer.abortTxn;
> >>>
> >>> if (e instanceof /*fatal exceptions*/) {
> >>> producer.close();
> >>> }
> >>> }
> >>>
> >>>
> >>> Guozhang
> >>>
> >>> On Thu, Apr 16, 2020 at 12:14 AM 张祥 <xiangzhang1...@gmail.com> wrote:
> >>>
> >>>> Thanks for the structure change Boyang. And I agree with you on the
> weak
> >>>> proposal part, I have adjusted it according to your suggestion. Thanks
> >>>> again!
> >>>>
> >>>> Boyang Chen <reluctanthero...@gmail.com> 于2020年4月16日周四 下午2:39写道：
> >>>>
> >>>>> Thanks for the KIP Xiang!
> >>>>>
> >>>>> I think the motivation looks good, and I just did a slight structure
> >>>> change
> >>>>> to separate "Proposed Changes" and "Public Interfaces", hope you
> don't
> >>>>> mind.
> >>>>>
> >>>>> However, "we can determine whether the producer client is already in
> >>>> error
> >>>>> state in abortTransaction" sounds a bit weak about the actual
> >>> proposal,
> >>>>> instead we could propose something as "we would remember whether a
> >>> fatal
> >>>>> exception has already been thrown to the application level, so that
> in
> >>>>> abort transaction we will not throw again, thus making the function
> >>> safe
> >>>> to
> >>>>> be called in an error state".
> >>>>>
> >>>>> Other than that, I think the KIP is in pretty good shape.
> >>>>>
> >>>>> Boyang
> >>>>>
> >>>>> On Wed, Apr 15, 2020 at 7:07 PM 张祥 <xiangzhang1...@gmail.com> wrote:
> >>>>>
> >>>>>> Hi everyone,
> >>>>>>
> >>>>>> I have opened a small KIP about safely aborting transaction during
> >>>>>> shutdown. I'd like to use this thread to discuss about it and any
> >>>>> feedback
> >>>>>> is appreciated (sorry for earlier KIP number mistake). Here is a
> >>> link
> >>>> to
> >>>>>> KIP-596 :
> >>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-596%3A+Safely+abort+Producer+transactions+during+application+shutdown
> >>>>>>
> >>>>>> Thank you!
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>>
> >>> --
> >>> -- Guozhang
> >>>
> >>
> >
>
>
