zclllyybb commented on issue #65750: URL: https://github.com/apache/doris/issues/65750#issuecomment-4999604889
Breakwater-GitHub-Analysis-Slot: slot_8fa76c90e7f5 This content is generated by AI for reference only. Initial triage: I re-read the live issue and checked the referenced code at `3a75387e61388bd886eeef38b794d5ed4eb298bf`. I also refreshed `upstream/master` to `9a5afd4958f2794a2dd458b2e7463315e9746ce3`; the relevant files have no diff from the issue commit, so these observations still apply to the current master snapshot I inspected. Judgment: - The report looks valid and is best handled as four independent FE HMS incremental-event cache consistency bugs. - I did not find evidence in the code path that makes any of the four cases intentionally safe. Code-backed details: 1. Rename cancellation path: confirmed. `AlterTableEvent.processRename()` calls `externalTableExistInLocal(tableAfter...)`; for HMS, this reaches `HMSExternalCatalog.tableExistInLocal()`, which calls `getDbNullable()` and `hmsExternalDatabase.getTable(tblName)`. `ExternalCatalog.getDbNullable()` and `ExternalDatabase.getTableNullable()` both use `metaCache.getMetaObj(...)`, and `MetaCache.getMetaObj()` loads on miss. That makes the check not cache-only. If HMS already exposes the renamed target, the check can materialize the new table and return true; `processRename()` then returns before unregistering `tableBefore`, so the old local name can remain. `AlterDatabaseEvent.processRename()` has the analogous risk because `catalog.getDbNullable(dbAfter.getName())` is also load-through; `getDbForReplay(...)` shows there is already a cache-only pattern available for databases. 2. Partition events: confirmed, with a concurrency nuance. `MetaCacheEntry` has generation-based protection against stale in-flight manual loads, but only when an invalidation actually happens. In `HiveExternalMetaCache.PartitionCacheCoordinator.addPartitionsCache()` and `dropPartitionsCache()`, the code returns when the partition-values entry/key is not present, so no generation is bumped for an in-flight partition-values load. In `dropPartitionsCache()`, this early return also prevents `invalidatePartitionCache(...)` from running, so stale partition metadata and file-listing entries can survive after a DROP. The lower-level invalidation helper can already fall back to table-id + partition-values matching when the exact partition metadata is cold, but the current DROP event path can skip calling that helper entirely. 3. Visibility filters: confirmed. Full names loading applies `include_database_list`, `exclude_database_list`, and `include_table_list` through `ExternalCatalog.getFilteredDatabaseNames()` and `ExternalCatalog.listTableNames()`. Incremental CREATE paths bypass those predicates: `CreateDatabaseEvent` -> `CatalogMgr.registerExternalDatabaseFromEvent()` -> `HMSExternalCatalog.registerDatabase()` updates the catalog meta cache directly, and `CreateTableEvent` -> `CatalogMgr.registerExternalTableFromEvent()` -> `ExternalDatabase.registerTable()` updates the DB meta cache and `lowerCaseToTableName` directly. If the names cache is hot, a filtered object can be inserted into the visible cached names. 4. ExternalMetaIdMgr mapping: confirmed. `DropPartitionEvent.transferToMetaIdMappings()` emits `META_OBJECT_TYPE_DATABASE` for a partition delete. `ExternalMetaIdMgr.handleDelMetaIdMapping()` interprets DATABASE delete by removing the whole `dbNameToMgr` entry, while PARTITION delete would only remove `partitionNameToMgr` under the table. This is a direct type mismatch. Missing information / validation needed: - For (1), a small HMS rename regression test should assert that the target-name existence probe is cache-only and that the old local table/db name is always removed. - For (2), a regression/unit test should cover a DROP with a hot lower-level partition/file cache but a cold partition-values cache, plus an in-flight partition-values load where the event occurs before the load publishes. - For (3), tests should cover both database and table include/exclude filters with a pre-warmed names cache before processing CREATE_DATABASE / CREATE_TABLE events. - For (4), an event mapping replay test should verify DROP_PARTITION deletes only the partition mapping and preserves sibling table/db mappings. Suggested next steps: - Split into separate PRs if that is easier to review: rename cache-only existence, partition event fencing/invalidation, shared visibility predicates for incremental register paths, and the DROP_PARTITION meta-id type fix. - Prefer sharing the same database/table visibility helper between full-list loading and event registration, so incremental events cannot drift from catalog-list semantics. - For partition events, make DROP invalidate partition metadata/file cache entries before attempting the hot partition-values snapshot update, and make ADD/DROP invalidate or generation-bump the partition-values key when the entry exists but the value is cold/loading. - For rename, use cache-only lookup APIs similar to `getDbForReplay` / `getTableForReplay` for local-existence checks, and avoid returning before removing the old name. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
