HonestManXin commented on PR #67622: URL: https://github.com/apache/doris/pull/67622#issuecomment-5611962195
> Review completed for `27996438a614c9703e2eb664f7f94b09bebf63fd`: changes are still requested. > > The latest head removes the FE-local/config-dependent upper bound, so it resolves the all-upgraded leader-identity and differing-new-FE-config variants from the earlier review. Two blocking concerns remain, both already captured in existing inline threads: > > 1. Rolling upgrade/rollback is still unsafe. A new FE can journal a value above 10000, while a base-version FE rejects it during `UserProperty.update`; `Auth.replayUpdateUserProperty` logs the exception and replay advances with stale user state. This is already covered by [#67622 (comment)](https://github.com/apache/doris/pull/67622#discussion_r3953931824). > 2. Arrow Flight SQL still snapshots `max_user_connections / 2` only when a user's token LRU is first created, while its connection pool skips the per-user admission check. Raising or lowering the property after cache creation therefore does not enforce the current value and can evict a live session prematurely or retain an oversized per-user cache. This is already covered by [#67622 (comment)](https://github.com/apache/doris/pull/67622#discussion_r3954565427). > > No new inline comments are included in this review because every substantiated defect is a hard duplicate of those threads. > > Critical checkpoint conclusions: > > * Goal and proof: The edit does allow classic MySQL users to be configured above 10000, and classic admission reads the current property. The whole feature is not proven or complete because no PR test crosses the removed boundary and the compatibility/Arrow paths above remain unresolved. > * Scope and clarity: The validation edit is small and clear, but it is too local for the runtime and persistence topology on which its justification relies. > * Concurrency: Existing `Auth` read/write locking and classic connection atomics remain sound; a concurrent classic property update is observed by subsequent admissions. Arrow's long-lived per-user cache is the unresolved lifecycle/concurrency-adjacent path above. > * Lifecycle: No new static lifecycle is introduced. The relevant non-intuitive lifecycle is the Arrow per-user cache and its token-removal-driven session teardown, which is not updated when the property changes. > * Configuration: No configuration item is added. `qe_max_connection` and the Arrow global cap are captured by their pools at FE startup, while the user property is durable and independently mutable. > * Compatibility: The persisted field is already a `long` and all-upgraded image/replay paths preserve it, but mixed-version edit-log replay is not backward compatible. > * Parallel paths: Classic MySQL enforces the current user limit; Arrow Flight does not. No third connection-admission path was found. > * Conditional validation: `Long.parseLong` plus the retained `<= 0` check correctly rejects non-numeric, overflowing, zero, and negative values. No separate positive-`long` overflow or narrowing bug was found downstream. > * Tests and expected results: The PR changes no test or expected-result file. Existing tests use 100, 2048, or mocked limit 2; they do not prove above-10000 behavior, mixed-version replay, or bidirectional live Arrow property changes. > * Observability: Existing metrics accept the full `long`, display paths use BIGINT/string output, and current replay/token logs expose the known failures. No separate observability defect was found. > * Persistence and failover: Normal writes and replay share the setter and image state stores the `long`, so upgraded-only failover is consistent. The old-FE replay exception path remains the blocking compatibility issue. > * Metadata/data writes: The metadata update validates local copies and assigns under the `Auth` write lock before the standard edit-log call; no table-data transaction path is involved. > * FE/BE protocol: No new FE-BE variable, symbol, or storage format is introduced. > * Performance: The changed validation itself adds no hot-path work or allocation. The Arrow cache behavior is a correctness/lifecycle issue rather than a newly identified separate performance finding. > * Other issues: No additional distinct issue survived end-to-end review and deduplication. > > User focus: no additional user-provided review focus was supplied; the full PR was reviewed. > > Validation was static only. Per the automated-review instructions, no build or test command was run. All normal and risk-focused review agents converged with `NO_NEW_VALUABLE_FINDINGS` after the existing threads were treated as duplicate fences. we can update max_user_connections after all fe upgrade, this is not big deal. The issue with the Arrow Flight connection pool caching is a legacy one, not something introduced in this iteration. -- 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]
