Thanks for raising this here, Calvin. Since this is the first "streaming
results" type API in KafkaAdminClient (as far as I know), we're treading
new ground here.
As you mentioned, we can either accept a consumer or return some iterable
result. Returning a java.util.Stream is also an option, and a bit more
modern/convenient than java.util.Iterator. Personally, I like the consumer
approach, but I'm interested in hearing other's opinions.
This actually brings up another question: Do we think it's safe to assume
that one topic's description can fit into memory? The RPC supports paging
across partitions within a single topic, so maybe the admin API should as
well?
-David
On Fri, Feb 23, 2024 at 12:22 PM Calvin Liu <ca...@confluent.io> wrote:
> Hey,
> As we agreed to implement the pagination for the new API
> DescribeTopicPartitions, the client side must also add a proper interface
> to handle the pagination.
> The current KafkaAdminClient.describeTopics returns
> the DescribeTopicsResult which is the future for querying all the topics.
> It is awkward to fit the pagination into it because
>
> 1. Each future corresponds to a topic. We also want to have the
> pagination on huge topics for their partitions.
> 2. To avoid OOM, we should only fetch the new topics when we need them
> and release the used topics. Especially the main use case of looping the
> topic list is when the client prints all the topics.
>
> So, to better serve the pagination, @David Arthur
> <david.art...@confluent.io> suggested to add a new interface in the Admin
> client between the following 2.
>
> describeTopics(TopicCollection topics, DescribeTopicsOptions options,
> Consumer<TopicDescription>);
>
> Iterator<TopicDescription> describeTopics(TopicCollection topics,
> DescribeTopicsOptions options);
>
> David and I would prefer the first Consumer version which works better as a
> stream purposes.
>
>
> On Wed, Oct 11, 2023 at 4:28 PM Calvin Liu <ca...@confluent.io> wrote:
>
>> Hi David,
>> Thanks for the comment.
>> Yes, we can separate the ELR enablement from the metadata version. It is
>> also helpful to avoid blocking the following MV releases if the user is not
>> ready for ELR.
>> One thing to correct is that, the Unclean recovery is controlled
>> by unclean.recovery.manager.enabled, a separate config
>> from unclean.recovery.strategy. It determines whether unclean recovery will
>> be used in an unclean leader election.
>> Thanks
>>
>> On Wed, Oct 11, 2023 at 4:11 PM David Arthur <mum...@gmail.com> wrote:
>>
>>> One thing we should consider is a static config to totally enable/disable
>>> the ELR feature. If I understand the KIP correctly, we can effectively
>>> disable the unclean recovery by setting the recovery strategy config to
>>> "none".
>>>
>>> This would make development and rollout of this feature a bit smoother.
>>> Consider the case that we find bugs in ELR after a cluster has updated to
>>> its MetadataVersion. It's simpler to disable the feature through config
>>> rather than going through a MetadataVersion downgrade (once that's
>>> supported).
>>>
>>> Does that make sense?
>>>
>>> -David
>>>
>>> On Wed, Oct 11, 2023 at 1:40 PM Calvin Liu <ca...@confluent.io.invalid>
>>> wrote:
>>>
>>> > Hi Jun
>>> > -Good catch, yes, we don't need the -1 in the DescribeTopicRequest.
>>> > -No new value is added. The LeaderRecoveryState will still be set to 1
>>> if
>>> > we have an unclean leader election. The unclean leader election
>>> includes
>>> > the old random way and the unclean recovery. During the unclean
>>> recovery,
>>> > the LeaderRecoveryState will not change until the controller decides to
>>> > update the records with the new leader.
>>> > Thanks
>>> >
>>> > On Wed, Oct 11, 2023 at 9:02 AM Jun Rao <j...@confluent.io.invalid>
>>> wrote:
>>> >
>>> > > Hi, Calvin,
>>> > >
>>> > > Another thing. Currently, when there is an unclean leader election,
>>> we
>>> > set
>>> > > the LeaderRecoveryState in PartitionRecord and PartitionChangeRecord
>>> to
>>> > 1.
>>> > > With the KIP, will there be new values for LeaderRecoveryState? If
>>> not,
>>> > > when will LeaderRecoveryState be set to 1?
>>> > >
>>> > > Thanks,
>>> > >
>>> > > Jun
>>> > >
>>> > > On Tue, Oct 10, 2023 at 4:24 PM Jun Rao <j...@confluent.io> wrote:
>>> > >
>>> > > > Hi, Calvin,
>>> > > >
>>> > > > One more comment.
>>> > > >
>>> > > > "The first partition to fetch details for. -1 means to fetch all
>>> > > > partitions." It seems that FirstPartitionId of 0 naturally means
>>> > fetching
>>> > > > all partitions?
>>> > > >
>>> > > > Thanks,
>>> > > >
>>> > > > Jun
>>> > > >
>>> > > > On Tue, Oct 10, 2023 at 12:40 PM Calvin Liu
>>> <ca...@confluent.io.invalid
>>> > >
>>> > > > wrote:
>>> > > >
>>> > > >> Hi Jun,
>>> > > >> Yeah, with the current Metadata request handling, we only return
>>> > errors
>>> > > on
>>> > > >> the Topic level, like topic not found. It seems that querying a
>>> > specific
>>> > > >> partition is not a valid use case. Will update.
>>> > > >> Thanks
>>> > > >>
>>> > > >> On Tue, Oct 10, 2023 at 11:55 AM Jun Rao <j...@confluent.io.invalid
>>> >
>>> > > >> wrote:
>>> > > >>
>>> > > >> > Hi, Calvin,
>>> > > >> >
>>> > > >> > 60. If the range query has errors for some of the partitions,
>>> do we
>>> > > >> expect
>>> > > >> > different responses when querying particular partitions?
>>> > > >> >
>>> > > >> > Thanks,
>>> > > >> >
>>> > > >> > Jun
>>> > > >> >
>>> > > >> > On Tue, Oct 10, 2023 at 10:50 AM Calvin Liu
>>> > > <ca...@confluent.io.invalid
>>> > > >> >
>>> > > >> > wrote:
>>> > > >> >
>>> > > >> > > Hi Jun
>>> > > >> > > 60. Yes, it is a good question. I was thinking the API could
>>> be
>>> > > >> flexible
>>> > > >> > to
>>> > > >> > > query the particular partitions if the range query has errors
>>> for
>>> > > >> some of
>>> > > >> > > the partitions. Not sure whether it is a valid assumption,
>>> what do
>>> > > you
>>> > > >> > > think?
>>> > > >> > >
>>> > > >> > > 61. Good point, I will update them to partition level with the
>>> > same
>>> > > >> > limit.
>>> > > >> > >
>>> > > >> > > 62. Sure, will do.
>>> > > >> > >
>>> > > >> > > Thanks
>>> > > >> > >
>>> > > >> > > On Tue, Oct 10, 2023 at 10:12 AM Jun Rao
>>> <j...@confluent.io.invalid
>>> > >
>>> > > >> > wrote:
>>> > > >> > >
>>> > > >> > > > Hi, Calvin,
>>> > > >> > > >
>>> > > >> > > > A few more minor comments on your latest update.
>>> > > >> > > >
>>> > > >> > > > 60. DescribeTopicRequest: When will the Partitions field be
>>> > used?
>>> > > It
>>> > > >> > > seems
>>> > > >> > > > that the FirstPartitionId field is enough for AdminClient
>>> usage.
>>> > > >> > > >
>>> > > >> > > > 61. Could we make the limit for DescribeTopicRequest,
>>> > > >> > > ElectLeadersRequest,
>>> > > >> > > > GetReplicaLogInfo consistent? Currently,
>>> ElectLeadersRequest's
>>> > > >> limit is
>>> > > >> > > at
>>> > > >> > > > topic level and GetReplicaLogInfo has a different partition
>>> > level
>>> > > >> limit
>>> > > >> > > > from DescribeTopicRequest.
>>> > > >> > > >
>>> > > >> > > > 62. Should ElectLeadersRequest.DesiredLeaders be at the same
>>> > level
>>> > > >> as
>>> > > >> > > > ElectLeadersRequest.TopicPartitions.Partitions? In the KIP,
>>> it
>>> > > looks
>>> > > >> > like
>>> > > >> > > > it's at the same level as
>>> ElectLeadersRequest.TopicPartitions.
>>> > > >> > > >
>>> > > >> > > > Thanks,
>>> > > >> > > >
>>> > > >> > > > Jun
>>> > > >> > > >
>>> > > >> > > > On Wed, Oct 4, 2023 at 3:55 PM Calvin Liu
>>> > > >> <ca...@confluent.io.invalid>
>>> > > >> > > > wrote:
>>> > > >> > > >
>>> > > >> > > > > Hi David,
>>> > > >> > > > > Thanks for the comments.
>>> > > >> > > > > ----
>>> > > >> > > > > I thought that a new snapshot with the downgraded MV is
>>> > created
>>> > > in
>>> > > >> > this
>>> > > >> > > > > case. Isn’t it the case?
>>> > > >> > > > > Yes, you are right, a metadata delta will be generated
>>> after
>>> > the
>>> > > >> MV
>>> > > >> > > > > downgrade. Then the user can start the software downgrade.
>>> > > >> > > > > -----
>>> > > >> > > > > Could you also elaborate a bit more on the reasoning
>>> behind
>>> > > adding
>>> > > >> > the
>>> > > >> > > > > limits to the admin RPCs? This is a new pattern in Kafka
>>> so it
>>> > > >> would
>>> > > >> > be
>>> > > >> > > > > good to clear on the motivation.
>>> > > >> > > > > Thanks to Colin for bringing it up. The current
>>> > MetadataRequest
>>> > > >> does
>>> > > >> > > not
>>> > > >> > > > > have a limit on the number of topics to query in a single
>>> > > request.
>>> > > >> > > > Massive
>>> > > >> > > > > requests can mess up the JVM. We want to have some sort of
>>> > > >> throttle
>>> > > >> > on
>>> > > >> > > > the
>>> > > >> > > > > new APIs.
>>> > > >> > > > > -----
>>> > > >> > > > > Could you also explain how the client is supposed to
>>> handle
>>> > the
>>> > > >> > > > > topics/partitions above the limit? I suppose that it will
>>> have
>>> > > to
>>> > > >> > retry
>>> > > >> > > > > those, correct?
>>> > > >> > > > > Corrent. For the official admin clients, it will split the
>>> > large
>>> > > >> > > request
>>> > > >> > > > > into proper pieces and query one after another.
>>> > > >> > > > > -----
>>> > > >> > > > > My understanding is that the topics/partitions above the
>>> limit
>>> > > >> will
>>> > > >> > be
>>> > > >> > > > > failed with an invalid exception error. I wonder if this
>>> > choice
>>> > > is
>>> > > >> > > > > judicious because the invalide request exception is
>>> usually
>>> > > >> fatal. It
>>> > > >> > > may
>>> > > >> > > > > be better to use an new and explicit error for this case.
>>> > > >> > > > >
>>> > > >> > > > > Thanks for bringing this up. How about
>>> > "REQUEST_LIMIT_REACHED"?
>>> > > >> > > > > --------
>>> > > >> > > > > It seems that we still need to specify the changes to the
>>> > admin
>>> > > >> api
>>> > > >> > to
>>> > > >> > > > > accommodate the new or updated apis. Do you plan to add
>>> them?
>>> > > >> > > > > Try to cover the following
>>> > > >> > > > > 1. The admin client will use the new DescribeTopicRequest
>>> to
>>> > > query
>>> > > >> > the
>>> > > >> > > > > topics
>>> > > >> > > > > 2. Mention the API limit and the new retriable error.
>>> > > >> > > > > 3. Output changes for the admin client when describing a
>>> topic
>>> > > >> (new
>>> > > >> > > > fields
>>> > > >> > > > > of ELR...)
>>> > > >> > > > > 4. Changes to data structures like TopicPartitionInfo to
>>> > include
>>> > > >> the
>>> > > >> > > ELR.
>>> > > >> > > > > Anything else I missed?
>>> > > >> > > > >
>>> > > >> > > > > Thanks!
>>> > > >> > > > >
>>> > > >> > > > >
>>> > > >> > > > >
>>> > > >> > > > >
>>> > > >> > > > >
>>> > > >> > > > > On Wed, Oct 4, 2023 at 12:27 PM David Jacot <
>>> > > >> david.ja...@gmail.com>
>>> > > >> > > > wrote:
>>> > > >> > > > >
>>> > > >> > > > > > Hi Calvin,
>>> > > >> > > > > >
>>> > > >> > > > > > I thought that a new snapshot with the downgraded MV is
>>> > > created
>>> > > >> in
>>> > > >> > > this
>>> > > >> > > > > > case. Isn’t it the case?
>>> > > >> > > > > >
>>> > > >> > > > > > Could you also elaborate a bit more on the reasoning
>>> behind
>>> > > >> adding
>>> > > >> > > the
>>> > > >> > > > > > limits to the admin RPCs? This is a new pattern in
>>> Kafka so
>>> > it
>>> > > >> > would
>>> > > >> > > be
>>> > > >> > > > > > good to clear on the motivation.
>>> > > >> > > > > >
>>> > > >> > > > > > Could you also explain how the client is supposed to
>>> handle
>>> > > the
>>> > > >> > > > > > topics/partitions above the limit? I suppose that it
>>> will
>>> > have
>>> > > >> to
>>> > > >> > > retry
>>> > > >> > > > > > those, correct?
>>> > > >> > > > > >
>>> > > >> > > > > > My understanding is that the topics/partitions above the
>>> > limit
>>> > > >> will
>>> > > >> > > be
>>> > > >> > > > > > failed with an invalid exception error. I wonder if this
>>> > > choice
>>> > > >> is
>>> > > >> > > > > > judicious because the invalide request exception is
>>> usually
>>> > > >> fatal.
>>> > > >> > It
>>> > > >> > > > may
>>> > > >> > > > > > be better to use an new and explicit error for this
>>> case.
>>> > > >> > > > > >
>>> > > >> > > > > > It seems that we still need to specify the changes to
>>> the
>>> > > admin
>>> > > >> api
>>> > > >> > > to
>>> > > >> > > > > > accommodate the new or updated apis. Do you plan to add
>>> > them?
>>> > > >> > > > > >
>>> > > >> > > > > > Best,
>>> > > >> > > > > > David
>>> > > >> > > > > >
>>> > > >> > > > > > Le mer. 4 oct. 2023 à 20:39, Calvin Liu
>>> > > >> <ca...@confluent.io.invalid
>>> > > >> > >
>>> > > >> > > a
>>> > > >> > > > > > écrit :
>>> > > >> > > > > >
>>> > > >> > > > > > > Hi Jun,
>>> > > >> > > > > > > After the MV downgrade, the controller will write in
>>> the
>>> > old
>>> > > >> > > version
>>> > > >> > > > of
>>> > > >> > > > > > the
>>> > > >> > > > > > > PartitionRecord/PartitionChangeRecord. If I understand
>>> > > >> correctly,
>>> > > >> > > it
>>> > > >> > > > is
>>> > > >> > > > > > > possible to downgrade the software version if the
>>> > controller
>>> > > >> only
>>> > > >> > > has
>>> > > >> > > > > to
>>> > > >> > > > > > > handle old version records.
>>> > > >> > > > > > > However, the controller will not automatically
>>> rewrite the
>>> > > >> > > > > > PartitionRecord
>>> > > >> > > > > > > with the old version unless there is a partition
>>> update.
>>> > > Then,
>>> > > >> > the
>>> > > >> > > > user
>>> > > >> > > > > > may
>>> > > >> > > > > > > have to wait an unknown amount of time before the
>>> software
>>> > > >> > > downgrades
>>> > > >> > > > > > > unless they do a roll to force update every
>>> partition. If
>>> > it
>>> > > >> > makes
>>> > > >> > > > > > sense, I
>>> > > >> > > > > > > can mention these steps to do a software downgrade.
>>> > > >> > > > > > > Thanks
>>> > > >> > > > > > >
>>> > > >> > > > > > > On Wed, Oct 4, 2023 at 11:20 AM Jun Rao
>>> > > >> <j...@confluent.io.invalid
>>> > > >> > >
>>> > > >> > > > > > wrote:
>>> > > >> > > > > > >
>>> > > >> > > > > > > > Hi, Calvin and Justine,
>>> > > >> > > > > > > >
>>> > > >> > > > > > > > Historically, when we change the record format in
>>> the
>>> > log,
>>> > > >> we
>>> > > >> > > don't
>>> > > >> > > > > > > support
>>> > > >> > > > > > > > software version downgrading.
>>> > > >> > > > > > > >
>>> > > >> > > > > > > > For the record format change in the metadata log,
>>> have
>>> > we
>>> > > >> > thought
>>> > > >> > > > > about
>>> > > >> > > > > > > > forcing the write of the latest metadata records
>>> with
>>> > the
>>> > > >> old
>>> > > >> > > > version
>>> > > >> > > > > > > > during MV downgrading? This will in theory allow
>>> the old
>>> > > >> > version
>>> > > >> > > of
>>> > > >> > > > > the
>>> > > >> > > > > > > > software to obtain the latest metadata.
>>> > > >> > > > > > > >
>>> > > >> > > > > > > > Thanks,
>>> > > >> > > > > > > >
>>> > > >> > > > > > > > Jun
>>> > > >> > > > > > > >
>>> > > >> > > > > > > > On Wed, Oct 4, 2023 at 9:53 AM Justine Olshan
>>> > > >> > > > > > > <jols...@confluent.io.invalid
>>> > > >> > > > > > > > >
>>> > > >> > > > > > > > wrote:
>>> > > >> > > > > > > >
>>> > > >> > > > > > > > > Sorry -- not MV but software version.
>>> > > >> > > > > > > > >
>>> > > >> > > > > > > > > On Wed, Oct 4, 2023 at 9:51 AM Justine Olshan <
>>> > > >> > > > > jols...@confluent.io>
>>> > > >> > > > > > > > > wrote:
>>> > > >> > > > > > > > >
>>> > > >> > > > > > > > > > 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
>>> > > >> > > > > > > > > >> > > > >> > > >>>> >> >> >> > on
>>> > > >> > > > > >
>>> > > >> > > > >
>>> > > >> > > >
>>> > > >> > >
>>> > > >> >
>>> > > >>
>>> > > >
>>> > >
>>> >
>>>
>>>
>>> --
>>> David Arthur
>>>
>>
--
-David
