Catching up with this discussion.
I was just curious -- have we had other instances where downgrading MV is
not supported? I think Kafka typically tries to support downgrades, and I
couldn't think of other examples.
Thanks,
Justine
On Wed, Oct 4, 2023 at 9:40 AM Calvin Liu <ca...@confluent.io.invalid>
wrote:
> Hi Jun,
> 54. Marked the software downgrading is not supported. As the old controller
> will not understand the new PartitionRecord and PartitionChangeRecord.
> Thanks!
>
> On Wed, Oct 4, 2023 at 9:12 AM Jun Rao <j...@confluent.io.invalid> wrote:
>
> > Hi, Calvin,
> >
> > Thanks for the reply. Just one more comment.
> >
> > 54. It seems that downgrading MV is supported. Is downgrading the
> software
> > version supported? It would be useful to document that.
> >
> > Thanks,
> >
> > Jun
> >
> > On Tue, Oct 3, 2023 at 4:55 PM Artem Livshits
> > <alivsh...@confluent.io.invalid> wrote:
> >
> > > Hi Colin,
> > >
> > > I think in your example "do_unclean_recovery" would need to do
> different
> > > things depending on the strategy.
> > >
> > > do_unclean_recovery() {
> > > if (unclean.recovery.manager.enabled) {
> > > if (strategy == Aggressive)
> > > use UncleanRecoveryManager(waitLastKnownERL=false) // just
> inspect
> > > logs from whoever is available
> > > else
> > > use UncleanRecoveryManager(waitLastKnownERL=true) // must wait
> > for
> > > at least last known ELR
> > > } else {
> > > if (strategy == Aggressive)
> > > choose the last known leader if that is available, or a random
> > leader
> > > if not)
> > > else
> > > wait for last known leader to get back
> > > }
> > > }
> > >
> > > The idea is that the Aggressive strategy would kick in as soon as we
> lost
> > > the leader and would pick a leader from whoever is available; but the
> > > Balanced will only kick in when ELR is empty and will wait for the
> > brokers
> > > that likely have most data to be available.
> > >
> > > On Tue, Oct 3, 2023 at 3:04 PM Colin McCabe <cmcc...@apache.org>
> wrote:
> > >
> > > > On Tue, Oct 3, 2023, at 10:49, Jun Rao wrote:
> > > > > Hi, Calvin,
> > > > >
> > > > > Thanks for the update KIP. A few more comments.
> > > > >
> > > > > 41. Why would a user choose the option to select a random replica
> as
> > > the
> > > > > leader instead of using unclean.recovery.strateg=Aggressive? It
> seems
> > > > that
> > > > > the latter is strictly better? If that's not the case, could we
> fold
> > > this
> > > > > option under unclean.recovery.strategy instead of introducing a
> > > separate
> > > > > config?
> > > >
> > > > Hi Jun,
> > > >
> > > > I thought the flow of control was:
> > > >
> > > > If there is no leader for the partition {
> > > > If (there are unfenced ELR members) {
> > > > choose_an_unfenced_ELR_member
> > > > } else if (there are fenced ELR members AND strategy=Aggressive) {
> > > > do_unclean_recovery
> > > > } else if (there are no ELR members AND strategy != None) {
> > > > do_unclean_recovery
> > > > } else {
> > > > do nothing about the missing leader
> > > > }
> > > > }
> > > >
> > > > do_unclean_recovery() {
> > > > if (unclean.recovery.manager.enabled) {
> > > > use UncleanRecoveryManager
> > > > } else {
> > > > choose the last known leader if that is available, or a random
> > leader
> > > > if not)
> > > > }
> > > > }
> > > >
> > > > However, I think this could be clarified, especially the behavior
> when
> > > > unclean.recovery.manager.enabled=false. Inuitively the goal for
> > > > unclean.recovery.manager.enabled=false is to be "the same as now,
> > mostly"
> > > > but it's very underspecified in the KIP, I agree.
> > > >
> > > > >
> > > > > 50. ElectLeadersRequest: "If more than 20 topics are included, only
> > the
> > > > > first 20 will be served. Others will be returned with
> > DesiredLeaders."
> > > > Hmm,
> > > > > not sure that I understand this. ElectLeadersResponse doesn't have
> a
> > > > > DesiredLeaders field.
> > > > >
> > > > > 51. GetReplicaLogInfo: "If more than 2000 partitions are included,
> > only
> > > > the
> > > > > first 2000 will be served" Do we return an error for the remaining
> > > > > partitions? Actually, should we include an errorCode field at the
> > > > partition
> > > > > level in GetReplicaLogInfoResponse to cover non-existing partitions
> > and
> > > > no
> > > > > authorization, etc?
> > > > >
> > > > > 52. The entry should matches => The entry should match
> > > > >
> > > > > 53. ElectLeadersRequest.DesiredLeaders: Should it be nullable
> since a
> > > > user
> > > > > may not specify DesiredLeaders?
> > > > >
> > > > > 54. Downgrade: Is that indeed possible? I thought earlier you said
> > that
> > > > > once the new version of the records are in the metadata log, one
> > can't
> > > > > downgrade since the old broker doesn't know how to parse the new
> > > version
> > > > of
> > > > > the metadata records?
> > > > >
> > > >
> > > > MetadataVersion downgrade is currently broken but we have fixing it
> on
> > > our
> > > > plate for Kafka 3.7.
> > > >
> > > > The way downgrade works is that "new features" are dropped, leaving
> > only
> > > > the old ones.
> > > >
> > > > > 55. CleanShutdownFile: Should we add a version field for future
> > > > extension?
> > > > >
> > > > > 56. Config changes are public facing. Could we have a separate
> > section
> > > to
> > > > > document all the config changes?
> > > >
> > > > +1. A separate section for this would be good.
> > > >
> > > > best,
> > > > Colin
> > > >
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Jun
> > > > >
> > > > > On Mon, Sep 25, 2023 at 4:29 PM Calvin Liu
> > <ca...@confluent.io.invalid
> > > >
> > > > > wrote:
> > > > >
> > > > >> Hi Jun
> > > > >> Thanks for the comments.
> > > > >>
> > > > >> 40. If we change to None, it is not guaranteed for no data loss.
> For
> > > > users
> > > > >> who are not able to validate the data with external resources,
> > manual
> > > > >> intervention does not give a better result but a loss of
> > availability.
> > > > So
> > > > >> practically speaking, the Balance mode would be a better default
> > > value.
> > > > >>
> > > > >> 41. No, it represents how we want to do the unclean leader
> election.
> > > If
> > > > it
> > > > >> is false, the unclean leader election will be the old random way.
> > > > >> Otherwise, the unclean recovery will be used.
> > > > >>
> > > > >> 42. Good catch. Updated.
> > > > >>
> > > > >> 43. Only the first 20 topics will be served. Others will be
> returned
> > > > with
> > > > >> InvalidRequestError
> > > > >>
> > > > >> 44. The order matters. The desired leader entries match with the
> > topic
> > > > >> partition list by the index.
> > > > >>
> > > > >> 45. Thanks! Updated.
> > > > >>
> > > > >> 46. Good advice! Updated.
> > > > >>
> > > > >> 47.1, updated the comment. Basically it will elect the replica in
> > the
> > > > >> desiredLeader field to be the leader
> > > > >>
> > > > >> 47.2 We can let the admin client do the conversion. Using the
> > > > desiredLeader
> > > > >> field in the json format seems easier for users.
> > > > >>
> > > > >> 48. Once the MV version is downgraded, all the ELR related fields
> > will
> > > > be
> > > > >> removed on the next partition change. The controller will also
> > ignore
> > > > the
> > > > >> ELR fields. Updated the KIP.
> > > > >>
> > > > >> 49. Yes, it would be deprecated/removed.
> > > > >>
> > > > >>
> > > > >> On Mon, Sep 25, 2023 at 3:49 PM Jun Rao <j...@confluent.io.invalid
> >
> > > > wrote:
> > > > >>
> > > > >> > Hi, Calvin,
> > > > >> >
> > > > >> > Thanks for the updated KIP. Made another pass. A few more
> comments
> > > > below.
> > > > >> >
> > > > >> > 40. unclean.leader.election.enable.false ->
> > > > >> > unclean.recovery.strategy.Balanced: The Balanced mode could
> still
> > > > lead to
> > > > >> > data loss. So, I am wondering if
> > > unclean.leader.election.enable.false
> > > > >> > should map to None?
> > > > >> >
> > > > >> > 41. unclean.recovery.manager.enabled: I am not sure why we
> > introduce
> > > > this
> > > > >> > additional config. Is it the same as
> > unclean.recovery.strategy=None?
> > > > >> >
> > > > >> > 42. DescribeTopicResponse.TopicAuthorizedOperations: Should this
> > be
> > > at
> > > > >> the
> > > > >> > topic level?
> > > > >> >
> > > > >> > 43. "Limit: 20 topics max per request": Could we describe what
> > > > happens if
> > > > >> > the request includes more than 20 topics?
> > > > >> >
> > > > >> > 44. ElectLeadersRequest.DesiredLeaders: Could we describe
> whether
> > > the
> > > > >> > ordering matters?
> > > > >> >
> > > > >> > 45. GetReplicaLogInfo.TopicPartitions: "about": "The topic
> > > partitions
> > > > to
> > > > >> > elect leaders.": The description in "about" is incorrect.
> > > > >> >
> > > > >> > 46. GetReplicaLogInfoResponse: Should we nest partitions under
> > > > topicId to
> > > > >> > be consistent with other types of responses?
> > > > >> >
> > > > >> > 47. kafka-leader-election.sh:
> > > > >> > 47.1 Could we explain DESIGNATION?
> > > > >> > 47.2 desiredLeader: Should it be a list to match the field in
> > > > >> > ElectLeadersRequest?
> > > > >> >
> > > > >> > 48. We could add a section on downgrade?
> > > > >> >
> > > > >> > 49. LastKnownLeader: This seems only needed in the first phase
> of
> > > > >> > delivering ELR. Will it be removed when the complete KIP is
> > > delivered?
> > > > >> >
> > > > >> > Thanks,
> > > > >> >
> > > > >> > Jun
> > > > >> >
> > > > >> > On Tue, Sep 19, 2023 at 1:30 PM Colin McCabe <
> cmcc...@apache.org>
> > > > wrote:
> > > > >> >
> > > > >> > > Hi Calvin,
> > > > >> > >
> > > > >> > > Thanks for the explanations. I like the idea of using none,
> > > > balanced,
> > > > >> > > aggressive. We also had an offline discussion about why it is
> > good
> > > > to
> > > > >> > use a
> > > > >> > > new config key (basically, so that we can deprecate the old
> one
> > > > which
> > > > >> had
> > > > >> > > only false/true values in 4.0) With these changes, I am +1.
> > > > >> > >
> > > > >> > > best,
> > > > >> > > Colin
> > > > >> > >
> > > > >> > > On Mon, Sep 18, 2023, at 15:54, Calvin Liu wrote:
> > > > >> > > > Hi Colin,
> > > > >> > > > Also, can we deprecate unclean.leader.election.enable in
> 4.0?
> > > > Before
> > > > >> > > that,
> > > > >> > > > we can have both the config unclean.recovery.strategy and
> > > > >> > > > unclean.leader.election.enable
> > > > >> > > > and using the unclean.recovery.Enabled to determine which
> > config
> > > > to
> > > > >> use
> > > > >> > > > during the unclean leader election.
> > > > >> > > >
> > > > >> > > > On Mon, Sep 18, 2023 at 3:51 PM Calvin Liu <
> > ca...@confluent.io>
> > > > >> wrote:
> > > > >> > > >
> > > > >> > > >> Hi Colin,
> > > > >> > > >> For the unclean.recovery.strategy config name, how about we
> > use
> > > > the
> > > > >> > > >> following
> > > > >> > > >> None. It basically means no unclean recovery will be
> > performed.
> > > > >> > > >> Aggressive. It means availability goes first. Whenever the
> > > > partition
> > > > >> > > can't
> > > > >> > > >> elect a durable replica, the controller will try the
> unclean
> > > > >> recovery.
> > > > >> > > >> Balanced. It is the balance point of the availability
> > > > >> > first(Aggressive)
> > > > >> > > >> and least availability(None). The controller performs
> unclean
> > > > >> recovery
> > > > >> > > when
> > > > >> > > >> both ISR and ELR are empty.
> > > > >> > > >>
> > > > >> > > >>
> > > > >> > > >> On Fri, Sep 15, 2023 at 11:42 AM Calvin Liu <
> > > ca...@confluent.io>
> > > > >> > wrote:
> > > > >> > > >>
> > > > >> > > >>> Hi Colin,
> > > > >> > > >>>
> > > > >> > > >>> > So, the proposal is that if someone sets
> > > > >> > > "unclean.leader.election.enable
> > > > >> > > >>> = true"...
> > > > >> > > >>>
> > > > >> > > >>>
> > > > >> > > >>> The idea is to use one of the
> unclean.leader.election.enable
> > > and
> > > > >> > > >>> unclean.recovery.strategy based on the
> > > > unclean.recovery.Enabled. A
> > > > >> > > possible
> > > > >> > > >>> version can be
> > > > >> > > >>>
> > > > >> > > >>> If unclean.recovery.Enabled:
> > > > >> > > >>>
> > > > >> > > >>> {
> > > > >> > > >>>
> > > > >> > > >>> Check unclean.recovery.strategy. If set, use it.
> Otherwise,
> > > > check
> > > > >> > > >>> unclean.leader.election.enable and translate it to
> > > > >> > > >>> unclean.recovery.strategy.
> > > > >> > > >>>
> > > > >> > > >>> } else {
> > > > >> > > >>>
> > > > >> > > >>> Use unclean.leader.election.enable
> > > > >> > > >>>
> > > > >> > > >>> }
> > > > >> > > >>>
> > > > >> > > >>>
> > > > >> > > >>> —--------
> > > > >> > > >>>
> > > > >> > > >>> >The configuration key should be
> > > > >> "unclean.recovery.manager.enabled",
> > > > >> > > >>> right?
> > > > >> > > >>>
> > > > >> > > >>>
> > > > >> > > >>> I think we have two ways of choosing a leader uncleanly,
> > > unclean
> > > > >> > leader
> > > > >> > > >>> election and unclean recovery(log inspection) and we try
> to
> > > > switch
> > > > >> > > between
> > > > >> > > >>> them.
> > > > >> > > >>>
> > > > >> > > >>> Do you mean we want to develop two ways of performing the
> > > > unclean
> > > > >> > > >>> recovery and one of them is using “unclean recovery
> > manager”?
> > > I
> > > > >> guess
> > > > >> > > we
> > > > >> > > >>> haven’t discussed the second way.
> > > > >> > > >>>
> > > > >> > > >>>
> > > > >> > > >>> —-------
> > > > >> > > >>>
> > > > >> > > >>> >How do these 4 levels of overrides interact with your new
> > > > >> > > >>> configurations?
> > > > >> > > >>>
> > > > >> > > >>>
> > > > >> > > >>> I do notice in the Kraft controller code, the method to
> > check
> > > > >> whether
> > > > >> > > >>> perform unclean leader election is hard coded to false
> since
> > > > >> > > >>> 2021(uncleanLeaderElectionEnabledForTopic). Isn’t it a
> good
> > > > chance
> > > > >> to
> > > > >> > > >>> completely deprecate the unclean.leader.election.enable?
> We
> > > > don’t
> > > > >> > even
> > > > >> > > have
> > > > >> > > >>> to worry about the config conversion.
> > > > >> > > >>>
> > > > >> > > >>> On the other hand, whatever the override is, as long as
> the
> > > > >> > controller
> > > > >> > > >>> can have the final effective
> unclean.leader.election.enable,
> > > the
> > > > >> > topic
> > > > >> > > >>> level config unclean.recovery.strategy, the cluster level
> > > config
> > > > >> > > >>> unclean.recovery.Enabled, the controller can calculate the
> > > > correct
> > > > >> > > methods
> > > > >> > > >>> to use right?
> > > > >> > > >>>
> > > > >> > > >>>
> > > > >> > > >>> On Fri, Sep 15, 2023 at 10:02 AM Colin McCabe <
> > > > cmcc...@apache.org>
> > > > >> > > wrote:
> > > > >> > > >>>
> > > > >> > > >>>> On Thu, Sep 14, 2023, at 22:23, Calvin Liu wrote:
> > > > >> > > >>>> > Hi Colin
> > > > >> > > >>>> > 1. I think using the new config name is more clear.
> > > > >> > > >>>> > a. The unclean leader election is actually
> removed
> > > if
> > > > >> > unclean
> > > > >> > > >>>> > recovery is in use.
> > > > >> > > >>>> > b. Using multiple values in
> > > > >> unclean.leader.election.enable
> > > > >> > is
> > > > >> > > >>>> > confusing and it will be more confusing after people
> > forget
> > > > >> about
> > > > >> > > this
> > > > >> > > >>>> > discussion.
> > > > >> > > >>>>
> > > > >> > > >>>> Hi Calvin,
> > > > >> > > >>>>
> > > > >> > > >>>> So, the proposal is that if someone sets
> > > > >> > > "unclean.leader.election.enable
> > > > >> > > >>>> = true" but then sets one of your new configurations, the
> > > > value of
> > > > >> > > >>>> unclean.leader.election.enable is ignored? That seems
> less
> > > > clear
> > > > >> to
> > > > >> > > me, not
> > > > >> > > >>>> more. Just in general, having multiple configuration keys
> > to
> > > > >> control
> > > > >> > > the
> > > > >> > > >>>> same thing confuses users. Basically, they are sitting
> at a
> > > > giant
> > > > >> > > control
> > > > >> > > >>>> panel, and some of the levers do nothing.
> > > > >> > > >>>>
> > > > >> > > >>>> > 2. Sorry I forgot to mention in the response that I did
> > add
> > > > the
> > > > >> > > >>>> > unclean.recovery.Enabled flag.
> > > > >> > > >>>>
> > > > >> > > >>>> The configuration key should be
> > > > >> "unclean.recovery.manager.enabled",
> > > > >> > > >>>> right? Becuase we can do "unclean recovery" without the
> > > > manager.
> > > > >> > > Disabling
> > > > >> > > >>>> the manager just means we use a different mechanism for
> > > > recovery.
> > > > >> > > >>>>
> > > > >> > > >>>> > c. Maybe I underestimated the challenge of
> > replacing
> > > > the
> > > > >> > > >>>> config. Any
> > > > >> > > >>>> > implementation problems ahead?
> > > > >> > > >>>>
> > > > >> > > >>>> There are four levels of overrides for
> > > > >> > unclean.leader.election.enable.
> > > > >> > > >>>>
> > > > >> > > >>>> 1. static configuration for node.
> > > > >> > > >>>> This goes in the configuration file, typically named
> > > > >> > > >>>> server.properties
> > > > >> > > >>>>
> > > > >> > > >>>> 2. dynamic configuration for node default
> > > > >> > > >>>> ConfigResource(type=BROKER, name="")
> > > > >> > > >>>>
> > > > >> > > >>>> 3. dynamic configuration for node
> > > > >> > > >>>> ConfigResource(type=BROKER, name=<controller id>)
> > > > >> > > >>>>
> > > > >> > > >>>> 4. dynamic configuration for topic
> > > > >> > > >>>> ConfigResource(type=TOPIC, name=<topic-name>)
> > > > >> > > >>>>
> > > > >> > > >>>> How do these 4 levels of overrides interact with your new
> > > > >> > > >>>> configurations? If the new configurations dominate over
> the
> > > old
> > > > >> > ones,
> > > > >> > > it
> > > > >> > > >>>> seems like this will get a lot more confusing to
> implement
> > > (and
> > > > >> also
> > > > >> > > to
> > > > >> > > >>>> use.)
> > > > >> > > >>>>
> > > > >> > > >>>> Again, I'd recommend just adding some new values to
> > > > >> > > >>>> unclean.leader.election.enable. It's simple and will
> > prevent
> > > > user
> > > > >> > > confusion
> > > > >> > > >>>> (as well as developer confusion.)
> > > > >> > > >>>>
> > > > >> > > >>>> best,
> > > > >> > > >>>> Colin
> > > > >> > > >>>>
> > > > >> > > >>>>
> > > > >> > > >>>> > 3. About the admin client, I mentioned 3 changes in the
> > > > client.
> > > > >> > > >>>> Anything
> > > > >> > > >>>> > else I missed in the KIP?
> > > > >> > > >>>> > a. The client will switch to using the new RPC
> > > instead
> > > > of
> > > > >> > > >>>> > MetadataRequest for the topics.
> > > > >> > > >>>> > b. The TopicPartitionInfo used in
> TopicDescription
> > > > needs
> > > > >> to
> > > > >> > > add
> > > > >> > > >>>> new
> > > > >> > > >>>> > fields related to the ELR.
> > > > >> > > >>>> > c. The outputs will add the ELR related fields.
> > > > >> > > >>>> >
> > > > >> > > >>>> > On Thu, Sep 14, 2023 at 9:19 PM Colin McCabe <
> > > > >> cmcc...@apache.org>
> > > > >> > > >>>> wrote:
> > > > >> > > >>>> >
> > > > >> > > >>>> >> Hi Calvin,
> > > > >> > > >>>> >>
> > > > >> > > >>>> >> Thanks for the changes.
> > > > >> > > >>>> >>
> > > > >> > > >>>> >> 1. Earlier I commented that creating
> > > > >> "unclean.recovery.strategy "
> > > > >> > > is
> > > > >> > > >>>> not
> > > > >> > > >>>> >> necessary, and we can just reuse the existing
> > > > >> > > >>>> >> "unclean.leader.election.enable" configuration key.
> > Let's
> > > > >> discuss
> > > > >> > > >>>> that.
> > > > >> > > >>>> >>
> > > > >> > > >>>> >> 2.I also don't understand why you didn't add a
> > > > configuration to
> > > > >> > > >>>> enable or
> > > > >> > > >>>> >> disable the Unclean Recovery Manager. This seems like
> a
> > > very
> > > > >> > simple
> > > > >> > > >>>> way to
> > > > >> > > >>>> >> handle the staging issue which we discussed. The URM
> can
> > > > just
> > > > >> be
> > > > >> > > >>>> turned off
> > > > >> > > >>>> >> until it is production ready. Let's discuss this.
> > > > >> > > >>>> >>
> > > > >> > > >>>> >> 3. You still need to describe the changes to
> AdminClient
> > > > that
> > > > >> are
> > > > >> > > >>>> needed
> > > > >> > > >>>> >> to use DescribeTopicRequest.
> > > > >> > > >>>> >>
> > > > >> > > >>>> >> Keep at it. It's looking better. :)
> > > > >> > > >>>> >>
> > > > >> > > >>>> >> best,
> > > > >> > > >>>> >> Colin
> > > > >> > > >>>> >>
> > > > >> > > >>>> >>
> > > > >> > > >>>> >> On Thu, Sep 14, 2023, at 11:03, Calvin Liu wrote:
> > > > >> > > >>>> >> > Hi Colin
> > > > >> > > >>>> >> > Thanks for the comments!
> > > > >> > > >>>> >> >
> > > > >> > > >>>> >> > I did the following changes
> > > > >> > > >>>> >> >
> > > > >> > > >>>> >> > 1.
> > > > >> > > >>>> >> >
> > > > >> > > >>>> >> > Simplified the API spec section to only include
> the
> > > > diff.
> > > > >> > > >>>> >> > 2.
> > > > >> > > >>>> >> >
> > > > >> > > >>>> >> > Reordered the HWM requirement section.
> > > > >> > > >>>> >> > 3.
> > > > >> > > >>>> >> >
> > > > >> > > >>>> >> > Removed the URM implementation details to keep
> the
> > > > >> necessary
> > > > >> > > >>>> >> > characteristics to perform the unclean recovery.
> > > > >> > > >>>> >> > 1.
> > > > >> > > >>>> >> >
> > > > >> > > >>>> >> > When to perform the unclean recovery
> > > > >> > > >>>> >> > 2.
> > > > >> > > >>>> >> >
> > > > >> > > >>>> >> > Under different config, how the unclean
> recovery
> > > > finds
> > > > >> > the
> > > > >> > > >>>> leader.
> > > > >> > > >>>> >> > 3.
> > > > >> > > >>>> >> >
> > > > >> > > >>>> >> > How the config unclean.leader.election.enable
> > and
> > > > >> > > >>>> >> > unclean.recovery.strategy are converted when
> > users
> > > > >> > > >>>> enable/disable
> > > > >> > > >>>> >> the
> > > > >> > > >>>> >> > unclean recovery.
> > > > >> > > >>>> >> > 4.
> > > > >> > > >>>> >> >
> > > > >> > > >>>> >> > More details about how we change admin client.
> > > > >> > > >>>> >> > 5.
> > > > >> > > >>>> >> >
> > > > >> > > >>>> >> > API limits on the GetReplicaLogInfoRequest and
> > > > >> > > >>>> DescribeTopicRequest.
> > > > >> > > >>>> >> > 6.
> > > > >> > > >>>> >> >
> > > > >> > > >>>> >> > Two metrics added
> > > > >> > > >>>> >> > 1.
> > > > >> > > >>>> >> >
> > > > >> > > >>>> >> >
> > > > Kafka.controller.global_under_min_isr_partition_count
> > > > >> > > >>>> >> > 2.
> > > > >> > > >>>> >> >
> > > > >> > > >>>> >> >
> kafka.controller.unclean_recovery_finished_count
> > > > >> > > >>>> >> >
> > > > >> > > >>>> >> >
> > > > >> > > >>>> >> > On Wed, Sep 13, 2023 at 10:46 AM Colin McCabe <
> > > > >> > > cmcc...@apache.org>
> > > > >> > > >>>> >> wrote:
> > > > >> > > >>>> >> >
> > > > >> > > >>>> >> >> On Tue, Sep 12, 2023, at 17:21, Calvin Liu wrote:
> > > > >> > > >>>> >> >> > Hi Colin
> > > > >> > > >>>> >> >> > Thanks for the comments!
> > > > >> > > >>>> >> >> >
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> Hi Calvin,
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> Thanks again for the KIP.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> One meta-comment: it's usually better to just do a
> > diff
> > > > on a
> > > > >> > > >>>> message
> > > > >> > > >>>> >> spec
> > > > >> > > >>>> >> >> file or java file if you're including changes to it
> > in
> > > > the
> > > > >> > KIP.
> > > > >> > > >>>> This is
> > > > >> > > >>>> >> >> easier to read than looking for "new fields begin"
> > etc.
> > > > in
> > > > >> the
> > > > >> > > >>>> text, and
> > > > >> > > >>>> >> >> gracefully handles the case where existing fields
> > were
> > > > >> > changed.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> > Rewrite the Additional High Watermark advancement
> > > > >> > requirement
> > > > >> > > >>>> >> >> > There was feedback on this section that some
> > readers
> > > > may
> > > > >> not
> > > > >> > > be
> > > > >> > > >>>> >> familiar
> > > > >> > > >>>> >> >> > with HWM and Ack=0,1,all requests. This can help
> > them
> > > > >> > > understand
> > > > >> > > >>>> the
> > > > >> > > >>>> >> >> > proposal. I will rewrite this part for more
> > > > readability.
> > > > >> > > >>>> >> >> >
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> To be clear, I wasn't suggesting dropping either
> > > > section. I
> > > > >> > > agree
> > > > >> > > >>>> that
> > > > >> > > >>>> >> >> they add useful background. I was just suggesting
> > that
> > > we
> > > > >> > should
> > > > >> > > >>>> discuss
> > > > >> > > >>>> >> >> the "acks" setting AFTER discussing the new high
> > > > watermark
> > > > >> > > >>>> advancement
> > > > >> > > >>>> >> >> conditions. We also should discuss acks=0. While it
> > > isn't
> > > > >> > > >>>> conceptually
> > > > >> > > >>>> >> much
> > > > >> > > >>>> >> >> different than acks=1 here, its omission from this
> > > > section
> > > > >> is
> > > > >> > > >>>> confusing.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> > Unclean recovery
> > > > >> > > >>>> >> >> >
> > > > >> > > >>>> >> >> > The plan is to replace the
> > > > unclean.leader.election.enable
> > > > >> > with
> > > > >> > > >>>> >> >> > unclean.recovery.strategy. If the Unclean
> Recovery
> > is
> > > > >> > enabled
> > > > >> > > >>>> then it
> > > > >> > > >>>> >> >> deals
> > > > >> > > >>>> >> >> > with the three options in the
> > > > unclean.recovery.strategy.
> > > > >> > > >>>> >> >> >
> > > > >> > > >>>> >> >> >
> > > > >> > > >>>> >> >> > Let’s refine the Unclean Recovery. We have
> already
> > > > taken a
> > > > >> > > lot of
> > > > >> > > >>>> >> >> > suggestions and I hope to enhance the durability
> of
> > > > Kafka
> > > > >> to
> > > > >> > > the
> > > > >> > > >>>> next
> > > > >> > > >>>> >> >> level
> > > > >> > > >>>> >> >> > with this KIP.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> I am OK with doing the unclean leader recovery
> > > > improvements
> > > > >> in
> > > > >> > > >>>> this KIP.
> > > > >> > > >>>> >> >> However, I think we need to really work on the
> > > > configuration
> > > > >> > > >>>> settings.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> Configuration overrides are often quite messy. For
> > > > example,
> > > > >> > the
> > > > >> > > >>>> cases
> > > > >> > > >>>> >> >> where we have log.roll.hours and
> log.roll.segment.ms
> > ,
> > > > the
> > > > >> > user
> > > > >> > > >>>> has to
> > > > >> > > >>>> >> >> remember which one takes precedence, and it is not
> > > > obvious.
> > > > >> > So,
> > > > >> > > >>>> rather
> > > > >> > > >>>> >> than
> > > > >> > > >>>> >> >> creating a new configuration, why not add
> additional
> > > > values
> > > > >> to
> > > > >> > > >>>> >> >> "unclean.leader.election.enable"? I think this will
> > be
> > > > >> simpler
> > > > >> > > for
> > > > >> > > >>>> >> people
> > > > >> > > >>>> >> >> to understand, and simpler in the code as well.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> What if we continued to use
> > > > "unclean.leader.election.enable"
> > > > >> > but
> > > > >> > > >>>> >> extended
> > > > >> > > >>>> >> >> it so that it took a string? Then the string could
> > have
> > > > >> these
> > > > >> > > >>>> values:
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> never
> > > > >> > > >>>> >> >> never automatically do an unclean leader
> election
> > > > under
> > > > >> > any
> > > > >> > > >>>> >> conditions
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> false / default
> > > > >> > > >>>> >> >> only do an unclean leader election if there may
> > be
> > > > >> > possible
> > > > >> > > >>>> data
> > > > >> > > >>>> >> loss
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> true / always
> > > > >> > > >>>> >> >> always do an unclean leader election if we
> can't
> > > > >> > immediately
> > > > >> > > >>>> elect a
> > > > >> > > >>>> >> >> leader
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> It's a bit awkward that false maps to default
> rather
> > > > than to
> > > > >> > > >>>> never. But
> > > > >> > > >>>> >> >> this awkwardness exists if we use two different
> > > > >> configuration
> > > > >> > > keys
> > > > >> > > >>>> as
> > > > >> > > >>>> >> well.
> > > > >> > > >>>> >> >> The reason for the awkwardness is that we simply
> > don't
> > > > want
> > > > >> > most
> > > > >> > > >>>> of the
> > > > >> > > >>>> >> >> people currently setting
> > > > >> unclean.leader.election.enable=false
> > > > >> > to
> > > > >> > > >>>> get the
> > > > >> > > >>>> >> >> "never" behavior. We have to bite that bullet.
> Better
> > > to
> > > > be
> > > > >> > > clear
> > > > >> > > >>>> and
> > > > >> > > >>>> >> >> explicit than hide it.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> Another thing that's a bit awkward is having two
> > > > different
> > > > >> > ways
> > > > >> > > to
> > > > >> > > >>>> do
> > > > >> > > >>>> >> >> unclean leader election specified in the KIP. You
> > > > descirbe
> > > > >> two
> > > > >> > > >>>> methods:
> > > > >> > > >>>> >> the
> > > > >> > > >>>> >> >> simple "choose the last leader" method, and the
> > > "unclean
> > > > >> > > recovery
> > > > >> > > >>>> >> manager"
> > > > >> > > >>>> >> >> method. I understand why you did it this way --
> > "choose
> > > > the
> > > > >> > last
> > > > >> > > >>>> >> leader" is
> > > > >> > > >>>> >> >> simple, and will help us deliver an implementation
> > > > quickly,
> > > > >> > > while
> > > > >> > > >>>> the
> > > > >> > > >>>> >> URM
> > > > >> > > >>>> >> >> is preferable in the long term. My suggestion here
> is
> > > to
> > > > >> > > separate
> > > > >> > > >>>> the
> > > > >> > > >>>> >> >> decision of HOW to do unclean leader election from
> > the
> > > > >> > decision
> > > > >> > > of
> > > > >> > > >>>> WHEN
> > > > >> > > >>>> >> to
> > > > >> > > >>>> >> >> do it.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> So in other words, have
> > > "unclean.leader.election.enable"
> > > > >> > specify
> > > > >> > > >>>> when we
> > > > >> > > >>>> >> >> do unclean leader election, and have a new
> > > configuration
> > > > >> like
> > > > >> > > >>>> >> >> "unclean.recovery.manager.enable" to determine if
> we
> > > use
> > > > the
> > > > >> > > URM.
> > > > >> > > >>>> >> >> Presumably the URM will take some time to get fully
> > > > stable,
> > > > >> so
> > > > >> > > >>>> this can
> > > > >> > > >>>> >> >> default to false for a while, and we can flip the
> > > > default to
> > > > >> > > true
> > > > >> > > >>>> when
> > > > >> > > >>>> >> we
> > > > >> > > >>>> >> >> feel ready.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> The URM is somewhat under-described here. I think
> we
> > > > need a
> > > > >> > few
> > > > >> > > >>>> >> >> configurations here for it. For example, we need a
> > > > >> > > configuration to
> > > > >> > > >>>> >> specify
> > > > >> > > >>>> >> >> how long it should wait for a broker to respond to
> > its
> > > > RPCs
> > > > >> > > before
> > > > >> > > >>>> >> moving
> > > > >> > > >>>> >> >> on. We also need to understand how the URM
> interacts
> > > with
> > > > >> > > >>>> >> >> unclean.leader.election.enable=always. I assume
> that
> > > with
> > > > >> > > "always"
> > > > >> > > >>>> we
> > > > >> > > >>>> >> will
> > > > >> > > >>>> >> >> just unconditionally use the URM rather than
> choosing
> > > > >> > randomly.
> > > > >> > > >>>> But this
> > > > >> > > >>>> >> >> should be spelled out in the KIP.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> >
> > > > >> > > >>>> >> >> > DescribeTopicRequest
> > > > >> > > >>>> >> >> >
> > > > >> > > >>>> >> >> > 1.
> > > > >> > > >>>> >> >> > Yes, the plan is to replace the
> MetadataRequest
> > > with
> > > > >> the
> > > > >> > > >>>> >> >> > DescribeTopicRequest for the admin clients.
> Will
> > > > check
> > > > >> > the
> > > > >> > > >>>> details.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> Sounds good. But as I said, you need to specify how
> > > > >> > AdminClient
> > > > >> > > >>>> >> interacts
> > > > >> > > >>>> >> >> with the new request. This will involve adding some
> > > > fields
> > > > >> to
> > > > >> > > >>>> >> >> TopicDescription.java. And you need to specify the
> > > > changes
> > > > >> to
> > > > >> > > the
> > > > >> > > >>>> >> >> kafka-topics.sh command line tool. Otherwise we
> > cannot
> > > > use
> > > > >> the
> > > > >> > > >>>> tool to
> > > > >> > > >>>> >> see
> > > > >> > > >>>> >> >> the new information.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> The new requests, DescribeTopicRequest and
> > > > >> > > >>>> GetReplicaLogInfoRequest,
> > > > >> > > >>>> >> need
> > > > >> > > >>>> >> >> to have limits placed on them so that their size
> > can't
> > > be
> > > > >> > > >>>> infinite. We
> > > > >> > > >>>> >> >> don't want to propagate the current problems of
> > > > >> > MetadataRequest,
> > > > >> > > >>>> where
> > > > >> > > >>>> >> >> clients can request massive responses that can mess
> > up
> > > > the
> > > > >> JVM
> > > > >> > > when
> > > > >> > > >>>> >> handled.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> Adding limits is simple for
> GetReplicaLogInfoRequest
> > --
> > > > we
> > > > >> can
> > > > >> > > >>>> just say
> > > > >> > > >>>> >> >> that only 2000 partitions at a time can be
> requested.
> > > For
> > > > >> > > >>>> >> >> DescribeTopicRequest we can probably just limit to
> 20
> > > > topics
> > > > >> > or
> > > > >> > > >>>> >> something
> > > > >> > > >>>> >> >> like that, to avoid the complexity of doing
> > pagination
> > > in
> > > > >> this
> > > > >> > > KIP.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> > 2.
> > > > >> > > >>>> >> >> > I can let the broker load the ELR info so that
> > > they
> > > > can
> > > > >> > > serve
> > > > >> > > >>>> the
> > > > >> > > >>>> >> >> > DescribeTopicRequest as well.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> Yes, it's fine to add to MetadataCache. In fact,
> > you'll
> > > > be
> > > > >> > > loading
> > > > >> > > >>>> it
> > > > >> > > >>>> >> >> anyway once it's added to PartitionImage.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> > 3.
> > > > >> > > >>>> >> >> > Yeah, it does not make sense to have the topic
> > id
> > > if
> > > > >> > > >>>> >> >> > DescribeTopicRequest is only used by the admin
> > > > client.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> OK. That makes things simpler. We can always
> create a
> > > new
> > > > >> API
> > > > >> > > later
> > > > >> > > >>>> >> >> (hopefully not in this KIP!) to query by topic ID.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> >
> > > > >> > > >>>> >> >> >
> > > > >> > > >>>> >> >> > Metrics
> > > > >> > > >>>> >> >> >
> > > > >> > > >>>> >> >> > As for overall cluster health metrics, I think
> > > > >> under-min-ISR
> > > > >> > > is
> > > > >> > > >>>> still
> > > > >> > > >>>> >> a
> > > > >> > > >>>> >> >> > useful one. ELR is more like a safety belt. When
> > the
> > > > ELR
> > > > >> is
> > > > >> > > >>>> used, the
> > > > >> > > >>>> >> >> > cluster availability has already been impacted.
> > > > >> > > >>>> >> >> >
> > > > >> > > >>>> >> >> > Maybe we can have a metric to count the
> partitions
> > > that
> > > > >> > > sum(ISR,
> > > > >> > > >>>> ELR)
> > > > >> > > >>>> >> <
> > > > >> > > >>>> >> >> min
> > > > >> > > >>>> >> >> > ISR. What do you think?
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> How about:
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> A. a metric for the totoal number of under-min-isr
> > > > >> > partitions?
> > > > >> > > We
> > > > >> > > >>>> don't
> > > > >> > > >>>> >> >> have that in Apache Kafka at the moment.
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> B. a metric for the number of unclean leader
> > elections
> > > we
> > > > >> did
> > > > >> > > (for
> > > > >> > > >>>> >> >> simplicity, it can reset to 0 on controller
> restart:
> > we
> > > > >> expect
> > > > >> > > >>>> people to
> > > > >> > > >>>> >> >> monitor the change over time anyway)
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> best,
> > > > >> > > >>>> >> >> Colin
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >> >> >
> > > > >> > > >>>> >> >> > Yeah, for the ongoing unclean recoveries, the
> > > > controller
> > > > >> can
> > > > >> > > >>>> keep an
> > > > >> > > >>>> >> >> > accurate count through failover because partition
> > > > >> > registration
> > > > >> > > >>>> can
> > > > >> > > >>>> >> >> indicate
> > > > >> > > >>>> >> >> > whether a recovery is needed. However, for the
> > > happened
> > > > >> > ones,
> > > > >> > > >>>> unless
> > > > >> > > >>>> >> we
> > > > >> > > >>>> >> >> > want to persist the number somewhere, we can only
> > > > figure
> > > > >> it
> > > > >> > > out
> > > > >> > > >>>> from
> > > > >> > > >>>> >> the
> > > > >> > > >>>> >> >> > log.
> > > > >> > > >>>> >> >> >
> > > > >> > > >>>> >> >> > On Tue, Sep 12, 2023 at 3:16 PM Colin McCabe <
> > > > >> > > cmcc...@apache.org
> > > > >> > > >>>> >
> > > > >> > > >>>> >> wrote:
> > > > >> > > >>>> >> >> >
> > > > >> > > >>>> >> >> >> Also, we should have metrics that show what is
> > going
> > > > on
> > > > >> > with
> > > > >> > > >>>> regard
> > > > >> > > >>>> >> to
> > > > >> > > >>>> >> >> the
> > > > >> > > >>>> >> >> >> eligible replica set. I'm not sure exactly what
> to
> > > > >> suggest,
> > > > >> > > but
> > > > >> > > >>>> >> >> something
> > > > >> > > >>>> >> >> >> that could identify when things are going wrong
> in
> > > the
> > > > >> > > clsuter.
> > > > >> > > >>>> >> >> >>
> > > > >> > > >>>> >> >> >> For example, maybe a metric for partitions
> > > containing
> > > > >> > > replicas
> > > > >> > > >>>> that
> > > > >> > > >>>> >> are
> > > > >> > > >>>> >> >> >> ineligible to be leader? That would show a spike
> > > when
> > > > a
> > > > >> > > broker
> > > > >> > > >>>> had an
> > > > >> > > >>>> >> >> >> unclean restart.
> > > > >> > > >>>> >> >> >>
> > > > >> > > >>>> >> >> >> Ideally, we'd also have a metric that indicates
> > when
> > > > an
> > > > >> > > unclear
> > > > >> > > >>>> >> leader
> > > > >> > > >>>> >> >> >> election or a recovery happened. It's a bit
> tricky
> > > > >> because
> > > > >> > > the
> > > > >> > > >>>> simple
> > > > >> > > >>>> >> >> >> thing, of tracking it per controller, may be a
> bit
> > > > >> > confusing
> > > > >> > > >>>> during
> > > > >> > > >>>> >> >> >> failovers.
> > > > >> > > >>>> >> >> >>
> > > > >> > > >>>> >> >> >> best,
> > > > >> > > >>>> >> >> >> Colin
> > > > >> > > >>>> >> >> >>
> > > > >> > > >>>> >> >> >>
> > > > >> > > >>>> >> >> >> On Tue, Sep 12, 2023, at 14:25, Colin McCabe
> > wrote:
> > > > >> > > >>>> >> >> >> > Hi Calvin,
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> > Thanks for the KIP. I think this is a great
> > > > >> improvement.
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> >> Additional High Watermark advance requirement
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> > Typo: change "advance" to "advancement"
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> >> A bit recap of some key concepts.
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> > Typo: change "bit" to "quick"
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> >> Ack=1/all produce request. It defines when
> the
> > > > Kafka
> > > > >> > > server
> > > > >> > > >>>> should
> > > > >> > > >>>> >> >> >> respond to the produce request
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> > I think this section would be clearer if we
> > talked
> > > > >> about
> > > > >> > > the
> > > > >> > > >>>> new
> > > > >> > > >>>> >> high
> > > > >> > > >>>> >> >> >> > watermark advancement requirement first, and
> > THEN
> > > > >> talked
> > > > >> > > >>>> about its
> > > > >> > > >>>> >> >> >> > impact on acks=0, acks=1, and acks=all.
> > > > acks=all
> > > > >> is
> > > > >> > of
> > > > >> > > >>>> course
> > > > >> > > >>>> >> the
> > > > >> > > >>>> >> >> >> > main case we care about here, so it would be
> > good
> > > to
> > > > >> lead
> > > > >> > > with
> > > > >> > > >>>> >> that,
> > > > >> > > >>>> >> >> >> > rather than delving into the technicalities of
> > > > acks=0/1
> > > > >> > > first.
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> >> Unclean recovery
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> > So, here you are introducing a new
> > configuration,
> > > > >> > > >>>> >> >> >> > unclean.recovery.strategy. The difficult thing
> > > here
> > > > is
> > > > >> > that
> > > > >> > > >>>> there
> > > > >> > > >>>> >> is a
> > > > >> > > >>>> >> >> >> > lot of overlap with
> > > unclean.leader.election.enable.
> > > > So
> > > > >> we
> > > > >> > > >>>> have 3
> > > > >> > > >>>> >> >> >> > different settings for
> > unclean.recovery.strategy,
> > > > plus
> > > > >> 2
> > > > >> > > >>>> different
> > > > >> > > >>>> >> >> >> > settings for unclean.leader.election.enable,
> > > giving
> > > > a
> > > > >> > cross
> > > > >> > > >>>> >> product of
> > > > >> > > >>>> >> >> >> > 6 different options. The following "unclean
> > > recovery
> > > > >> > > manager"
> > > > >> > > >>>> >> section
> > > > >> > > >>>> >> >> >> > only applies to one fo those 6 different
> > > > possibilities
> > > > >> (I
> > > > >> > > >>>> think?)
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> > I simply don't think we need so many different
> > > > election
> > > > >> > > types.
> > > > >> > > >>>> >> Really
> > > > >> > > >>>> >> >> >> > the use-cases we need are people who want NO
> > > unclean
> > > > >> > > >>>> elections,
> > > > >> > > >>>> >> people
> > > > >> > > >>>> >> >> >> > who want "the reasonable thing" and people who
> > > want
> > > > >> > > >>>> avaialbility at
> > > > >> > > >>>> >> >> all
> > > > >> > > >>>> >> >> >> > costs.
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> > Overall, I feel like the first half of the KIP
> > is
> > > > about
> > > > >> > the
> > > > >> > > >>>> ELR,
> > > > >> > > >>>> >> and
> > > > >> > > >>>> >> >> >> > the second half is about reworking unclean
> > leader
> > > > >> > > election. It
> > > > >> > > >>>> >> might
> > > > >> > > >>>> >> >> be
> > > > >> > > >>>> >> >> >> > better to move that second half to a separate
> > KIP
> > > so
> > > > >> that
> > > > >> > > we
> > > > >> > > >>>> can
> > > > >> > > >>>> >> >> figure
> > > > >> > > >>>> >> >> >> > it out fully. It should be fine to punt this
> > until
> > > > >> later
> > > > >> > > and
> > > > >> > > >>>> just
> > > > >> > > >>>> >> have
> > > > >> > > >>>> >> >> >> > the current behavior on empty ELR be waiting
> for
> > > the
> > > > >> last
> > > > >> > > >>>> known
> > > > >> > > >>>> >> leader
> > > > >> > > >>>> >> >> >> > to return. After all, that's what we do today.
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> >> DescribeTopicRequest
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> > Is the intention for AdminClient to use this
> RPC
> > > for
> > > > >> > > >>>> >> >> >> > Admin#describeTopics ? If so, we need to
> > describe
> > > > all
> > > > >> of
> > > > >> > > the
> > > > >> > > >>>> >> changes
> > > > >> > > >>>> >> >> to
> > > > >> > > >>>> >> >> >> > the admin client API, as well as changes to
> > > > >> command-line
> > > > >> > > >>>> tools like
> > > > >> > > >>>> >> >> >> > kafka-topics.sh (if there are any). For
> example,
> > > you
> > > > >> will
> > > > >> > > >>>> probably
> > > > >> > > >>>> >> >> need
> > > > >> > > >>>> >> >> >> > changes to TopicDescription.java. You will
> also
> > > > need to
> > > > >> > > >>>> provide
> > > > >> > > >>>> >> all of
> > > > >> > > >>>> >> >> >> > the things that admin client needs -- for
> > example,
> > > > >> > > >>>> >> >> >> > TopicAuthorizedOperations.
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> > I also don't think the controller should serve
> > > this
> > > > >> > > request.
> > > > >> > > >>>> We
> > > > >> > > >>>> >> want
> > > > >> > > >>>> >> >> to
> > > > >> > > >>>> >> >> >> > minimize load on the controller. Just like
> with
> > > the
> > > > >> other
> > > > >> > > >>>> metadata
> > > > >> > > >>>> >> >> >> > requests like MetadataRequest, this should be
> > > > served by
> > > > >> > > >>>> brokers.
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> > It's a bit confusing why both topic ID and
> topic
> > > > name
> > > > >> are
> > > > >> > > >>>> provided
> > > > >> > > >>>> >> to
> > > > >> > > >>>> >> >> >> > this API. Is the intention that callers should
> > set
> > > > one
> > > > >> > but
> > > > >> > > >>>> not the
> > > > >> > > >>>> >> >> >> > other? Or both? This needs to be clarified.
> > Also,
> > > > if we
> > > > >> > do
> > > > >> > > >>>> want to
> > > > >> > > >>>> >> >> >> > support lookups by UUID, that is another thing
> > > that
> > > > >> needs
> > > > >> > > to
> > > > >> > > >>>> be
> > > > >> > > >>>> >> added
> > > > >> > > >>>> >> >> >> > to adminclient.
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> > In general, I feel like this should also
> > probably
> > > be
> > > > >> its
> > > > >> > > own
> > > > >> > > >>>> KIP
> > > > >> > > >>>> >> since
> > > > >> > > >>>> >> >> >> > it's fairly complex
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> > best,
> > > > >> > > >>>> >> >> >> > Colin
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> >
> > > > >> > > >>>> >> >> >> > On Thu, Aug 10, 2023, at 15:46, Calvin Liu
> > wrote:
> > > > >> > > >>>> >> >> >> >> Hi everyone,
> > > > >> > > >>>> >> >> >> >> I'd like to discuss a series of enhancement
> to
> > > the
> > > > >> > > >>>> replication
> > > > >> > > >>>> >> >> protocol.
> > > > >> > > >>>> >> >> >> >>
> > > > >> > > >>>> >> >> >> >> A partition replica can experience local data
> > > loss
> > > > in
> > > > >> > > unclean
> > > > >> > > >>>> >> >> shutdown
> > > > >> > > >>>> >> >> >> >> scenarios where unflushed data in the OS page
> > > > cache is
> > > > >> > > lost
> > > > >> > > >>>> - such
> > > > >> > > >>>> >> >> as an
> > > > >> > > >>>> >> >> >> >> availability zone power outage or a server
> > error.
> > > > The
> > > > >> > > Kafka
> > > > >> > > >>>> >> >> replication
> > > > >> > > >>>> >> >> >> >> protocol is designed to handle these
> situations
> > > by
> > > > >> > > removing
> > > > >> > > >>>> such
> > > > >> > > >>>> >> >> >> replicas
> > > > >> > > >>>> >> >> >> >> from the ISR and only re-adding them once
> they
> > > have
> > > > >> > caught
> > > > >> > > >>>> up and
> > > > >> > > >>>> >> >> >> therefore
> > > > >> > > >>>> >> >> >> >> recovered any lost data. This prevents
> replicas
> > > > that
> > > > >> > lost
> > > > >> > > an
> > > > >> > > >>>> >> >> arbitrary
> > > > >> > > >>>> >> >> >> log
> > > > >> > > >>>> >> >> >> >> suffix, which included committed data, from
> > being
> > > > >> > elected
> > > > >> > > >>>> leader.
> > > > >> > > >>>> >> >> >> >> However, there is a "last replica standing"
> > state
> > > > >> which
> > > > >> > > when
> > > > >> > > >>>> >> combined
> > > > >> > > >>>> >> >> >> with
> > > > >> > > >>>> >> >> >> >> a data loss unclean shutdown event can turn a
> > > local
> > > > >> data
> > > > >> > > loss
> > > > >> > > >>>> >> >> scenario
> > > > >> > > >>>> >> >> >> into
> > > > >> > > >>>> >> >> >> >> a global data loss scenario, i.e., committed
> > data
> > > > can
> > > > >> be
> > > > >> > > >>>> removed
> > > > >> > > >>>> >> from
> > > > >> > > >>>> >> >> >> all
> > > > >> > > >>>> >> >> >> >> replicas. When the last replica in the ISR
> > > > experiences
> > > > >> > an
> > > > >> > > >>>> unclean
> > > > >> > > >>>> >> >> >> shutdown
> > > > >> > > >>>> >> >> >> >> and loses committed data, it will be
> reelected
> > > > leader
> > > > >> > > after
> > > > >> > > >>>> >> starting
> > > > >> > > >>>> >> >> up
> > > > >> > > >>>> >> >> >> >> again, causing rejoining followers to
> truncate
> > > > their
> > > > >> > logs
> > > > >> > > and
> > > > >> > > >>>> >> thereby
> > > > >> > > >>>> >> >> >> >> removing the last copies of the committed
> > records
> > > > >> which
> > > > >> > > the
> > > > >> > > >>>> leader
> > > > >> > > >>>> >> >> lost
> > > > >> > > >>>> >> >> >> >> initially.
> > > > >> > > >>>> >> >> >> >>
> > > > >> > > >>>> >> >> >> >> The new KIP will maximize the protection and
> > > > provides
> > > > >> > > >>>> MinISR-1
> > > > >> > > >>>> >> >> >> tolerance to
> > > > >> > > >>>> >> >> >> >> data loss unclean shutdown events.
> > > > >> > > >>>> >> >> >> >>
> > > > >> > > >>>> >> >> >> >>
> > > > >> > > >>>> >> >> >>
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >>
> > > > >> > > >>>>
> > > > >> > >
> > > > >> >
> > > > >>
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-966%3A+Eligible+Leader+Replicas
> > > > >> > > >>>> >> >> >>
> > > > >> > > >>>> >> >>
> > > > >> > > >>>> >>
> > > > >> > > >>>>
> > > > >> > > >>>
> > > > >> > >
> > > > >> >
> > > > >>
> > > >
> > >
> >
>
