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
>>>
>>
>
signature.asc
Description: OpenPGP digital signature
