Thanks Jason. The proposed solution sounds good to me.
Guozhang On Mon, Jan 7, 2019 at 3:52 PM Jason Gustafson <ja...@confluent.io> wrote: > Hey Guozhang, > > Thanks for sharing the article. The INVALID_PRODUCER_ID_MAPPING error > occurs following expiration of the producerId. It's possible that another > producerId has been installed in its place following expiration (if another > producer instance has become active), or the mapping is empty. We can > safely retry the InitProducerId with the logic in this KIP in order to > detect which case it is. So I'd suggest something like this: > > 1. After receiving INVALID_PRODUCER_ID_MAPPING, the producer can send > InitProducerId using the current producerId and epoch. > 2. If no mapping exists, the coordinator can generate a new producerId and > return it. If a transaction is in progress on the client, it will have to > be aborted, but the producer can continue afterwards. > 3. Otherwise if a different producerId has been assigned, then we can > return INVALID_PRODUCER_ID_MAPPING. To simplify error handling, we can > probably raise this as ProducerFencedException since that is effectively > what has happened. Ideally this is the only fatal case that users have to > handle. > > I'll give it a little more thought and update the KIP. > > Thanks, > Jason > > On Thu, Jan 3, 2019 at 1:38 PM Guozhang Wang <wangg...@gmail.com> wrote: > > > You're right about the dangling txn since it will actually block > > read-committed consumers from proceeding at all. I'd agree that since > this > > is a very rare case, we can consider fixing it not via broker-side logic > > but via tooling in a future work. > > > > I've also discovered some related error handling logic inside producer > that > > may be addressed together with this KIP (since it is mostly for internal > > implementations the wiki itself does not need to be modified): > > > > > > > https://stackoverflow.com/questions/53976117/why-did-the-kafka-stream-fail-to-produce-data-after-a-long-time/54029181#54029181 > > > > Guozhang > > > > > > > > On Thu, Nov 29, 2018 at 2:25 PM Jason Gustafson <ja...@confluent.io> > > wrote: > > > > > 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 > > > > > > > > > > > > > -- > > -- Guozhang > > > -- -- Guozhang
