wenzhenghu commented on PR #65126:
URL: https://github.com/apache/doris/pull/65126#issuecomment-4975748559
## Final Review Summary
I completed another full review of PR #65126 at the latest head
`d8390a10538d509b2e52c5c4973f7cf651607819`, including static analysis, focused
FE unit tests, an FE build, and deployment-backed functional validation.
### Static Review
I did not find any remaining P0/P1 correctness issue in the production
implementation.
The following areas were reviewed in detail:
- **Manual miss loading and load deduplication:** slow external metadata I/O
is performed under the key-level load lock, while the publication lock is held
only for short cache mutation windows.
- **Generation-based stale write-back protection:** manual loads, explicit
mutations, per-key invalidation, bulk invalidation, and post-publication
identity removal form a consistent concurrency model.
- **Refresh behavior:** `refreshAfterWrite` remains asynchronous. Its
best-effort generation handling matches the explicitly accepted
eventual-consistency semantics and does not make the Replayer wait for remote
I/O.
- **Legacy MetaCache migration:** database names, table names, object
entries, and ID mappings are now represented by independent `MetaCacheEntry`
instances, with the existing loading, invalidation, replay, and
incremental-event behavior preserved.
- **Replay paths:** cache misses do not synchronously load external
metadata. Cache hits may still trigger Caffeine's asynchronous refresh
behavior, which is non-blocking and consistent with the documented cache-only
definition.
- **Case-insensitive mode 2:** canonical database/table cache keys are
resolved before drop, rename, replay, and engine-cache invalidation. The
previously reported raw-name invalidation issues are fixed.
- **Special paths and user input:** system databases, JDBC include/exclude
database filters, Iceberg REST session bypass, external DDL registration, and
normal/replay lookups retain their intended semantics.
- **Error handling:** disabled caches, null loader results, invalid stripe
configuration, load failures, and removal-listener constraints are covered by
focused logic and tests.
The previously reported Groovy closure forward-reference issue in
`test_jdbc_refresh_catalog_schema_refresh_non_blocking.groovy` has also been
fixed by declaring `getSchemaCacheSize` before `preheatSchemaCache`.
`git diff --check upstream/master...upstream/pr-65126` passes.
### FE Unit Tests
The following focused FE tests passed:
- `MetaCacheEntryTest`
- `NameCacheValueTest`
- `MetaCacheDeadlockTest`
- `ExternalCatalogTest`
- `ExternalDatabaseTest`
- `ExternalDatabaseSessionContextTest`
Result: **83 tests, 0 failures, 0 errors, 0 skipped**. Checkstyle also
passed.
### Build and Deployment Validation
- The FE build completed successfully.
- The latest PR FE was deployed on a single-FE test instance.
- The current master BE build encountered compiler failures in unrelated,
unmodified files (`-Wshadow` diagnostics and `std::powf` compatibility), so the
metadata functional tests used the current PR FE with an existing compatible
BE. Therefore, this review does not claim a complete current-head FE+BE build.
### Functional Validation
The following scenarios were exercised against a deployed instance:
1. **Slow table-name load versus `REFRESH CATALOG`**
- Slow `SHOW TABLES`: approximately 15.32 seconds.
- Concurrent `REFRESH CATALOG`: approximately 0.47 seconds.
- A second `SHOW TABLES` again took approximately 15.39 seconds,
confirming that the invalidated in-flight result was not retained.
2. **Slow schema load versus `REFRESH CATALOG`**
- Slow `DESC`: approximately 15.07 seconds.
- Concurrent `REFRESH CATALOG`: 47 ms.
- After the first load completed, the schema entry size remained 0.
- A second `DESC` loaded again in approximately 15.06 seconds and then
populated the cache.
3. **Name-miss refresh enabled**
- After names were preheated, a table created directly in the remote
MySQL instance became visible without an explicit Doris refresh.
- An unrestricted JDBC catalog also discovered a newly created remote
database without an explicit refresh.
4. **Name-miss refresh disabled**
- With `enable_external_meta_cache_name_miss_refresh=false`, a newly
created remote table correctly returned `Unknown table`.
- After an explicit `REFRESH CATALOG`, the table became visible.
- The mutable configuration was restored to `true` after validation.
5. **JDBC database filtering**
- `only_specified_database` and `include_database_list` continued to
filter newly created databases as expected.
These manual scenarios exercise the same non-blocking invalidation and
name-miss semantics targeted by the added regression cases. I did not run the
complete external JDBC regression suite end to end because it depends on a
dedicated external environment.
### Existing Review Findings
The mode-2 database/table invalidation findings raised against the older
head `0587501676` were valid and have been correctly fixed in the current
implementation. All review threads are currently resolved.
GitHub still reports `CHANGES_REQUESTED` because the automated review state
was submitted against that older head. I do not see a remaining production-code
blocker in the latest head; the stale review state should be refreshed or
dismissed after re-review.
### Conclusion
From the external metadata cache implementation perspective, the PR is ready
for further community review and merge. I found no remaining blocking
production issue after the latest fixes.
--
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]
