roryqi commented on PR #11036: URL: https://github.com/apache/gravitino/pull/11036#issuecomment-4438434755
> ## Review Summary (post-merge with main) > Three inline comments are attached to lines changed directly by this PR. The following issues were found in code already merged to `main` via prior PRs — they can't be raised as inline comments here but are worth tracking separately: > > **`selectChangedOwners` (introduced in #10913, already in `main`)** > > 1. **Hardcoded `LIMIT 1000` with no pagination path** — if there are >1000 owner changes since `updatedAtFrom`, the poller silently drops the remainder. `EntityChangeLogMapper.selectEntityChanges` uses a `maxRows` parameter; this query should do the same, or the caller must document that it pages by advancing `updatedAtFrom` to the max value in the last batch. > 2. **No metalake scope** — returns changed owners across all metalakes. `selectEntityChanges` scopes by `metalakeName`; the contract here should either match or be explicitly documented as intentionally global. > > **`OwnerInfo.ownerId` (already in `main`)** > > 3. **Primitive `long` risks NPE from MyBatis** — `selectOwnerByMetadataObjectIdAndType` returns `null` when no row matches. If MyBatis tries to set a null `owner_id` column value into the primitive `long` field it will throw `NullPointerException`. Use `Long` (boxed) or add a `NOT NULL` guarantee at the DB level. This is out of this pull request scope. -- 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]
