Hi Chia-Ping,
chia_00:
I'll update the KIP, thanks!
chia_01:
Regarding "partition(TopicPartition topicPartition)", it should return an
Optional<PartitionMetadata>. For the others, we should either return an empty
map (e.g., partitionsForTopic(String topic)) or an empty list (e.g.,
offlineReplicas(PartitionMetadata)).
I'll update the Javadoc as well — thanks for the reminder!
chia_02:
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.
Thank you!
Best,
Kuan-Po Tseng
On 2025/04/29 11:20:30 Chia-Ping Tsai wrote:
> hi Kuan-Po
>
> Thanks for updating KIP. This approach is better. Some comments are listed
> below.
>
> chia_00:
>
> Could you please update docs of `ClientQuotaCallback` to remind users to
> use ClientQuotaCallbackHandler instead.
>
> chia_01:
>
> what happens if users query a non-existent object? throw exception? or use
> `Optional` instead like "Optional<BrokerMetadata> broker(int nodeId);"
>
> chia_02:
>
> Do we have similar structure already to implement the "Map<TopicPartition,
> PartitionMetadata> partitionsForNode(int nodeId);"? If not, it seems we
> need to iterate all partitions to generate the collection?
>
> Best,
> Chia-Ping
>
>
>
>
>
> Kuan Po Tseng <brandb...@apache.org> 於 2025年4月29日 週二 下午6:44寫道：
>
> > 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
> > > > > >
> > > > >
> > > >
> > >
> >
>
