Hey Guozhang, To clarify, the broker does not actually use the ApiVersion API for inter-broker communications. The use of an API and its corresponding version is controlled by `inter.broker.protocol.version`.
Nevertheless, it sounds like we're on the same page about removing DescribeTransactionState. The impact of a dangling transaction is a little worse than what you describe though. Consumers with the read_committed isolation level will be stuck. Still, I think we agree that this case should be rare and we can reconsider for future work. Rather than preventing dangling transactions, perhaps we should consider options which allows us to detect them and recover. Anyway, this needs more thought. I will update the KIP. Best, Jason On Tue, Nov 27, 2018 at 6:51 PM Guozhang Wang <wangg...@gmail.com> wrote: > 0. My original question is about the implementation details primarily, > since current the handling logic of the APIVersionResponse is simply "use > the highest supported version of the corresponding request", but if the > returned response from APIVersionRequest says "I don't even know about the > DescribeTransactionStateRequest at all", then we need additional logic for > the falling back logic. Currently this logic is embedded in NetworkClient > which is shared by all clients, so I'd like to avoid making this logic more > complicated. > > As for the general issue that a broker does not recognize a producer with > sequence number 0, here's my thinking: as you mentioned in the wiki, this > is only a concern for transactional producer since for idempotent producer > it can just bump the epoch and go. For transactional producer, even if the > producer request from a fenced producer gets accepted, its transaction will > never be committed and hence messages not exposed to read-committed > consumers as well. The drawback is though, 1) read-uncommitted consumers > will still read those messages, 2) unnecessary storage for those fenced > produce messages, but in practice should not accumulate to a large amount > since producer should soon try to commit and be told it is fenced and then > stop, 3) there will be no markers for those transactional messages ever. > Looking at the list and thinking about the likelihood it may happen > assuming we retain the producer up to transactional.id.timeout (default is > 7 days), I feel comfortable leaving it as is. > > Guozhang > > On Mon, Nov 26, 2018 at 6:09 PM Jason Gustafson <ja...@confluent.io> > wrote: > > > Hey Guozhang, > > > > Thanks for the comments. Responses below: > > > > 0. The new API is used between brokers, so we govern its usage using > > `inter.broker.protocol.version`. If the other broker hasn't upgraded, we > > will just fallback to the old logic, which is to accept the write. This > is > > similar to how we introduced the OffsetsForLeaderEpoch API. Does that > seem > > reasonable? > > > > To tell the truth, after digging this KIP up and reading it over, I am > > doubting how crucial this API is. It is attempting to protect a write > from > > a zombie which has just reset its sequence number after that producer had > > had its state cleaned up. However, one of the other improvements in this > > KIP is to maintain producer state beyond its retention in the log. I > think > > that makes this case sufficiently unlikely that we can leave it for > future > > work. I am not 100% sure this is the only scenario where transaction > state > > and log state can diverge anyway, so it would be better to consider this > > problem more generally. What do you think? > > > > 1. Thanks, from memory, the term changed after the first iteration. I'll > > make a pass and try to clarify usage. > > 2. I was attempting to handle some edge cases since this check would be > > asynchronous. In any case, if we drop this validation as suggested above, > > then we can ignore this. > > > > -Jason > > > > > > > > On Tue, Nov 13, 2018 at 6:23 PM Guozhang Wang <wangg...@gmail.com> > wrote: > > > > > Hello Jason, thanks for the great write-up. > > > > > > 0. One question about the migration plan: "The new GetTransactionState > > API > > > and the new version of the transaction state message will not be used > > until > > > the inter-broker version supports it." I'm not so clear about the > > > implementation details here: say a broker is on the newer version and > the > > > txn-coordinator is still on older version. Today the APIVersionsRequest > > can > > > only help upgrade / downgrade the request version, but not forbidding > > > sending any. Are you suggesting we add additional logic on the broker > > side > > > to handle the case of "not sending the request"? If yes my concern is > > that > > > this will be some tech-debt code that will live long before being > > removed. > > > > > > Some additional minor comments: > > > > > > 1. "last epoch" and "instance epoch" seem to be referring to the same > > thing > > > in your wiki. > > > 2. "The broker must verify after receiving the response that the > producer > > > state is still unknown.": not sure why we have to validate? If the > > metadata > > > returned from the txn-coordinator can always be considered the > > > source-of-truth, can't we just bindly use it to update the cache? > > > > > > > > > Guozhang > > > > > > > > > > > > On Thu, Sep 6, 2018 at 9:10 PM Matthias J. Sax <matth...@confluent.io> > > > wrote: > > > > > > > I am +1 on this :) > > > > > > > > > > > > -Matthias > > > > > > > > On 9/4/18 9:55 AM, Jason Gustafson wrote: > > > > > Bump. Thanks to Magnus for noticing that I forgot to link to the > KIP: > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-360%3A+Improve+handling+of+unknown+producer > > > > > . > > > > > > > > > > -Jason > > > > > > > > > > On Tue, Aug 21, 2018 at 4:37 PM, Jason Gustafson < > ja...@confluent.io > > > > > > > wrote: > > > > > > > > > >> Hi All, > > > > >> > > > > >> I have a proposal to improve the transactional/idempotent > producer's > > > > >> handling of the UNKNOWN_PRODUCER error, which is the result of > > losing > > > > >> producer state following segment removal. The current behavior is > > both > > > > >> complex and limited. Please take a look and let me know what you > > > think. > > > > >> > > > > >> Thanks in advance to Matthias Sax for feedback on the initial > draft. > > > > >> > > > > >> -Jason > > > > >> > > > > > > > > > > > > > > > > > > > -- > > > -- Guozhang > > > > > > > > -- > -- Guozhang >
