kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Guozhang Wang <wangg...@gmail.com>
Subject Re: [DISCUSS] KIP-360: Improve handling of unknown producer
Date Thu, 03 Jan 2019 21:37:57 GMT
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 <jason@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 <wangguoz@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 <jason@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 <wangguoz@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 <
> matthias@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 <
> > jason@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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message