[SPARK] introduce circuit breaker #2407

pawel-big-lebowski · 2024-02-01T11:19:56Z

Problem

Add mechanism to prevent from over-instrumentation which turns off Openlineage integration if specified criteria are met.

Closes: #1255

Solution

Implement circuit breaker config section within java-client,
Implement test circuit breaker, which opens/closes based on constructor param,
Implement Spark / Flink integration with an integration test based on test circuit breaker
Implement memory circuit breaker, which closes based on amount of free memory available
Implement Java runtime circuit breaker, which closes based on memory and time spent on Garbage Collection

Note: All schema changes require discussion. Please link the issue for context.

Your change modifies the core OpenLineage model
Your change modifies one or more OpenLineage facets

If you're contributing a new integration, please specify the scope of the integration and how/where it has been tested (e.g., Apache Spark integration supports S3 and GCS filesystem operations, tested with AWS EMR).

One-line summary:

Checklist

You've signed-off your work
Your pull request title follows our guidelines
Your changes are accompanied by tests (if relevant)
Your change contains a small diff and is self-contained
You've updated any relevant documentation (if relevant)
Your comment includes a one-liner for the changelog about the specific purpose of the change (if necessary)
You've versioned the core OpenLineage model or facets according to SchemaVer (if relevant)
You've added a header to source files (if relevant)

SPDX-License-Identifier: Apache-2.0
Copyright 2018-2023 contributors to the OpenLineage project

mobuchowski

Thanks @pawel-big-lebowski for the awesome work! I have a few comments.

mobuchowski · 2024-02-13T15:31:22Z

client/java/README.md

+More complex version of circuit breaker. The amount of free memory can be low as long as 
+amount of time spent on Garbage Collection is acceptable. `JavaRuntimeCircuitBreaker` closes
+when free memory drops below threshold and amount of time spent on garbage collection exceeds
+given threshold (`10%` by default). The circuit breaker is always open when checked for the first time


In general: I don't think we should have defaults here. If user hasn't configured any threshold, it should mean that we don't take this particular factor into account.

It's based on this circuit breaker https://docs.newrelic.com/docs/apm/agents/java-agent/custom-instrumentation/circuit-breaker-java-custom-instrumentation/ which has the same default values.

I think it makes sense for people to turn something on and experiment with values later when needed.

mobuchowski · 2024-02-13T15:34:28Z

client/java/src/main/java/io/openlineage/client/circuitBreaker/CircuitBreaker.java

