github-actions[bot] commented on PR #61819: URL: https://github.com/apache/doris/pull/61819#issuecomment-4141772521
## Additional Findings (outside diff hunks) ### 1. Concurrency Bug (High): `Auth.doesRoleExist()` — `Auth.java:160-162` `doesRoleExist()` accesses `roleManager` without acquiring `Auth`'s read lock. Per the comment at line 104-106: *"There is no concurrency control logic inside roleManager... their methods cannot be directly called outside."* Every other public method in `Auth` acquires the lock. This is called from `RoleMappingMgr.normalizeGrantedRoles()` at line 231, which holds `RoleMappingMgr`'s write lock but NOT `Auth`'s lock, creating a data race on `roleManager`'s internal map. **Fix:** Wrap the body in `readLock()`/`readUnlock()`, or have callers acquire `Auth.readLock` first. ### 2. EditLog Replay Bug (Medium): `EditLog.java:1095-1104` Replay of `OP_ALTER_AUTHENTICATION_INTEGRATION` and `OP_DROP_AUTHENTICATION_INTEGRATION` updates only the metadata manager, not the `AuthenticationIntegrationRuntime`. On the master-side DDL path, `alterAuthenticationIntegrationProperties()` calls `runtime.markAuthenticationIntegrationDirty()` and `dropAuthenticationIntegration()` calls `runtime.removeAuthenticationIntegration()`. On followers, after replay, stale plugins remain cached. Note: `AuthenticationIntegrationRuntime.replayUpsertAuthenticationIntegration()` (line 155) and `rebuildAuthenticationIntegrations()` (line 162) exist but are **never called** — dead code that was intended to solve this. **Fix:** Wire ALTER replay to call `runtime.replayUpsertAuthenticationIntegration(log)` and DROP replay to call `runtime.removeAuthenticationIntegration(log.getIntegrationName())`. ### 3. Lifecycle Issue (Medium): `Env.java:2522-2531` Image load replaces `authenticationIntegrationMgr` with a freshly deserialized object, but `authenticationIntegrationRuntime` (caching plugins, dirty flags, runtime states) is never reset. Stale state persists from prior lifecycle. **Fix:** After loading, call `authenticationIntegrationRuntime.rebuildAuthenticationIntegrations(...)`. ### 4. Coding Standard (Low): `AuthenticationIntegrationRuntime.java:280-288,331-333` Defensive null checks on `Env.getCurrentEnv()` should be assertions per Doris coding standard (*"Assert correctness only — never use defensive programming"*). These silent fallbacks mask bugs. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
