snazy commented on PR #3803: URL: https://github.com/apache/polaris/pull/3803#issuecomment-4032799492
I prefer to see the whole solution in one PR. There are still some issues, many with a very high severity that have to be solved before we can continue with this PR and #3915. * Unlimited response body size * "Check then act" issue (InMemoryIdempotencyStore.reserve() L90) * Non-atomic read-modify-write (InMemoryIdempotencyStore.load() L130) * Missing database level concurrency control (RelationalJdbcIdempotencyStore.updateHeartbeat() L86) * Potential lost update (RelationalJdbcIdempotencyStore.finalizeRecord(), whole function) * Heartbeat race conditions in multi-instance scenarios (IdempotencyFilter.maybeStartHeartbeat(), whole function) * Insufficient stale owner detection (IdempotencyFilter.reserveOrReplay() ~L230) * Polling implementation can cause database overload (polling frequency 50ms - 100 duplicate requests lead to 2000 DB requests per second) * Missing transaction boundaries in JDBC Operations (relies on auto-commit) * Still clock skew issues - even small-ish drifts of 10s lead to premature or delayed execution * Error handling Swallows important exceptions and doesn't propagate (e.g. IdempotencyFilter.finalizeOwned --> cancelInProgressReservation) leading to records remaining in the backend store, in turn wrong responses to duplicate requests. * Expiry in the in-memory impl can still run into OOMs - it just needs 255 bigger requests. * Polling is hardcoded, not configurable * JDBC is likely missing important database indexes -- 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]
