yuqi1129 commented on PR #13388: URL: https://github.com/apache/gravitino/pull/13388#issuecomment-5772495147
> Verdict: blocking issues > > Re-review of the 1 commit since `951964ce` (head `182b832`, 6 files). I read the full delta plus every file it touches, re-grepped the callers of both changed metric signatures, and checked the three new tests against the paths they claim to cover. > > **Previously raised:** > > * **[Important] Tail sample sat on the delivery critical path — fixed.** `fetchNextDelivery` now wraps the `selectMaxChangeId` call in its own try/catch (EntityChangeLogPoller.java:224-235); a non-interrupt failure only logs, and `deliver()` still runs on the batch that was already fetched. `testTailSampleFailureDoesNotSuppressFetchedBatch` covers it. One consequence of the fix is not covered — see the inline comment. > * **[Nit] `pollFailed()` counted interrupted shutdown polls — fixed.** `metrics.pollFailed()` now sits below the interrupt check (EntityChangeLogPoller.java:202-205), with `testInterruptedPollIsNotCountedAsFailure` and `testInterruptedTailSampleStopsDeliveryWithoutCountingFailure`. > * **[Nit] Unbounded metric names for lambda listeners — fixed, and the guard verifiably works.** `listenerMetricName` (EntityChangeLogPoller.java:384-389) buckets synthetic and anonymous classes as `anonymous`. I ran a small program on this container's JDK to check the predicate rather than assume it: a lambda reports `isSynthetic()=true` (name `L$$Lambda/0x00007ff79014c408`, a hidden class), an anonymous class reports `isAnonymousClass()=true`, and named, local and inner classes report false for both. So the guard catches exactly the unstable names and leaves `EntityCacheChangeLogListener`, `CatalogChangeLogListener` and `JcasbinChangeListener` on their real class names. > * **[Nit] Test-only nullable metrics — declined, and the reasoning holds.** Both single-argument constructors predate this PR, so keeping them is fair. The part that mattered is gone: `EntityCacheChangeLogListener` has no `@Nullable` field and none of the four hot-loop guards (lines 113-132). The residual is only that the legacy constructor now builds its own unregistered source, so a listener created that way records into a registry nothing reads; the Javadoc says so, so this is a note rather than a request. > * **[Question] Per-listener delivery breakdown — implemented.** `records-delivered.<class>-total` (EntityChangeLogMetricsSource.java:86-89) reuses the same stable listener name as the failure counter, with docs and tests updated. I also confirmed these lazily created per-class counters really are exported, which was not obvious: `MetricsSystem.register` hands the source's `MetricRegistry` to `MetricRegistry.register(String, Metric)` (MetricsSystem.java:92-93), and Dropwizard Metrics 4.2.25 (gradle/libs.versions.toml:99) special-cases a child `MetricRegistry` by attaching a listener to it, so counters created after registration still propagate to the parent registry and reach JMX and Prometheus. > > **Tests:** adequate for the delta itself. Two gaps remain worth closing: > > 1. Nothing asserts `record-lag` or `db-tail-id` once the cursor has advanced past a stale tail. `testTailSampleFailureDoesNotSuppressFetchedBatch` pre-seeds the tail to 9 and advances the cursor only to 1, so it never reaches the state the inline finding describes; `grep -rn "record-lag" core/src/test` shows the gauge is only asserted on healthy paths (TestEntityChangeLogMetricsSource.java:44, TestEntityChangeLogPoller.java:308). > 2. `RelationalEntityStore`'s register/unregister of the source — including the null-`MetricsSystem` path at RelationalEntityStore.java:109-111 and the unregister at 296-298 — is still untested. `grep -rn "changeLogMetrics" core/src/test` returns nothing. This is the code that decides whether any of these metrics are exported at all. > > **Not verified:** no build or test execution. A `:core` compile does not finish inside the time budget available here, so every finding comes from reading this checkout, plus the standalone JDK check described above. > > _Review drafted with Claude, requested by @jerryshao._ Changed as suggested. -- 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]
