rahil-c commented on code in PR #19790:
URL: https://github.com/apache/hudi/pull/19790#discussion_r3897322125
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/metrics/DistributedRegistry.java:
##########
@@ -45,9 +45,27 @@ public String getName() {
return name;
}
- public void register(JavaSparkContext jsc) {
- if (!isRegistered()) {
+ public DistributedRegistry register(JavaSparkContext jsc) {
+ if (isRegistered()) {
+ return this;
+ }
+ try {
jsc.sc().register(this);
+ return this;
+ } catch (IllegalStateException e) {
+ // Stale singleton: was registered to a previous SparkContext that no
longer exists.
+ // AccumulatorV2.metadata is non-null (so register() rejects it) but
isRegistered()
+ // returned false (the id is not in the current AccumulatorContext).
+ // Create a fresh accumulator, register it, and swap it into the
registry cache.
+ DistributedRegistry fresh = new DistributedRegistry(this.name);
+ fresh.counters.putAll(this.counters);
+ jsc.sc().register(fresh);
+ Registry.REGISTRY_MAP.forEach((key, registry) -> {
Review Comment:
This swap can only reach `Registry.REGISTRY_MAP`, but
`HoodieSparkEngineContext` keeps a second cache, `DISTRIBUTED_REGISTRY_MAP`,
which is private static and not visible from here.
`createWrapperFileSystemRegistries` resolves the HoodieWrapperFileSystem
registries through `getMetricRegistry`, so after a swap that path would still
hand back the dead accumulator and silently report nothing, even once the
metadata table path recovers.
Would it work better to do the detect-and-replace in `getMetricRegistry`,
where both caches are in scope? That would also cover the `computeIfAbsent`
short circuit Danny raised above, since the two look like the same root cause.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
