davidzollo commented on PR #11215: URL: https://github.com/apache/seatunnel/pull/11215#issuecomment-4885608240
Thanks again for pushing this observability work forward — the feature itself is genuinely useful, and the overall direction still looks good to me. I do still see one blocker before merge in `ClusterMetricExports.resolveClusterSafe()`. When `PartitionServiceMBean` is unavailable, the fallback currently returns `!hasOngoingMigration() && hasSafeLocalMemberState()`. That is only a local-member-safe signal, but `seatunnel_engine_cluster_safe` is documented and exported as a cluster-level signal, so this path can report a false-positive `safe=1` from master-local evidence only. Hazelcast already exposes a cluster-level `PartitionService.isClusterSafe()`, so I would strongly recommend using that as the fallback instead, and adding one regression test for the MBean-missing path. Once that is tightened up, this PR should be in very good shape. After that and a sync with the latest `dev`, I'd be happy to take another look. -- 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]
