Hi Tom,
I haven taken a look to the updated KIP, some thoughts :
* in the "Public Interfaces" section you wrote
alterTopics(Set<AlterTopics>) but then a collection is used (instead of a set)
in the Proposed Changes section. I'm ok with collection.
* in the summary of the alterTopics method you say "The request can change
the number of partitions, replication factor and/or the partition assignments."
I think that the "and/or" is misleading (at least for me). For the TopicCommand
tool you can specify partitions AND replication factor ... OR partition
assignments (but not for example partitions AND replication factor AND
partition assignments). Maybe it could be "The request can change the number of
partitions and the related replication factor or specifying a partition
assignments."
* I know that it would be a breaking change in the Admin Client API but why
having an AlteredTopic class which is quite similar to the already existing
NewTopic class ? I know that using NewTopic for the alter method could be
misleading. What about renaming NewTopic in something like AdminTopic ? At same
time it means that the TopicDetails class (which you can get from the current
NewTopic) should be outside the CreateTopicsRequest because it could be used in
the AlterTopicsRequest as well.
* A typo in the ReplicaStatus : gpartition() instead of partition()
* In the AlterTopicRequets
* the replication factor should be INT16
* I would use same fields name as CreateTopicsRequest (they are quite
similar)
* What's the broker id in the ReplicaStatusRequest ?
* Thinking aloud, could make sense having "Partition" in the
ReplicaStatusRequest as an array so that I can specify in only one request the
status for partitions I'm interested in, in order to avoid e request for each
partition for the same topic. Maybe empty array could mean .. "give me status
for all partitions of this topic". Of course it means that the
ReplicaStatusResponse should change because we should have an array with
partition, broker, lag and so on
Thanks,
Paolo
Paolo Patierno
Senior Software Engineer (IoT) @ Red Hat
Microsoft MVP on Windows Embedded & IoT
Microsoft Azure Advisor
Twitter : @ppatierno<http://twitter.com/ppatierno>
Linkedin : paolopatierno<http://it.linkedin.com/in/paolopatierno>
Blog : DevExperience<http://paolopatierno.wordpress.com/>
________________________________
From: Tom Bentley <t.j.bent...@gmail.com>
Sent: Tuesday, July 25, 2017 11:07 AM
To: dev@kafka.apache.org
Subject: Re: [DISCUSS] KIP-179: Change ReassignPartitionsCommand to use
AdminClient
Hi Ismael and Paolo,
I've updated my KIP [1] to describe an alterTopics() API which would work
for kafka-reassign-partitions.sh. It's still a bit rough, but should be a
good basis for a KIP to cover both tools.
As a first step, if Paolo could review this and check it's compatible with
what he needs for the kafka-topics.sh tool that would be great. Then we can
add to describe his changes to the rest of the kafka-topics.sh tool,
assuming Paolo is happy with this arrangement, Paolo?
Cheers,
Tom
[1]
https://cwiki.apache.org/confluence/display/KAFKA/KIP-179+-+Change+ReassignPartitionsCommand+to+use+AdminClient
On 25 July 2017 at 11:38, Ismael Juma <ism...@juma.me.uk> wrote:
> Hi Tom and Paolo,
>
> It's true that increasing the number of partitions is done via the
> kafka-topics tool, which is also being converted to use the AdminClient
> (but via a different JIRA). I also agree that it would be good to consider
> if alterTopics would be a sensible way to support all the use cases or if
> it's better to have separate APIs. I think it makes sense to have a single
> KIP though as they are related and it will be easier to evaluate as a
> whole.
>
> Does that make sense?
>
> Ismael
>
> On Tue, Jul 25, 2017 at 10:16 AM, Paolo Patierno <ppatie...@live.com>
> wrote:
>
> > Hi,
> >
> >
> > I was digging into it because I need something like an Admin Client alter
> > API for my work on rewriting the TopicCommand tool using them.
> >
> > The AlterConfigs API is used for changing topic level configuration (i.e.
> > retention.ms, retention.bytes and so on).
> >
> > A new AlterTopic API could be better in order to change topic "high
> level"
> > structure so number of partitions, replication factors and so on.
> >
> > My opinion is that we need separate API because from my point of view
> they
> > are different settings.
> >
> >
> > Thanks,
> >
> > Paolo.
> >
> >
> > Paolo Patierno
> > Senior Software Engineer (IoT) @ Red Hat
> > Microsoft MVP on Windows Embedded & IoT
> > Microsoft Azure Advisor
> >
> > Twitter : @ppatierno<http://twitter.com/ppatierno>
> > Linkedin : paolopatierno<http://it.linkedin.com/in/paolopatierno>
> > Blog : DevExperience<http://paolopatierno.wordpress.com/>
> >
> >
> > ________________________________
> > From: Tom Bentley <t.j.bent...@gmail.com>
> > Sent: Tuesday, July 25, 2017 9:02 AM
> > To: dev@kafka.apache.org
> > Subject: Re: [DISCUSS] KIP-179: Change ReassignPartitionsCommand to use
> > AdminClient
> >
> > Hi Ismael,
> >
> > I assume that's the --partitions for kafka-topics.sh? I must admit I
> hadn't
> > considered that tool, only kafka-reassign-partitions.sh. Thanks for
> > pointing it out, because obviously the AdminClient API needs to be
> suitable
> > for reuse in kafka-topics.sh too. Since AdminClient doesn't currently
> > support an alterTopics() I guess the API should mirror the API for
> > newTopics(), so something like:
> >
> > public AlterTopicsResult alterTopics(Set<AlteredTopic> topics);
> > // where:
> > public class AlteredTopic {
> > // constructor etc
> > public String name();
> > public int numPartitions();
> > public int replicationFactor();
> > Map<Integer,List<Integer>> replicasAssignment();
> > }
> >
> > Note that although NewTopic contains a `Map<String,String> configs`, I
> > think the API for changing a topic's config already exists:
> alterConfigs().
> >
> > This API is better than having separate methods to set the number of
> > partitions/replicas and assigning them to brokers, since sometimes people
> > will want to set the assignment at the same time as changing the
> > partitions/replicas.
> >
> > An API like this could then be used by both tools.
> >
> >
> >
> > On 24 July 2017 at 16:23, Ismael Juma <ism...@juma.me.uk> wrote:
> >
> > > Hi Tom,
> > >
> > > I think it makes sense to keep progress reporting simple in the initial
> > > version. As you say, time to completion is tricky to compute and it
> seems
> > > like it should be tackled as its own KIP.
> > >
> > > Regarding waiting for reassignment completion, it's easy enough for
> > people
> > > to do that via a script, so I don't think we need to add it to the
> tool.
> > >
> > > One question that occurred to me, the tool allows one to add partitions
> > to
> > > existing topics:
> > >
> > > val partitionsOpt = parser.accepts("partitions", "The number of
> > partitions
> > > for the topic being created or " +
> > > "altered (WARNING: If partitions are increased for a topic that
> > has a
> > > key, the partition logic or ordering of the messages will be affected")
> > >
> > > It seems like it may make sense to have that as an AdminClient API as
> > well.
> > > If we agree to do that, then we need to decide if it should be
> > implemented
> > > client-side or by adding a protocol API. The former is simpler, but the
> > > latter would allow non Java clients to use it without duplicating the
> > logic
> > > of assigning replicas to the new partitions. What are your thoughts?
> > >
> > > Ismael
> > >
> > > On Sat, Jul 22, 2017 at 10:14 AM, Tom Bentley <t.j.bent...@gmail.com>
> > > wrote:
> > >
> > > > Thinking about this some more, I release that the proposed API for
> > > tracking
> > > > progress is a bit specific to this reassignment use case. A more
> > > generally
> > > > useful API would be to be able to find out, for a partition on a
> > broker:
> > > >
> > > > * When the broker became a follower
> > > > * When the broker was last in the ISR
> > > > * If the broker is not in the ISR, how many messages behind it is
> > > >
> > > > That's not enough to be able to calculate a percentage completion of
> a
> > > > reassignment though (we don't know how far behind it was when it
> > became a
> > > > follower). But maybe we could maintain a maximum of how far behind it
> > has
> > > > fallen behind the ISR, since it became a follower/dropped out of the
> > ISR.
> > > >
> > > > btw, we need the middle bullet to cope with following sequence:
> > > >
> > > > 1. Start syncing
> > > > 2. Finish syncing
> > > > 3. Fall behind (drop out of ISR for some reason)
> > > > 4. User queries for if reassignment has finished (it has, but just
> > > looking
> > > > at the ISR would give the impression that it has not).
> > > >
> > > >
> > > >
> > > >
> > > > On 21 July 2017 at 11:09, Tom Bentley <t.j.bent...@gmail.com> wrote:
> > > >
> > > > > Aside: I've started this new DISCUSS thread for KIP-179 since the
> > > > original
> > > > > one had the incorrect KIP number 178. The original thread can be
> > found
> > > > > here: http://mail-archives.apache.org/mod_mbox/kafka-dev/201707.
> > > > > mbox/%3cCAMd5YszudP+-8z5KTbFh6JscT2p4xFi1=VZWWX+
> > > > > 5dccpxry...@mail.gmail.com%3e
> > > > >
> > > > > I've just updated KIP-179 to support Ismael's request for the
> command
> > > to
> > > > > be able to support progress reporting of an ongoing partition
> > > > reassignment.
> > > > >
> > > > > I'll call out two things which I'm not sure about since I don't yet
> > > have
> > > > > much experience of Kafka being used operationally:
> > > > >
> > > > > 1. As currently constructed the --progress option could report an
> > > overall
> > > > > progress percentage, per-partition percentages and errors. It
> cannot
> > > > > provide any kind of time-to-completion estimate. Part of me is
> loath
> > to
> > > > do
> > > > > this, as I'm sure we all remember file transfer dialogs that
> provide
> > > > > amusing/baffling time-to-completion estimates. So it might be hard
> to
> > > do
> > > > > _well_. On the other hand I expect the thing people will be
> > interested
> > > in
> > > > > will often be "when will it be finished?"
> > > > >
> > > > > 2. There is no option for the tool to wait for reassignment
> > > completion. I
> > > > > can imagine users might want to script something to happen after
> the
> > > > > reassignment is complete, and without some kind of --wait option
> they
> > > > will
> > > > > have to poll for completion "manually". Having a --wait optin and
> > > putting
> > > > > this polling in the tool means we have a lot more control over how
> > > often
> > > > > such polling happens.
> > > > >
> > > > > The KIP is available here:
> > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 179+-+Change+
> > > > > ReassignPartitionsCommand+to+use+AdminClient
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Tom
> > > > >
> > > >
> > >
> >
>
