MINOR: Update GroupCoordinator interface to received MetadataImage/Delta directly
#21008
Conversation
| * This is initialised when the {@link GroupCoordinator#onMetadataUpdate(MetadataDelta, MetadataImage)} is called | ||
| */ | ||
| private CoordinatorMetadataImage metadataImage = null; | ||
| private volatile CoordinatorMetadataImage metadataImage = null; |
There was a problem hiding this comment.
This is a bug. The attribute must be volatile because it is accessed from multiple threads.
| var wrappedImage = newImage == null ? null : new KRaftCoordinatorMetadataImage(newImage); | ||
| var wrappedDelta = delta == null ? null : new KRaftCoordinatorMetadataDelta(delta); |
There was a problem hiding this comment.
This is pretty ugly and it should not be here because the image and the delta can never be null. Unfortunately, we have a couple of tests passing nulls here. I will fix those separately and remove this code in a follow-up. I want the core change to be well scoped.
| metadataImage = new MetadataImageBuilder() | ||
| .addTopic(TOPIC_ID, TOPIC_NAME, 1) | ||
| .build(); |
There was a problem hiding this comment.
This change is semi-related. It is just easier to build the image rather than mocking it.
dajac
changed the title
GroupCoordinator interface to received MetadataImage/Delta directly
squah-confluent
left a comment
squah-confluent
left a comment
There was a problem hiding this comment.
Refactor looks good to me
lianetm
left a comment
lianetm
left a comment
There was a problem hiding this comment.
Nice refactoring, makes sense to me. Just a nit.
| var wrappedImage = newImage == null ? null : new KRaftCoordinatorMetadataImage(newImage); | ||
| var wrappedDelta = delta == null ? null : new KRaftCoordinatorMetadataDelta(delta); | ||
| metadataImage = wrappedImage; | ||
| runtime.onNewMetadataImage(wrappedImage, wrappedDelta); |
There was a problem hiding this comment.
do we want to rename this on the CoordinatorRuntime to onNewMetadataUpdate for consistency?
There was a problem hiding this comment.
Our parameter order isn't consistent. We have one onNewMetadataUpdate definition take (delta, image) and the other take (image, delta).
3 participants
In #20061, we introduced the
CoordinatorMetadataImageandCoordinatorMetadataDeltainterfaces tobasically contain/control how metadata is used within the
GroupCoordinatorService, more precisely within theGroupCoordinatorShard. When we did the change, we directly changed theGroupCoordinatorinterface to take implementation of those interfaces,requiring to wrap the
MetadataImageand theMetadataDeltain theBrokerMetadataPublisher. While looking at it now, I find this not ideafor a couple a reasons:
BrokerMetadataPublisherperspective, propagating metadatashould be standardized. Basically, all the internal components should
work the same from his point of view. If a component wants to be more
restrictive within his scope, it is fine but the publisher should not be
aware of this.
GroupCoordinatorperspective, requiringCoordinatorMetadataImageandCoordinatorMetadataDeltalimits what wecan do. For instance, we could move more functionality such as electing
shards from the
BrokerMetadataPublisherto theGroupCoordinatorServiceto further simplify the metadata publishingand improving the encapsulation of the components.
ShareCoordinatorperspective, the abstraction failed shortas we require
CoordinatorMetadataImageandCoordinatorMetadataDeltain the interface but we still require
FeaturesImageas well. Theabstraction fails short here.
This patch is an attempt to change this by moving back to requiring
MetadataImageandMetadataDeltain theGroupCoordinatorinterfaceand to wrap them within the service itself. It does not change the
ShareCoordinatoryet. I will do this as a follow-up if people agreewith the general approach.
Reviewers: Sean Quah squah@confluent.io, Lianet Magrans
lmagrans@confluent.io