[PR] Isolate per-event failures in OpenLineage batch ingest [polaris]

["adnanhemani (via GitHub)" <gi...@apache.org>]

adnanhemani opened a new pull request, #5573:
URL: https://github.com/apache/polaris/pull/5573

   The OpenLineage batch ingest endpoint called the ingest provider in a bare loop, so an unexpected `RuntimeException` from one event escaped the loop entirely:
   
   ```java
   for (int i = 0; i < events.size(); i++) {
     OpenLineageIngestResult result = ingestOne(events.get(i));
     switch (result) { ... }
   }
   ```
   
   `IcebergExceptionMapper` is registered as an unqualified `ExceptionMapper<RuntimeException>`, so it applies across `/api/openlineage/v1` too. That means one such throw became a **single error response for the whole batch**, discarding the per-event outcomes of everything else in the request — including events that had already been ingested successfully. A client had no way to learn which of those had landed, so the natural response is to resend the entire batch.
   
   `OpenLineageIngestProvider` is a pluggable CDI SPI, so a provider throwing something other than the outcomes it declares is a realistic failure mode rather than a hypothetical one.
   
   ### Changes
   
   Each event is now attempted independently. An unexpected throw is recorded as a failed entry at that index and the loop continues, so the existing `SUCCESS`/`PARTIAL`/`FAILURE` aggregation and the per-event `failedEvents` list describe what actually happened.
   
   Two deliberate choices worth review:
   
   - **Reported as non-retriable.** The provider contract already models the transient case as `UNAVAILABLE`, so an unexpected throw is more likely deterministic; marking it retriable would invite a client to replay the batch indefinitely. The exception is logged server-side rather than returned, so provider internals are not disclosed to the caller.
   - **Batch-only, by design.** The single-event endpoint has no sibling outcomes to protect, so a throw there still propagates to the exception mapper rather than being quietly converted into a success. A test pins that distinction so the isolation is not widened by accident.
   
   ### Testing
   
   Three unit tests in `OpenLineageAdapterTest`, all against a mocked provider:
   
   - one throwing event among three → `PARTIAL`, with the other two still counted successful, the failed entry at the right index, non-retriable, and the exception message not echoed to the caller
   - every event throwing → `FAILURE`
   - a throw on the single-event endpoint still propagates
   
   `:polaris-extensions-openlineage:test` passes with 14 tests, 0 failures; `spotlessCheck` and repo-wide `compileJava`/`compileTestJava` are clean. The `intTest` suite was not run locally — it could not start a server in my environment, which reproduces on an unmodified checkout, so it is unaffected by this change but unverified by me.
   
   ## Checklist
   - [x] 🛡️ Don't disclose security issues! (contact security@apache.org)
   - [x] 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
   - [x] 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
   - [x] 💡 Added comments for complex logic
   - [x] 🧾 Updated `CHANGELOG.md` (if needed)
   - [ ] 📚 Updated documentation in `site/content/in-dev/unreleased` (if needed)
   


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Isolate per-event failures in OpenLineage batch ingest [polaris]

["flyingImer (via GitHub)" <gi...@apache.org>]

flyingImer commented on code in PR #5573:
URL: https://github.com/apache/polaris/pull/5573#discussion_r4085604751


