jarredhj0214 commented on PR #12288: URL: https://github.com/apache/gravitino/pull/12288#issuecomment-5314512521
> I found a few things worth addressing: > > 1. `AccessControlEventDispatcher#addUsers` and `#removeUsers` currently just delegate to the underlying dispatcher. The single-user paths dispatch `AddUserPreEvent` / `AddUserEvent` / `AddUserFailureEvent` and `RemoveUserPreEvent` / `RemoveUserEvent` / `RemoveUserFailureEvent`, but the bulk paths skip those events entirely. This means audit/listener integrations will miss user changes made through the new bulk APIs. Could we either emit the existing per-user events for each item, or introduce explicit bulk events and cover them in tests? > 2. `BulkRequestField` feels a bit over-engineered and also leaks REST request field names into the core `BulkManager`. Since it is only used to format an error message, I think this can be simplified to `checkBulkSize(String fieldName, int size)`, with the REST layer passing `"users"` / `"names"` directly or via private constants in `BulkOperations`. That keeps payload naming at the REST boundary and makes the helper more generic for future bulk APIs. > 3. The new `gravitino.server.bulk.maxItems` config is added to `Configs` and `conf/gravitino.conf.template`, but it is missing from `docs/gravitino-server-config.md`. Please add it to the server configuration table so users can discover the setting from the main config reference. > 4. Related to the new config: Docker config rewrite does not expose an env var mapping for it. If this server setting should be configurable the same way as other `gravitino.server.*` options in Docker, please add something like `GRAVITINO_SERVER_BULK_MAX_ITEMS -> server.bulk.maxItems` in `dev/docker/gravitino/rewrite_gravitino_server_config.py` and document it in the container config table. Thanks for the review. I have addressed the comments in the latest update: - Updated `AccessControlEventDispatcher#addUsers` and `#removeUsers` to emit the existing per-user add/remove pre, success, and failure events for each bulk item, with tests added. - Simplified `BulkManager#checkBulkSize` to take `String fieldName, int size`, and moved the REST request field names back to `BulkOperations`. - Added `gravitino.server.bulk.maxItems` to `docs/gravitino-server-config.md`. - Added the Docker env mapping `GRAVITINO_SERVER_BULK_MAX_ITEMS -> server.bulk.maxItems` and documented it in the container configuration table. -- 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]
