jerryshao commented on code in PR #13067:
URL: https://github.com/apache/gravitino/pull/13067#discussion_r3976714176
##########
docs/health-and-readiness.md:
##########
@@ -54,7 +57,44 @@ endpoint rather than to a check of its own.
The response body carries an overall status and a list of individual checks.
Each check has a name,
a status of UP or DOWN, and a details map that explains a failure. On the
Gravitino server the two
-check names are `httpServer` and `entityStore`.
+normal check names are `httpServer` and `entityStore`. After an observed
out-of-memory error, all
+three endpoints instead report the `jvm` failure described below.
+
+## Out-of-memory Failures
+
+A Metaspace or heap `OutOfMemoryError` can leave already-loaded endpoints
responding successfully
+while other operations fail. A successful HTTP response or entity-store lookup
therefore does not
+prove recovery after OOM.
+
+The Gravitino, Iceberg REST, and Lance REST servers record OOM observed by
their Jersey exception
+listeners, error mappers, shared request execution/error-response helpers, and
Jetty worker
+uncaught-exception handlers. The main server also records failures in
health-probe tasks.
+Wrapped causes are checked too. Once recorded, the affected service’s health
endpoints and root
+aliases return HTTP 503 with this body (the main server uses the `/api/health`
prefix):
+
+```json
+{
+ "code": 0,
+ "status": "down",
+ "checks": [
+ {
+ "name": "jvm",
+ "status": "down",
+ "details": { "reason": "OutOfMemoryError; restart required" }
+ }
+ ]
+}
+```
+
+This state lasts until process restart, even if subsequent ordinary API
requests succeed. Health
+checks skip the entity-store probe once OOM is recorded. A database outage,
ordinary HTTP 500,
+`StackOverflowError`, or missing connector class alone does not set this state.
+
+Detection covers errors reaching these server boundaries; it cannot detect an
OOM swallowed
+entirely by a connector or unrelated background executor. This is not a
JVM-wide OOM trap. If the
+JVM cannot allocate enough memory to answer a probe, the probe may fail
without a JSON response.
+Each service tracks errors observed within its own runtime. Auxiliary services
with isolated
Review Comment:
Doc accuracy: this sentence says the opposite of what the code does. When
Iceberg REST and Lance REST run embedded in the main server, all three services
share one `ServerHealth` marker.
- `AuxiliaryServiceManager` builds each auxiliary classloader with
`IsolatedClassLoader.buildClassLoader`, which passes empty shared and barrier
lists (`IsolatedClassLoader.java:170-171`).
- `isSharedClass` returns true for every class where `!isCatalogClass(name)`
holds (`:273`). `isCatalogClass` only matches `org.apache.gravitino.hive.*` and
a fixed list of `org.apache.gravitino.catalog.*` prefixes, so
`org.apache.gravitino.server.web.ServerHealth` is treated as shared.
- Shared classes are loaded from `baseClassLoader` first (`:238-244`). That
is the server's context loader, captured at construction (`:71`), and it
already has `server-common` on its classpath (`server/build.gradle.kts:31`).
The result is a single static `INSTANCE`. An OOM recorded by the main server
on 8090 makes embedded `/iceberg/health/live` on 9001 return 503, and the
reverse also happens.
I think sharing is actually the right behaviour: it's one JVM, and an OOM
anywhere degrades all of it. So the fix belongs in this paragraph, not in the
code. The PR description has the same inaccuracy ("Auxiliary services with
isolated classloaders track their own observed errors"). As written, the
paragraph could lead operators to expect a probe on one port to stay green when
a co-located service hits OOM.
##########
docs/health-and-readiness.md:
##########
@@ -54,7 +57,44 @@ endpoint rather than to a check of its own.
The response body carries an overall status and a list of individual checks.
Each check has a name,
a status of UP or DOWN, and a details map that explains a failure. On the
Gravitino server the two
-check names are `httpServer` and `entityStore`.
+normal check names are `httpServer` and `entityStore`. After an observed
out-of-memory error, all
+three endpoints instead report the `jvm` failure described below.
+
+## Out-of-memory Failures
+
+A Metaspace or heap `OutOfMemoryError` can leave already-loaded endpoints
responding successfully
+while other operations fail. A successful HTTP response or entity-store lookup
therefore does not
+prove recovery after OOM.
+
+The Gravitino, Iceberg REST, and Lance REST servers record OOM observed by
their Jersey exception
+listeners, error mappers, shared request execution/error-response helpers, and
Jetty worker
+uncaught-exception handlers. The main server also records failures in
health-probe tasks.
+Wrapped causes are checked too. Once recorded, the affected service’s health
endpoints and root
+aliases return HTTP 503 with this body (the main server uses the `/api/health`
prefix):
+
+```json
+{
+ "code": 0,
+ "status": "down",
Review Comment:
Doc accuracy: this example body is presented as the response for all three
services, but the `status` casing differs by server.
`HealthCheckDTO.Status` is a plain enum (`UP`, `DOWN`) with no `@JsonValue`,
so its JSON casing depends on each server's `ObjectMapper`:
- **Main server:** `ObjectMapperProvider` enables
`WRITE_ENUMS_TO_LOWERCASE`, so it emits `"down"`. `TestOutOfMemoryHealthHttp`
asserts exactly this.
- **Lance REST:** its mapper copies that configuration, so it also emits
`"down"`.
- **Iceberg REST:** it registers `IcebergObjectMapperProvider`, and
`IcebergObjectMapper` only configures field visibility,
`FAIL_ON_UNKNOWN_PROPERTIES`, kebab-case naming, and `RESTSerializers`
(`IcebergObjectMapper.java:32-39`). There is no lowercase-enum setting, so it
emits `"status": "DOWN"`.
Anyone who writes a probe or alert rule that matches `"status": "down"`
case-sensitively from this example will never see a match on an Iceberg REST
server that has recorded an OOM.
The PR's own `gravitino-server-config.md` text has the opposite problem: it
shows `jvm: DOWN` in uppercase for the main server, which actually emits
lowercase.
Suggestion: show the casing per service, or tell readers to match
case-insensitively, and make `gravitino-server-config.md` consistent with it.
--
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]
