Hi Chia-Ping,
That works too. But in that case, the original method name feels inappropriate.
So, as you suggested, I’ll add the topic parameter and rename the method from:
Map<TopicPartition, PartitionMetadata> partitionsForNode(int nodeId)
to:
Map<Integer, PartitionMetadata> partitions(String topic, int nodeId)
Since the caller already knows the topic name, there's no need to wrap the key
in a TopicPartition.
---
And, I’ll rename the other related method from:
Map<Integer, PartitionMetadata> partitionsForTopic(String topic)
to:
Map<Integer, PartitionMetadata> partitions(String topic)
The reason is this method is essentially the same as the one above, with the
only difference being the additional node ID condition.
Best,
Kuan-Po Tseng
On 2025/04/30 18:21:23 Chia-Ping Tsai wrote:
> hi Kuan-Po
>
> > In TopicImage, there is a similar method, Map<Integer,
> PartitionRegistration> partitions(), but we can't directly reuse its return
> value here. As you mentioned, we'll still need to iterate through all
> partitions to build the collection. That said, since this KIP proposes to
> make PartitionRegistration implements PartitionInfo, the cost of generating
> the collection should be low.
>
> I guess the implementation looks like this? If so, could you please add
> docs to remind users that the operation is a bit expensive.
>
> ```
> var result = new HashMap<TopicPartition, PartitionRegistration>();
> image.topics().topicsByName().forEach((topic, partitions) ->
> partitions.partitions().forEach((partition, registration) -> {
> if (Arrays.stream(registration.isr).anyMatch(i -> i == broker)) {
> result.put(new TopicPartition(topic, partition), registration);
> }
> }));
> return result;
> ```
>
> Alternatively, we can add `topic name` as one more query. It can eliminate
> one loop. WDYT?
>
> Best,
> Chia-Ping
>
>
>
> Colin McCabe <cmcc...@apache.org> 於 2025年5月1日 週四 上午12:46寫道：
>
> > Hi Kuan Po Tseng,
> >
> > Yes, the old interface extending the new one could work.
> >
> > I don't have a strong preference about this, as long as someone with a
> > correctly configured and written implementation of the NEW
> > ClientQuotaCallback interface can upgrade from 4.x to 5.0 without hitting
> > compatibility problems. That was my main concern.
> >
> > best,
> > Colin
> >
> > On Tue, Apr 29, 2025, at 03:44, Kuan Po Tseng wrote:
> > > Hi Colin,
> > >
> > > I was thinking about it some more, and I feel like it might be better
> > > to have the old interface extend the new one. That way, it’ll be easier
> > > for us to remove the old interface later — we wouldn’t have to change
> > > too much code, just delete the old one.
> > >
> > > It would still keep everything compatible too: once users switch over
> > > to the new interface, they won’t need to recompile after we remove the
> > > old interface.
> > >
> > > By the way, I’ve also updated the KIP based on this idea. Would be
> > > great if you could take a look when you have some time. Thanks a lot!
> > >
> > > Best,
> > > Kuan-Po Tseng
> > >
> > > On 2025/04/29 08:39:55 Kuan Po Tseng wrote:
> > >> Hi everyone,
> > >>
> > >> Colin:
> > >> You’re right. I’ll adjust the KIP as you suggested—creating a new
> > interface that extends the old one to avoid any compilation issues when we
> > eventually remove the old method.
> > >>
> > >> However, Chia-Ping also suggested another approach: creating a
> > completely new interface that has no relation to the old one. Of course,
> > this would require more changes to the codebase than what we have now, but
> > I’d like to hear your thoughts on this.
> > >>
> > >> Chia-Ping:
> > >> Thanks for pointing out the typo. I totally missed that, and I’ll make
> > sure to update it. I’ll also add the items we discussed to the "rejected
> > alternatives" section as you suggested.
> > >>
> > >> Thanks again!
> > >>
> > >> Best,
> > >> Kuan-Po Tseng
> > >>
> > >> On 2025/04/29 08:18:34 Chia-Ping Tsai wrote:
> > >> > hi Kuan-Po
> > >> >
> > >> > Could you please add the approaches we discussed before to "rejected"
> > section? It could help readers to understand the picture easily.
> > >> >
> > >> > Best,
> > >> > Chia-Ping
> > >> >
> > >> > On 2025/04/29 04:32:52 Chia-Ping Tsai wrote:
> > >> > > hi Colin
> > >> > >
> > >> > > the interface `ClientQuotaCallackHandler` is a typo - the idea of
> > this PR
> > >> > > is to add new `updateClusterMetadata` to replace/deprecate old
> > >> > > `updateClusterMetadata`. Additionally, the production code will
> > call new
> > >> > > `updateClusterMetadata` instead of old `updateClusterMetadata`.
> > That means
> > >> > > users who to want migrate to new `updateClusterMetadata` should
> > make old
> > >> > > `updateClusterMetadata` throw exception and implement new
> > >> > > `updateClusterMetadata`.
> > >> > > For example:
> > >> > >
> > >> > > ```
> > >> > > MyCallback implements ClientQuotaCallback {
> > >> > >
> > >> > > boolean updateClusterMetadata(Cluster cluster) {
> > >> > > throw new UnsupportException();
> > >> > > }
> > >> > >
> > >> > > override boolean updateClusterMetadata(ClusterMetadata
> > clusterMetadata)
> > >> > > {
> > >> > > // new implementation
> > >> > > }
> > >> > > }
> > >> > > ```
> > >> > >
> > >> > >
> > >> > > Should we instead have a new interface (which can inherit from the
> > old
> > >> > > interface, for now)?
> > >> > >
> > >> > > Yes, we had discussed this approach. We can dynamically create the
> > object
> > >> > > based on the new interface first, and then try the old interface
> > for users
> > >> > > who are still using it. Additionally, a callback implementing the
> > old
> > >> > > interface can be wrapped to conform to the new interface. However,
> > this
> > >> > > still exposes the "deprecated" method/structure to users, even as
> > they are
> > >> > > migrating to the new interface.
> > >> > >
> > >> > > A more direct approach is to have the new interface *not* extend
> > the old
> > >> > > one. This ensures that users will definitely not interact with the
> > old
> > >> > > structure when using the new interface. Of course, wrapping will
> > still be
> > >> > > necessary. This approach was used when migrating the public APIs of
> > tools
> > >> > > from Scala to Java (KIP-641).
> > >> > > Best,
> > >> > > Chia-Ping
> > >> > >
> > >> > >
> > >> > >
> > >> > >
> > >> > > Colin McCabe <cmcc...@apache.org> 於 2025年4月29日 週二 上午4:38寫道：
> > >> > >
> > >> > > > Hi Kuan Po Tseng,
> > >> > > >
> > >> > > > Thank you for starting this discussion. This would be a very nice
> > >> > > > improvement.
> > >> > > >
> > >> > > > It seems that the current scheme requires everyone writing an
> > >> > > > implementation of ClientQuotaCallackHandler to implement the old
> > >> > > > updateClusterMetadata function that takes a Cluster. That seems
> > less than
> > >> > > > ideal, since once we remove that deprecated function, it will
> > break
> > >> > > > compilation for anyone implementing it. Should we instead have a
> > new
> > >> > > > interface (which can inherit from the old interface, for now)?
> > That would
> > >> > > > avoid this issue...
> > >> > > >
> > >> > > > best,
> > >> > > > Colin
> > >> > > >
> > >> > > >
> > >> > > > On Tue, Apr 22, 2025, at 09:23, Kuan Po Tseng wrote:
> > >> > > > > Hello everyone,
> > >> > > > >
> > >> > > > > I’d like to kick off a discussion about redesigning
> > >> > > > > ClientQuotaCallback#updateClusterMetadata.
> > >> > > > > As it stands, the current implementation is quite inefficient,
> > since it
> > >> > > > > requires creating
> > >> > > > > a full Cluster object each time any change is made. On top of
> > that, some
> > >> > > > of
> > >> > > > > the information
> > >> > > > > within the Cluster object is outdated.
> > >> > > > >
> > >> > > > > With that in mind, I’d love to propose a lighter, more efficient
> > >> > > > solution,
> > >> > > > > along with a redefined
> > >> > > > > interface for the object passed into updateClusterMetadata. If
> > you have a
> > >> > > > > moment, I’d really appreciate your thoughts and feedback on
> > this.
> > >> > > > >
> > >> > > > > Thank you!
> > >> > > > >
> > >> > > > > KIP link: https://cwiki.apache.org/confluence/x/zInoF
> > >> > > > >
> > >> > > > > Best,
> > >> > > > > Kuan-Po Tseng
> > >> > > >
> > >> > >
> > >> >
> > >>
> >
>
