jerryshao commented on PR #13067: URL: https://github.com/apache/gravitino/pull/13067#issuecomment-5614969808
One more coverage gap. It's in a file outside this PR's diff, so I can't anchor it inline: `plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/IdpRESTUtils.java`. The idp-basic plugin has its own request helpers instead of the ones this PR instruments. `IdpRESTUtils.doAs` (`:50-67`) wraps the action in `catch (Exception e)` and turns the exception into a `Response` inside the plugin. It also has its own `internalError` (`:111`). Neither one goes through `Utils.doAs`, `Utils.internalError`, or `ServerHealth`. The plugin is active whenever the `basic` authenticator is configured. On such a server, suppose an IdP user or group REST call fails with a `RuntimeException` that wraps an `OutOfMemoryError`. The exception is caught at `:67` and converted to a response there. It never reaches an instrumented helper, a Jersey exception mapper, or the `ON_EXCEPTION` listener, so the marker is never set. A direct, unwrapped OOM is still recorded, because `catch (Exception)` doesn't catch `Error`s and the OOM propagates up to Jersey. The PR describes the "shared request helpers" as covered, and this parallel set was missed. The simplest fix is to call `ServerHealth.getInstance().recordFailure(e)` in that catch and in `internalError`, or have them delegate to `Utils`. -- 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]
