MINOR: Refactor the MetadataPublisher interface#13337
Conversation
cmccabe
force-pushed
the
cmccabe_2023-03-03_refactor_metadatapublisher
branch
from
d06510f to
f563a51
Compare
This PR refactors MetadataPublisher's interface a bit. There is now a handleControllerChange callback. This is something that some publishers might want. A good example is ZkMigrationClient. There is now only a single publish() function rather than a separate function for publishing snapshots and log deltas. Most publishers didn't want to do anything different in those two cases. The ones that do want to do something different for snapshots can always check the manifest type. The close function now has a default empty implementation, since most publishers didn't need to do anything there.
|
rebase on trunk |
cmccabe
force-pushed
the
cmccabe_2023-03-03_refactor_metadatapublisher
branch
from
f563a51 to
3e96b22
Compare
There was a problem hiding this comment.
Should this have the override annotation?
| MetadataImage newImage, | ||
| SnapshotManifest manifest | ||
| ); | ||
| default void handleControllerChange(LeaderAndEpoch newLeaderAndEpoch) { } |
There was a problem hiding this comment.
nit: "publishControllerChange" to keep the naming consistent?
There was a problem hiding this comment.
How about
void onControllerChange(LeaderAndEpoch newLeaderAndEpoch)
and
void onMetadataUpdate(MetadataDelta delta, MetadataImage newImage, LoaderManifest manifest);?
Might be clearer.
| /** | ||
| * Contains information about what was loaded. | ||
| */ | ||
| public interface LoaderManifest { |
There was a problem hiding this comment.
Why can't we include the manifest type in the MetadataProvenance?
There was a problem hiding this comment.
We need a type to represent the 3-tuple of (offset, epoch, timestamp) since that comes up in many places. I agree that conceptually whether it came from a snapshot or a log is part of the "provenance", but I think it would be ugly in code terms to start putting stuff from the manifest into MetadataProvenance.
There was a problem hiding this comment.
Option[LoaderManifest] since it is optional?
cmccabe
force-pushed
the
cmccabe_2023-03-03_refactor_metadatapublisher
branch
from
da9a24d to
cd5d2fd
Compare
|
I fixed the case where I was invoking I also fixed it so that |
| /** | ||
| * Contains information about what was loaded. | ||
| */ | ||
| public interface LoaderManifest { |
| "change to " + currentLeaderAndEpoch + " with publisher " + | ||
| publisher.name(), e); |
| LoaderManifest manifest | ||
| ) { | ||
| enqueueMetadataChangeEvent(delta, | ||
| newImage, |
| publishSnapshot(delta, newImage, (SnapshotManifest) manifest); | ||
| break; | ||
| default: | ||
| break; |
There was a problem hiding this comment.
wdyt about throwing an IllegalStateException in the default. I do that sometimes to future-proof against unhandled enums down the road.
There was a problem hiding this comment.
Actually, let's just remove the default case. spotbugs will flag non-exhaustive switch statements on enums. And that's a hard build error for us.
| } | ||
| } | ||
| } | ||
| // APply SCRAM delta. |
|
Looks like compile failures in |
3 participants
This PR refactors MetadataPublisher's interface a bit. There is now a handleControllerChange callback. This is something that some publishers might want. A good example is ZkMigrationClient.
There is now only a single publish() function rather than a separate function for publishing snapshots and log deltas. Most publishers didn't want to do anything different in those two cases. The ones that do want to do something different for snapshots can always check the manifest type.
The close function now has a default empty implementation, since most publishers didn't need to do anything there.