Copilot commented on code in PR #10793: URL: https://github.com/apache/gravitino/pull/10793#discussion_r3129496053
########## scripts/postgresql/schema-1.3.0-postgresql.sql: ########## @@ -0,0 +1,954 @@ +-- +-- Licensed to the Apache Software Foundation (ASF) under one +-- or more contributor license agreements. See the NOTICE file-- +-- distributed with this work for additional information Review Comment: The Apache license header has a formatting typo: "See the NOTICE file--" (missing a newline/space before the trailing "--"). Please fix the header text so it matches the standard ASF template. ########## scripts/mysql/upgrade-1.2.0-to-1.3.0-mysql.sql: ########## @@ -0,0 +1,84 @@ +-- +-- Licensed to the Apache Software Foundation (ASF) under one +-- or more contributor license agreements. See the NOTICE file +-- distributed with this work for additional information +-- regarding copyright ownership. The ASF licenses this file +-- to you under the Apache License, Version 2.0 (the +-- "License"). You may not use this file except in compliance +-- with the License. You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +-- Unless required by applicable law or agreed to in writing, +-- software distributed under the License is distributed on an +-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +-- KIND, either express or implied. See the License for the +-- specific language governing permissions and limitations +-- under the License. +-- + +-- Role privilege tracking (strong consistency -- Step 3 version check) +ALTER TABLE `role_meta` + ADD COLUMN `updated_at` BIGINT NOT NULL DEFAULT 0 + COMMENT 'Set to currentTimeMillis() on any privilege grant/revoke for this role. + JcasbinAuthorizer compares db.updated_at vs cached updated_at per request + to decide whether to reload JCasbin policies for this role.'; + +-- User role assignment tracking (strong consistency -- Step 1a version check) +ALTER TABLE `user_meta` + ADD COLUMN `updated_at` BIGINT NOT NULL DEFAULT 0 + COMMENT 'Set to currentTimeMillis() on any role assign/revoke for this user. + JcasbinAuthorizer compares db.updated_at vs cached updated_at per request + to decide whether to reload the user-role mapping.'; + +-- Group role assignment tracking (strong consistency -- Step 1b version check) +ALTER TABLE `group_meta` + ADD COLUMN `updated_at` BIGINT NOT NULL DEFAULT 0 + COMMENT 'Set to currentTimeMillis() on any role assign/revoke for this group. + JcasbinAuthorizer compares db.updated_at vs cached updated_at per request + to decide whether to reload the group-role mapping.'; + +-- Ownership mutation tracking (eventual consistency -- owner change poller) +ALTER TABLE `owner_meta` + ADD COLUMN `updated_at` BIGINT NOT NULL DEFAULT 0 + COMMENT 'Set to currentTimeMillis() on any ownership transfer. + The owner change poller reads updated_at > maxSeen to find changed rows + and invalidates only the specific metadataObjectIds in ownerRelCache.'; + +-- Covering indexes for high-frequency read predicates +CREATE INDEX idx_user_meta_name_del_upd + ON user_meta (metalake_id, user_name, deleted_at, updated_at); +CREATE INDEX idx_group_meta_del_upd + ON group_meta (group_id, deleted_at, updated_at); +CREATE INDEX idx_role_meta_del_upd + ON role_meta (role_id, deleted_at, updated_at); +CREATE INDEX idx_owner_meta_obj_del_upd + ON owner_meta (metadata_object_id, deleted_at, updated_at); + +-- Backfill: set updated_at = audit_info-extracted time (use 1 as safe default for existing rows) +UPDATE `role_meta` SET `updated_at` = 1 WHERE `updated_at` = 0 AND `deleted_at` = 0; +UPDATE `user_meta` SET `updated_at` = 1 WHERE `updated_at` = 0 AND `deleted_at` = 0; +UPDATE `group_meta` SET `updated_at` = 1 WHERE `updated_at` = 0 AND `deleted_at` = 0; +UPDATE `owner_meta` SET `updated_at` = 1 WHERE `updated_at` = 0 AND `deleted_at` = 0; + +-- Group-user membership table +CREATE TABLE IF NOT EXISTS `group_user_rel` ( + `id` BIGINT(20) UNSIGNED NOT NULL AUTO_INCREMENT COMMENT 'auto increment id', + `group_id` BIGINT(20) UNSIGNED NOT NULL COMMENT 'group id', + `user_id` BIGINT(20) UNSIGNED NOT NULL COMMENT 'user id', + `deleted_at` BIGINT(20) UNSIGNED NOT NULL DEFAULT 0 COMMENT 'deleted at', + PRIMARY KEY (`id`), + UNIQUE KEY `uk_gid_uid_del` (`group_id`, `user_id`, `deleted_at`) +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin COMMENT 'group user relation'; + +-- Entity name->id mutation tracking (eventual consistency -- entity change poller) +CREATE TABLE IF NOT EXISTS `entity_change_log` ( + `id` BIGINT UNSIGNED NOT NULL AUTO_INCREMENT, + `metalake_name` VARCHAR(128) NOT NULL, + `entity_type` VARCHAR(32) NOT NULL COMMENT 'METALAKE | CATALOG | SCHEMA | TABLE | FILESET | TOPIC | MODEL | VIEW', + `full_name` VARCHAR(512) NOT NULL COMMENT 'Dot-separated full name of the affected entity. For RENAME, stores the OLD name (the stale key to invalidate). For DROP/ALTER, the entity name.', + `operate_type` VARCHAR(16) NOT NULL COMMENT 'DROP | CREATE | ALTER (ALTER covers rename and other structural changes)', + `created_at` BIGINT NOT NULL, + PRIMARY KEY (`id`), + INDEX `idx_created_at` (`created_at`) +) COMMENT 'Append-only log of entity structural changes. One row per affected entity per operation. The entity change poller reads this table to drive targeted invalidation of metadataIdCache on HA peer nodes. Rows older than the retention window (default 1 h) are pruned periodically.'; Review Comment: The upgrade script creates an index named `idx_created_at` on `entity_change_log`, but the 1.3.0 schema script names the same index `idx_ecl_created_at` (and Postgres/H2 use `idx_ecl_created_at`). This makes index naming inconsistent across installs/upgrades and can complicate troubleshooting/migrations. Consider renaming the upgrade index to `idx_ecl_created_at` to match the schema scripts and other dialects. ########## core/src/main/java/org/apache/gravitino/storage/relational/po/OwnerRelPO.java: ########## @@ -94,6 +95,11 @@ public Builder withDeleteAt(Long deleteAt) { return this; } + public Builder withUpdatedAt(Long updatedAt) { + ownerRelPO.updatedAt = updatedAt; + return this; + } Review Comment: `updatedAt` was added to `OwnerRelPO` and is now used in the owner_meta INSERT, but the builder doesn’t enforce that it’s set. Since the `owner_meta.updated_at` column is `NOT NULL` and `insertOwnerRel` binds `#{ownerRelPO.updatedAt}`, a missing `withUpdatedAt(...)` call would result in NULL insertion and a runtime failure. Consider validating `updatedAt != null` in the builder (or setting a default in the builder constructor) so the PO contract matches the table schema. ########## scripts/postgresql/upgrade-1.2.0-to-1.3.0-postgresql.sql: ########## @@ -0,0 +1,51 @@ +-- +-- Licensed to the Apache Software Foundation (ASF) under one +-- or more contributor license agreements. See the NOTICE file-- +-- distributed with this work for additional information Review Comment: The Apache license header has a formatting typo: "See the NOTICE file--" (missing a newline/space before the trailing "--"). Please fix the header text so it matches the standard ASF template. -- 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]