##########
extensions/openlineage/src/main/java/org/apache/polaris/service/lineage/OpenLineageAdapter.java:
##########
@@ -79,7 +83,24 @@ public Response sendLineageEventBatch(List<PolarisLineageEvent> events) {
     List<OpenLineageBatchIngestResponse.FailedEvent> failed = new ArrayList<>();
 
     for (int i = 0; i < events.size(); i++) {
-      OpenLineageIngestResult result = ingestOne(events.get(i));
+      OpenLineageIngestResult result;
+      try {
+        result = ingestOne(events.get(i));
+      } catch (RuntimeException e) {
+        // One bad event must not abort the batch. The ingest provider is a pluggable SPI that can
+        // throw anything, and the Iceberg exception mapper is registered globally across
+        // /api/openlineage/v1, so an escaping RuntimeException would otherwise be mapped into a
+        // single error response for the whole batch, losing the outcomes of every other event.
+        // Reported as non-retriable because the provider already models the transient case as
+        // UNAVAILABLE, so an unexpected throw is more likely deterministic; calling it retriable

Review Comment:
   Non-blocking: could we document on OpenLineageIngestProvider that implementations should return UNAVAILABLE for transient backend failures, and that exceptions escaping batch ingest are reported as non-retriable? That expectation is not currently part of the SPI documentation, so a provider that lets a transient failure escape would give the client a non-retriable result. I'm fine with keeping this fallback, but the existence of UNAVAILABLE alone does not establish that an unexpected exception is deterministic.
   



-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Isolate per-event failures in OpenLineage batch ingest [polaris]

["flyrain (via GitHub)" <gi...@apache.org>]

flyrain commented on code in PR #5573:
URL: https://github.com/apache/polaris/pull/5573#discussion_r4189571109


##########
extensions/openlineage/src/main/java/org/apache/polaris/service/lineage/OpenLineageAdapter.java:
##########
@@ -79,7 +83,24 @@ public Response sendLineageEventBatch(List<PolarisLineageEvent> events) {
     List<OpenLineageBatchIngestResponse.FailedEvent> failed = new ArrayList<>();
 
     for (int i = 0; i < events.size(); i++) {
-      OpenLineageIngestResult result = ingestOne(events.get(i));
+      OpenLineageIngestResult result;
+      try {
+        result = ingestOne(events.get(i));
+      } catch (RuntimeException e) {
+        // One bad event must not abort the batch. The ingest provider is a pluggable SPI that can
+        // throw anything, and the Iceberg exception mapper is registered globally across
+        // /api/openlineage/v1, so an escaping RuntimeException would otherwise be mapped into a
+        // single error response for the whole batch, losing the outcomes of every other event.
+        // Reported as non-retriable because the provider already models the transient case as
+        // UNAVAILABLE, so an unexpected throw is more likely deterministic; calling it retriable
+        // would invite a client to replay the batch forever. The exception is logged rather than
+        // returned, so provider internals are not disclosed to the caller.
+        LOGGER.warn("Lineage event at batch index {} failed unexpectedly", i, e);

Review Comment:
   A provider that's broken for every event will log one full stack trace per event at WARN, so a 1000-event batch means 1000 traces. Is that OK, or should we log the trace for the first failure and just count the rest?



##########
extensions/openlineage/src/main/java/org/apache/polaris/service/lineage/OpenLineageAdapter.java:
##########
@@ -79,7 +83,24 @@ public Response sendLineageEventBatch(List<PolarisLineageEvent> events) {
     List<OpenLineageBatchIngestResponse.FailedEvent> failed = new ArrayList<>();
 
     for (int i = 0; i < events.size(); i++) {
-      OpenLineageIngestResult result = ingestOne(events.get(i));
+      OpenLineageIngestResult result;
+      try {
+        result = ingestOne(events.get(i));
+      } catch (RuntimeException e) {

Review Comment:
   Not a blocker: Fine today, since only `provider.ingest` runs inside this try. But if per-event authorization moves into this loop later, a `ForbiddenException` would be reported as a generic non-retriable "Event could not be processed", and the client would never learn it was a permission issue. Should we let `ForbiddenException`/`WebApplicationException` through here, or is that a call for the authz follow-up PR?



##########
CHANGELOG.md:
##########
@@ -137,6 +137,13 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
 - Policy API: detaching a policy from a target it was never attached to now returns
   `404 Not Found` with error type `NoSuchMappingException`, as the policy API specification
   requires, instead of `500 Internal Server Error`.
+- A single failing event no longer aborts an entire OpenLineage batch ingest request. The batch

Review Comment:
   Nit: This entry is long for a fix. Can we cut it to the user-visible part? Something like: "One failing event no longer fails an entire OpenLineage batch ingest request. It's reported as a non-retriable failed entry for that event."



##########
extensions/openlineage/src/main/java/org/apache/polaris/service/lineage/OpenLineageAdapter.java:
##########
@@ -79,7 +83,24 @@ public Response sendLineageEventBatch(List<PolarisLineageEvent> events) {
     List<OpenLineageBatchIngestResponse.FailedEvent> failed = new ArrayList<>();
 
     for (int i = 0; i < events.size(); i++) {
-      OpenLineageIngestResult result = ingestOne(events.get(i));
+      OpenLineageIngestResult result;
+      try {
+        result = ingestOne(events.get(i));
+      } catch (RuntimeException e) {
+        // One bad event must not abort the batch. The ingest provider is a pluggable SPI that can

Review Comment:
   Nit: This comment mostly repeats the PR description. Can we trim it to the two whys: isolate per event, and non-retriable because UNAVAILABLE already covers the transient case?



-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org