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
> > > >
> > >
> >
>
