yuqi1129 commented on PR #13067: URL: https://github.com/apache/gravitino/pull/13067#issuecomment-5618693028
> Thanks for the updates. I re-reviewed the latest commit and the previous comments are addressed. I found no new correctness issues. Two non-blocking notes: > > 1. **A request-local OOM permanently fails liveness.** Any `OutOfMemoryError` that reaches a recording boundary marks the process unhealthy until restart. That includes one that comes from a single oversized allocation, e.g. a very large list response or `Requested array size exceeds VM limit`, even when the heap recovers after GC. If a client or load balancer keeps retrying such a request, replicas could restart one after another. This matches `-XX:+ExitOnOutOfMemoryError` and seems to be the intent of #13066, but it would be good to confirm this blast radius is acceptable, or to call it out in `docs/health-and-readiness.md`. > 2. **The HTTP tests don't exercise the production wiring.** `TestOutOfMemoryHealthHttp` builds its own `ResourceConfig` and filter chain: > > * `RequestContextFilter`, `HttpAuditFilter` and `VersioningFilter` are not installed. > * `ErrorExceptionMapper` is registered as an instance, but `GravitinoServer` registers it by class. Jersey then has to pick the no-arg constructor now that a `ServerHealth` constructor also exists. > > From reading the code both paths work: `HttpAuditFilter` rethrows, and Jersey won't pick the constructor with the unannotated parameter. But no test pins this down. A test against the real `GravitinoServer` filter and provider setup would guard against regressions. 1. Added the document to illustrate the limitation mentioned above. 2. Add a test case that covers the cases, if possible. -- 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]