+
+  boolean isClosed();
+
+  default boolean isOpen() {


isOpen and isClosed could provide additional information about reason of it being closed or open.

I think returning something some object that has both the boolean and the reason would be nice.

client/java/src/main/java/io/openlineage/client/circuitBreaker/CircuitBreakerResolver.java

mobuchowski · 2024-02-13T15:37:56Z

client/java/src/main/java/io/openlineage/client/circuitBreaker/TestCircuitBreaker.java

+import java.util.stream.Collectors;
+import lombok.NonNull;
+
+public class TestCircuitBreaker extends CommonCircuitBreaker {


Why not have TestCircuitBreaker in tests?

We use this for integration tests within Spark and Flink integration, so the class shall be part of a openlineage-java jar. I can rename it into StaticCircuitCreaker, because it closes/opens based on list of boolean values true, false,true provided.

mobuchowski · 2024-02-13T15:47:36Z

...a/src/main/java/io/openlineage/client/circuitBreaker/CircuitBreakerConfigTypeIdResolver.java

+    } else if (value instanceof TestCircuitBreaker) {
+      return CIRCUIT_BREAKER_TYPE_TEST;
+    }
+    throw new UnsupportedOperationException("Unsupported circuit breaker " + value);


Why can't we define custom CircuitBreaker similar to Transport?

If we want to allow that later but not do this in this PR then I'd document that

I don't like the ServiceLoader approach that we use for Transport. I agree the code with if/else chain has some code smell. I will introduce config builders as we have for transport. Custom circuit breaker is outside the scope of this PR.

client/java/src/main/java/io/openlineage/client/circuitBreaker/CommonCircuitBreaker.java

client/java/src/main/java/io/openlineage/client/circuitBreaker/JavaRuntimeCircuitBreaker.java

mobuchowski · 2024-02-13T16:13:41Z

client/java/src/main/java/io/openlineage/client/circuitBreaker/NoOpCircuitBreaker.java

+    try {
+      return callable.call();
+    } catch (Exception e) {
+      log.error("OpenLineage callable failed to execute. Swallowing the exception {}", e);


Swallowing the exception is not exactly no-op, do we really want that?

This can happen in case of some uncaught exception of OL integration. This should not happen, but if this is happens we should catch it. I think we want this.

Signed-off-by: Pawel Leszczynski <leszczynski.pawel@gmail.com>

* [SPARK][FLINK] introduce circuit breaker Signed-off-by: Pawel Leszczynski <leszczynski.pawel@gmail.com> * [SPARK] daemon circuit breaker Signed-off-by: Pawel Leszczynski <leszczynski.pawel@gmail.com> * [SPARK] pmd fix for java client Signed-off-by: Pawel Leszczynski <leszczynski.pawel@gmail.com> --------- Signed-off-by: Pawel Leszczynski <leszczynski.pawel@gmail.com>

boring-cyborg bot added client/java integration/spark labels Feb 1, 2024

pawel-big-lebowski force-pushed the spark/circuit-breaker branch 3 times, most recently from f74e9c8 to 73cc67b Compare February 5, 2024 13:49

pawel-big-lebowski mentioned this pull request Feb 6, 2024

[SPARK] adds timeout for event processing in spark listener #2392

Closed

8 tasks

pawel-big-lebowski force-pushed the spark/circuit-breaker branch from 73cc67b to 1a19ca5 Compare February 8, 2024 10:30

boring-cyborg bot added documentation Improvements or additions to documentation integration/flink streaming labels Feb 8, 2024

pawel-big-lebowski force-pushed the spark/circuit-breaker branch 10 times, most recently from 1850b8c to 3030482 Compare February 12, 2024 12:24

pawel-big-lebowski marked this pull request as ready for review February 12, 2024 12:24

pawel-big-lebowski force-pushed the spark/circuit-breaker branch 2 times, most recently from 0135cde to 70dff95 Compare February 12, 2024 14:39

pawel-big-lebowski requested a review from mobuchowski February 13, 2024 07:04

pawel-big-lebowski force-pushed the spark/circuit-breaker branch from 70dff95 to 4ce044f Compare February 13, 2024 09:23

mobuchowski requested changes Feb 13, 2024

View reviewed changes

pawel-big-lebowski force-pushed the spark/circuit-breaker branch 4 times, most recently from fd2200d to 1668468 Compare February 14, 2024 09:55

pawel-big-lebowski force-pushed the spark/circuit-breaker branch from 1668468 to 75f8a32 Compare February 14, 2024 10:38

pawel-big-lebowski added 3 commits February 14, 2024 19:42

[SPARK][FLINK] introduce circuit breaker

8e2624e

Signed-off-by: Pawel Leszczynski <leszczynski.pawel@gmail.com>

[SPARK] daemon circuit breaker

ae03476

Signed-off-by: Pawel Leszczynski <leszczynski.pawel@gmail.com>

[SPARK] pmd fix for java client

a1ecfed

Signed-off-by: Pawel Leszczynski <leszczynski.pawel@gmail.com>

pawel-big-lebowski force-pushed the spark/circuit-breaker branch from 75f8a32 to a1ecfed Compare February 14, 2024 18:43

pawel-big-lebowski requested a review from mobuchowski February 14, 2024 18:55

mobuchowski approved these changes Feb 15, 2024

View reviewed changes

pawel-big-lebowski merged commit 3dad978 into main Feb 15, 2024
30 checks passed

pawel-big-lebowski deleted the spark/circuit-breaker branch February 15, 2024 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK] introduce circuit breaker #2407

[SPARK] introduce circuit breaker #2407

pawel-big-lebowski commented Feb 1, 2024 •

edited

mobuchowski left a comment

mobuchowski Feb 13, 2024

pawel-big-lebowski Feb 14, 2024

mobuchowski Feb 13, 2024

mobuchowski Feb 13, 2024

pawel-big-lebowski Feb 14, 2024

mobuchowski Feb 13, 2024

pawel-big-lebowski Feb 14, 2024

mobuchowski Feb 13, 2024

pawel-big-lebowski Feb 14, 2024

[SPARK] introduce circuit breaker #2407

[SPARK] introduce circuit breaker #2407

Conversation

pawel-big-lebowski commented Feb 1, 2024 • edited

Problem

Solution

One-line summary:

Checklist

mobuchowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawel-big-lebowski commented Feb 1, 2024 •

edited