jerryshao commented on PR #13351:
URL: https://github.com/apache/gravitino/pull/13351#issuecomment-5754470736
**Verdict:** blocking issues — the lazy capability → `ops()` resolution
escapes the catalog's isolated classloader, and `getTableObjectsByName` now
costs two extra metastore round-trips per table.
### Findings
1.
`catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveCatalog.java:91`
— `HiveCatalogCapability` resolves the metastore version through `ops()`, and
that resolution can now run **outside** the catalog's `IsolatedClassLoader`.
`TableNormalizeDispatcher.createTable`
(core/src/main/java/org/apache/gravitino/catalog/TableNormalizeDispatcher.java:79-82)
obtains the `Capability` inside `doWithCatalog(...)` but calls
`applyCapabilities(columns, capability)` after returning, i.e. under the server
classloader; `CapabilityHelpers.applyColumnNotNull`
(core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java:570-575)
then calls `columnNotNull()`, which reaches
`HiveCatalogCapability.requireHive3` → `hiveVersion.get()` →
`HiveCatalog.hiveVersion()` → `BaseCatalog.ops()` →
`HiveCatalogOperations.initialize(...)` +
`clientPool.run(HiveClient::hiveVersion)`
(HiveCatalogOperations.java:1105-1124). So on the first `createTable` after a
restart, the wh
ole Hive ops stack (Hadoop `Configuration`, Kerberos login,
`HiveClientFactory`, the HMS connection) is constructed with the wrong TCCL —
exactly what `CatalogManager.initCatalogWrapper` avoids by preloading
`properties()`/`capability()` inside the isolated loader
(core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:1821-1827:
"Preload properties() and capability() inside the IsolatedClassLoader so that
AppClassLoader can read them later without needing the isolated context"). A
second consequence: an HMS outage now surfaces as a connection failure from
column validation rather than from the operation itself. Suggested fix: resolve
and cache the version inside the isolated context — e.g. in
`HiveCatalogOperations.initialize()`, with the capability reading only the
cached value — instead of letting the capability trigger ops creation.
(verified by: read the dispatch chain `TableNormalizeDispatcher` →
`CapabilityHelpers` → `HiveCatalogCapability` → `HiveCat
alog.hiveVersion()` → `BaseCatalog.ops()` in this checkout; not reproduced
against a running server, so please confirm whether Hive ops initialization
tolerates the app classloader.)
2.
`catalogs/hive-metastore3-libs/src/main/java/org/apache/gravitino/hive/client/hive3/HiveShimV3.java:344`
— `getTableObjectsByName` calls `loadColumnConstraints` per returned table,
and each call makes two RPCs (`getNotNullConstraints`, `getDefaultConstraints`,
lines 391/401). `HudiHMSBackendOps.listTables`
(catalogs/catalog-lakehouse-hudi/src/main/java/org/apache/gravitino/catalog/lakehouse/hudi/backend/hms/HudiHMSBackendOps.java:147-153)
calls it for every table in a schema and only uses `t.name()` plus properties,
so listing a 500-table Hudi schema on a Hive 3 HMS goes from 2 calls to ~1001
sequential round-trips. `HiveCatalogOperations` already avoids this API for
exactly this reason (HiveCatalogOperations.java:418-424, "getTableObjectsByName
materializes every table and is slow on large databases"). Suggested fix: skip
constraint loading in the batch path (or make it opt-in), so only single-table
`getTable` pays for it. (verified by: read both call sites and the constrain
t-loading helper in this checkout.)
3.
`catalogs/hive-metastore3-libs/src/main/java/org/apache/gravitino/hive/client/hive3/HiveShimV3.java:367`
— the `close()` override drops the null guard that the base class has
(`HiveShim.close()`,
catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/client/HiveShim.java:321-325).
The override adds nothing else; deleting it keeps the safer inherited
behavior. (verified by: compared both method bodies.)
4. `build.gradle.kts:355` — the root `subprojects {}` block returns early
for `:catalogs:hive-metastore2-libs` and `:catalogs:hive-metastore3-libs`,
which skips error-prone, the `-Xlint:*`/`-Werror` compiler args
(build.gradle.kts:475-491) and jacoco for those modules. That exclusion was
harmless while the modules only repackaged dependency jars; this PR moves ~575
lines of production logic (`HiveShimV3`, `HiveShimV2`) into them, so the new
code is the only shim code not covered by the project's static analysis, and
its tests do not appear in the coverage report. Suggested fix: narrow the early
return to the publishing/packaging parts and keep the java quality config.
(verified by: read the early return and the error-prone/`-Werror` configuration
it skips.)
5. Open question —
`catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/converter/HiveColumnDefaultValueConverter.java:202-207`
— string defaults are escaped/unescaped with backslashes (`'it\'s'`).
Gravitino-written values round-trip (that is what `CatalogHive3IT` asserts),
but a `DEFAULT` written by Hive DDL with SQL doubling (`'it''s'`) is read back
as the literal `it''s`, and it is not obvious that Hive's own parser accepts
the backslash form in the constraint text Gravitino writes. Could you confirm
against a real Hive 3 `CREATE TABLE ... DEFAULT` for a string containing a
quote?
### Tests
Unit coverage for the new pieces is good (`TestHiveShimV3`,
`TestHiveColumnDefaultValueConverter`, `TestHiveCatalogCapability`), and
`CatalogHive3IT` covers create/rename/property-alter round-trips through HMS.
Gaps:
- No test creates a `NOT NULL`/`DEFAULT` constraint on a **partition**
column. `CatalogHive3IT.checkColumnConstraintsOnCreate`
(catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/integration/test/CatalogHive3IT.java:56-87)
uses `Transforms.EMPTY_TRANSFORM`, and in `checkColumnConstraintsOnAlter`
(line 130) the partition column stays nullable with no default — yet
`buildColumnConstraints` (HiveShimV3.java:463-499) iterates all columns
including partition keys, which HMS stores outside the storage descriptor. One
IT for a partitioned table with a non-nullable partition column would close
this.
- `TestHiveCatalog.testCapabilityWithCustomOperations`
(catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/TestHiveCatalog.java:126-130)
sets `ops-impl` to `HiveCatalogOperations` itself, so `ops instanceof
HiveCatalogOperations` is true and the `HIVE2` fallback in
HiveCatalog.java:93-99 never runs; the assertion passes only because the test
HMS is Hive 2. Use a custom `CatalogOperations` class to actually exercise the
fallback.
- `TestHiveShimV3.testAlterTableDropsAndRecreatesConstraints`
(catalogs/hive-metastore3-libs/src/test/java/org/apache/gravitino/hive/client/hive3/TestHiveShimV3.java:165-185)
verifies that drop/alter/add all happened but not their order, which is the
invariant that makes the sequence safe. `InOrder` would pin it down.
### Nits
- `HiveShimV3.constraintName` (HiveShimV3.java:501-510) regenerates a random
constraint name on every alter, so constraint names are not stable across
alters; worth a line in the docs if users may reference them.
- `HiveCatalog.hiveVersion()` (HiveCatalog.java:93-99) downgrades silently
to `HIVE2` at `LOG.debug` level; a user on Hive 3 with a custom `ops-impl`
would get "the connected Hive Metastore version is HIVE2", which is misleading
— `LOG.warn` with the reason would help.
---
_Generated by [Claude Code](https://claude.ai/code)_
--
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]
