nevzheng opened a new issue, #13108: URL: https://github.com/apache/gravitino/issues/13108
### What would you like to be improved? With `gravitino.server.webserver.includeErrorStackTrace=false`, error responses omit the `stack` field, but the server still builds the stack text for every failed request. `ErrorResponse`'s factory methods call `getStackTrace(throwable)` unconditionally (formatting the stack into a string and splitting it into lines), and the `stack` field is only dropped when the response is written. Disabling the setting therefore saves response bytes but not the CPU and allocation cost of formatting stacks, which can be deep for exceptions with several causes. The JVM captures the stack when the exception is created either way; the avoidable cost is formatting it into text. This was raised in review of #13057 (https://github.com/apache/gravitino/pull/13057#discussion_r3976730059) and deferred there, because that PR changes what error responses display, not how they are built. The desired outcome is that failed requests don't pay to format stack traces when the setting is off, and that responses are unchanged in both modes. ### How should we improve? **Options:** - **A. Build the stack lazily in `ErrorResponse`:** keep the exception on the response (excluded from JSON and `equals`) and format the stack only when `getStack()` is read. With the setting off, the response writer skips `stack`, so the stack is never formatted. One file in `common/`, with no changes at call sites. Watch-outs: Lombok `equals`/`hashCode` and `toString` must not force formatting, and the response holds the exception until it is written. - **B. Pass the setting to every place that builds an error response:** about 135 `Utils.*` calls in the server plus direct `ErrorResponse.*` calls, or give the shared `ErrorResponse` class access to server config. Larger, easy to miss call sites, and moves enforcement away from the single response-writing point. - **C. Format the stack in the server's response writer:** `ErrorResponse` carries only the exception, and a server-side serializer next to `ObjectMapperProvider` formats `stack` only when the setting is on. Keeps the shared class a plain data holder, at the cost of more wiring than A. - **D. A process-wide switch in `ErrorResponse`,** set at server startup. A very small change, but global mutable state in shared code: tests must reset it, and servers sharing one JVM (for example Iceberg REST running inside the main server) would share one setting. - **E. Cap stack depth when stacks are enabled** (frames or causes). Reduces cost when the setting is on; it could be combined with A or C but does not address the disabled case alone. - **F. Leave as is:** the cost applies only to failed requests and is small next to creating, handling and logging the exception. Open question for triage: is the cost measurable enough to justify a change? Profiling the error path before choosing would answer that. Related: #12728, #13057, #13107. -- 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]
