Ach - Sorry. I meant Jason. I had just read a John Roesler email. On Mon, Mar 25, 2019 at 5:21 PM Adam Bellemare <adam.bellem...@gmail.com> wrote:
> Hi John > > What is the status of this KIP? > > My teammates and I are running into the "UNKNOWN_PRODUCER_ID" error on > 2.1.1 for a multitude of our internal topics, and I suspect that a proper > fix is needed. > > Adam > > On Mon, Jan 7, 2019 at 7:42 PM Guozhang Wang <wangg...@gmail.com> wrote: > >> 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 >> >
