yuqi1129 commented on PR #13904: URL: https://github.com/apache/gravitino/pull/13904#issuecomment-6312495089
Review of commit `76f58c09425d302f2f3dd5943865dc029a69649f`: three issues to address and one non-blocking configuration clarification. **1. [StaleRegistrationReconciler.java:139](https://github.com/apache/gravitino/blob/76f58c09425d302f2f3dd5943865dc029a69649f/core/src/main/java/org/apache/gravitino/catalog/StaleRegistrationReconciler.java#L139)** [Important] The scheduled entry point discards every report returned by reconcileRound(). The round itself does not log successful scan results, and there is currently no report cache or REST consumer, so enabling report-only produces no observable output when stale schemas are found. The tests inspect the returned list directly and therefore miss this production wiring gap. Could we log the per-round counts and stale object identifiers (or otherwise consume the reports) from the scheduled path, and cover that output in a test? Without this, the report-only feature does source probes but cannot tell an operator what it found. **2. [StaleRegistrationReconciler.java:159](https://github.com/apache/gravitino/blob/76f58c09425d302f2f3dd5943865dc029a69649f/core/src/main/java/org/apache/gravitino/catalog/StaleRegistrationReconciler.java#L159)** [Important] close() waits five seconds and then calls shutdownNow(), but an InterruptedException from jitter or a connector callback is caught as an ordinary catalog failure (or converted to UNREACHABLE by probeSchema), and the round proceeds to the next catalog. GravitinoEnv.shutdown() can meanwhile continue closing the entity store and catalog manager. I reproduced this with a latch blocking the first catalog: after close() interrupts it, the second catalog is still invoked. Please add closed/cancellation checks at the round/catalog/schema boundaries and restore the interrupt status and exit on interruption. The regression test should exercise an already-running round. **3. [StaleRegistrationReconciler.java:256](https://github.com/apache/gravitino/blob/76f58c09425d302f2f3dd5943865dc029a69649f/core/src/main/java/org/apache/gravitino/catalog/StaleRegistrationReconciler.java#L256)** [Important] This catch classifies every NotFoundException as confirmed absence of the target schema, including NoSuchCatalogException and NoSuchEntityException. Those can identify a missing catalog or another dependency rather than the target schema; treating them as ABSENT produces a stale report instead of discarding an unconfirmed catalog scan. The SupportsSchemas default implementation only maps NoSuchSchemaException to false. Could we similarly restrict this fallback to a schema-specific absence signal and classify other failures as unconfirmed/unreachable? A focused probe throwing NoSuchCatalogException currently returns ABSENT. **4. [Configs.java:800](https://github.com/apache/gravitino/blob/76f58c09425d302f2f3dd5943865dc029a69649f/core/src/main/java/org/apache/gravitino/Configs.java#L800)** [Nit] The configuration accepts auto-remove and its description says it removes registrations, but this implementation only logs the selected policy and always emits removed=false. Deferring removal to the next subtask is reasonable; could we reject auto-remove until implemented, or clearly warn/document that it currently remains report-only? Otherwise an operator can select an advertised policy and have it silently ignored. Validation: 83 relevant tests passed (TestStaleRegistrationReconciler, TestConfig, TestGravitinoEnvMetadataComponents, TestCatalogManager, and TestHierarchicalSchemaUtil). Two temporary probes reproduced continued catalog processing after cancellation and the broad NotFound classification. A temporary mutation of removed=false was detected by the existing regression test. All probes and mutations were reverted. The full core suite and Docker tests were not 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]
