jerryshao commented on code in PR #14641:
URL: https://github.com/apache/gravitino/pull/14641#discussion_r4479840647
##########
core/src/main/java/org/apache/gravitino/meta/ColumnEntity.java:
##########
@@ -163,13 +164,33 @@ public static Builder builder() {
return new Builder();
}
+ /**
+ * Projects a source column comment into the portable entity-store
representation. The full source
+ * comment remains available from the connector table.
+ *
+ * @param comment the source comment
+ * @return a NUL-free comment bounded by the storage capacity
+ */
+ @Nullable
+ public static String commentForStorage(@Nullable String comment) {
+ if (comment == null) {
+ return null;
+ }
+ String safe = comment.replace("\0", "");
+ int end = Math.min(safe.length(),
EntityFieldLimits.MAX_COLUMN_COMMENT_LENGTH);
Review Comment:
**major:** `commentForStorage` caps UTF-16 units, while
`ColumnEntity.COMMENT.validate` / `EntityFieldLimits.checkMaxLength` cap
Unicode code points (MySQL utf8mb4 and PostgreSQL `VARCHAR(4096)` count an
emoji as one character).
`"a".repeat(4095) + "😀"` is 4096 code points, so CREATE/ALTER accept it, and
then `toColumnEntity` stores it without the emoji. `TestColumnEntity` asserts
that drop. As a result, managed create returns a shorter comment than the
caller sent. Managed alter stores the raw comment in
`ManagedTableOperations.updateColumnEntity`, so create and alter disagree. On
external tables, `isSameColumn` compares projections, so two different in-limit
comments can skip the store update.
Could you truncate imports with `offsetByCodePoints` (which also keeps
surrogate pairs intact) and avoid projecting a comment that request validation
already accepted? If H2 really limits UTF-16 units, please reject that case in
`validateColumnComment` before source DDL. It would also be good to add
round-trip tests for 4096 emoji and for 4095 BMP chars plus one emoji, and to
apply the same rule on the managed alter path.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -861,7 +861,7 @@ public Catalog createCatalog(
.build();
return TreeLockUtils.doWithTreeLock(
- metalakeIdent,
+ ident,
Review Comment:
**major:** Narrowing `createCatalog` to the catalog write lock lets
`listCatalogs` (which takes the metalake read lock) run during backend
verification. However, `store.put` still happens before
`createAndCacheCatalogLease`. If init then fails, the row is deleted, so
`listCatalogs` can return a catalog that then disappears. `listCatalogsInfo`
loads through `acquireCatalogLease`, which does not take this catalog lock, so
it can start a second `validateOnCreate` and cache a wrapper alongside the
creator.
Could you probe the backend before `store.put`, still under the same catalog
write lock, and only persist the entity after init succeeds? Sibling catalogs
would stay unblocked, and a failed create would never be published.
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -447,42 +447,47 @@ public Table alterTable(NameIdentifier ident,
TableChange... changes)
@Override
public boolean dropTable(NameIdentifier ident) {
NameIdentifier schemaIdentifier = getSchemaIdentifier(ident);
- return TreeLockUtils.doWithTreeLock(
- schemaIdentifier,
- LockType.WRITE,
- () -> {
- NameIdentifier catalogIdent = getCatalogIdentifier(ident);
- boolean droppedFromCatalog =
- doWithCatalog(
- catalogIdent,
- c -> c.doWithTableOps(t -> t.dropTable(ident)),
- RuntimeException.class);
-
- boolean isManagedTable = isManagedEntity(catalogIdent,
Capability.Scope.TABLE);
- if (isManagedTable) {
- return droppedFromCatalog;
- }
-
- // A false result is ambiguous: the external table may have been
renamed or dropped out of
- // band. Preserve the registration because deleting it after a
rename would lose
- // Gravitino-only metadata. A true out-of-band drop can therefore
leave a stale
- // registration that requires separate cleanup.
- if (droppedFromCatalog) {
- try {
- store.delete(ident, TABLE);
- } catch (OptimisticLockException e) {
- throw e;
- } catch (NoSuchEntityException e) {
- LOG.warn("The table to be dropped does not exist in the store:
{}", ident, e);
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
- }
- // Run unconditionally: an out-of-band drop may have left orphaned
schema entities. The
- // cleanup is best-effort and stops as soon as a schema still exists.
- OrphanedSchemaCleanup.cleanUp(this, catalogIdent, schemaIdentifier);
- return droppedFromCatalog;
- });
+ Pair<Boolean, Boolean> result =
+ TreeLockUtils.doWithTreeLock(
+ ident,
Review Comment:
**minor:** `dropTable` no longer holds the schema write lock across source
DDL, but `purgeTable` (around L513) and `ViewOperationDispatcher#dropView`
still do. A blocked purge or view drop will therefore still stall sibling
list/load operations in that schema. Could you apply the same table-lock plus
read-probe cleanup to those paths, or call out the remaining schema-wide stall
in the PR description?
##########
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryIT.java:
##########
@@ -502,6 +534,7 @@ void executeSqlFileWithGenOutput(
firstLine = false;
String sql = resolveParameters(sqlMatcher.group(1));
+ sql = sql.replaceFirst("^<RETRY_UNTIL_MATCH>\\s*", "");
Review Comment:
**minor:** `executeSqlFileWithCheckResult` honors `<RETRY_UNTIL_MATCH>`, but
`executeSqlFileWithGenOutput` strips the flag and runs `SHOW` once.
Regenerating the PostgreSQL datatype golden file can then capture the staging
table, and the retry would match that dirty output immediately, so it would no
longer catch the leak. Could you generate that result through `executeQuery`
with the same timeout?
##########
core/src/main/java/org/apache/gravitino/lock/TreeLock.java:
##########
@@ -39,22 +39,22 @@
* /metalake readLock
* /metalake/catalog readLock
* /metalake/catalog/db1 readLock
- * /metalake/catalog/db/table1 readLock
+ * /metalake/catalog/db1/table1 readLock
* </pre>
*
- * If we need to alter a table `metalake.catalog.db1.table1` (without changing
the name of it), the
- * lock manager will lock the following:
+ * If we need to drop or alter a table `metalake.catalog.db1.table1` (without
renaming it), the lock
Review Comment:
**nit:** This example says a non-rename alter write-locks the table.
`dropTable` does, but `alterTable` still takes a read lock on the table unless
it renames (see `TableOperationDispatcher`, which locks READ when the lock
ident is the table). The preformatted rows also have an extra space before
`readLock`/`writeLock`.
--
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]
