rangareddy commented on issue #17349:
URL: https://github.com/apache/hudi/issues/17349#issuecomment-5351236735
This issue was reviewed as part of the JIRA-migrated backlog triage
(HUDI-8778).
**Findings: the mechanism you described appears to be addressed on `master`,
but it has never been verified end to end, so this needs one test run rather
than a disposition.**
The restart ordering looks correct today:
- `HoodieStreamer.reInitDeltaSync():806-810` calls `streamSync.close()`
**before** constructing the new `StreamSync`.
- `StreamSync.close():1422` calls `metrics.shutdown()`, and
`HoodieStreamerMetrics.shutdown():231-233` delegates to `Metrics.shutdown()`.
- `Metrics.shutdown(boolean):123-141` evicts the cached instance in its
`finally` block:
```java
} finally {
METRICS_INSTANCE_PER_BASEPATH.remove(basePath);
initialized = false;
}
```
so the next `HoodieStreamerMetrics` (`:51`, `Metrics.getInstance(...)`)
should build a fresh instance and start its reporters. That eviction is the one
line added by PR #11400 (`44922f160bd2`, merged 2024-06-05).
**Why that is not enough to close this.**
1. **#11400 predates this report by six months.** So the failure was
reproduced with that code already on master, which means reading the current
source cannot settle it - the code and the field evidence disagree.
2. **Your reproduction was never committed.** The test attached to
[#12282](https://github.com/apache/hudi/issues/12282)
(`TestHoodieDeltaStreamer#testMetricsWithHotSwap` plus `TestHotUpdateStrategy`,
in `hudi-12282-unit-test.patch`) is not in the repo - neither symbol exists on
`master`. Nothing has demonstrated the streamer path works, and there is no
regression guard if it does.
3. **#11400's only test does not cover this scenario.**
`hudi-common/src/test/java/org/apache/hudi/metrics/TestMetrics.java` exercises
`Metrics` in isolation - `assertSame` on reuse, `assertNotSame` and
`isInitialized` after shutdown - and never involves `StreamSync`
re-initialization.
**A latent gap worth closing either way.** `Metrics.getInstance():81-90`
hands back the cached instance without checking `initialized`:
```java
if (METRICS_INSTANCE_PER_BASEPATH.containsKey(basePath)) {
return METRICS_INSTANCE_PER_BASEPATH.get(basePath);
}
```
The eviction that keeps that safe lives inside `shutdown()`'s `if
(initialized)` block, preceded by an unguarded
`Runtime.getRuntime().removeShutdownHook(shutdownThread)`. Any path that leaves
`initialized == false` without evicting still returns a stopped instance. And
since `BaseHoodieClient.close()` and `BaseHoodieWriteClient.close()` do not
shut metrics down, `HoodieStreamerMetrics` is the only shutdown path - so this
invariant rests on a single call site. Validating `initialized` inside
`getInstance` would make the class safe regardless of caller behaviour.
**Concrete next step**, since the artifact already exists:
```bash
patch < hudi-12282-unit-test.patch
mvn -pl hudi-utilities -Dtest=TestHoodieDeltaStreamer#testMetricsWithHotSwap
test
```
If it passes on current `master`, please commit the test as a regression
guard and we can close this. If it fails, this is a live bug and we will treat
it as such.
Keeping this open pending that run.
--
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]
