jerryshao commented on PR #13307: URL: https://github.com/apache/gravitino/pull/13307#issuecomment-5728050394
Thanks a lot for the careful review, @yuqi1129! **1. Load/alter race.** Agreed: the in-lock re-match only covers "load read the catalog after the alter but the store before it". Your reverse ordering, and alter/alter, are still open. I'd prefer to keep the locking change out of this PR, since a WRITE lock in `alterTable` would block loads of that table for the whole catalog call, and that deserves its own discussion and tests. I updated the comment in `updateColumnsIfNecessaryWhenLoad` to say the window is narrowed, not closed. I also updated the PR description and am tracking the WRITE-lock option, together with your stale-snapshot test, under #13303. **2. HashMap order.** Good catch, fixed in 5b12d8bd3. Renamed and dropped stored columns now claim their catalog names before untouched ones. I added `testRenameIntoNameOfStaleStoredColumn`: it drops `b` via `TestCatalogOperations`, then renames `z -> b` through the dispatcher. The names are chosen so the stale `b` is iterated first, and I verified the test fails without the fix. **3. Owner branch.** I kept it. `SecurableObjects` doesn't support COLUMN, but the owner API doesn't reject it: `OwnerOperations.setOwnerForObject` only checks that the table exists (`MetadataObjectUtil.checkMetadataObject`), and the store resolves column ids, so column owner rows can exist. I clarified this in the comment. **4.** Agreed, I noted a possible guard for `columns() == null` on load in #13303. **Tests:** the stale-snapshot ordering test will go with the lock change. The case-insensitive, mixed-case rename is a known gap, also tracked in #13303; it falls back to drop + add, the same as before this PR. If a catalog ignores the rename, the mapped name isn't found in the returned table, so it also falls back to drop + add without errors. **CI:** `build (17)` runs with `-PskipDockerTests=false` on Linux, which sets `dockerTest=true`, so the `TestJDBCBackend`-based tests (including `TestTableColumnMetaService` and `TestOrphanedMetadataObjectRelationService`) run on H2, MySQL and PostgreSQL. Earlier runs were cancelled by newer pushes; the current one is pending. -- 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]
