Support TE connection dynamic config + fix heartbeat deadlock #585

Andyz26 · 2023-11-13T21:32:45Z

Context

Add TE-level dynamic configs for connection interval/timeout.

Refactor resource gateway connection on TE so that the startup call doesn't deadlock on the main thread when calling "getCurrentReport" to build heartbeat payload. Move both registration and HB calls into service runIteration.

Also, remove the "callAsync" on TaskExecutor's getCurrentReport method which caused a deadlock timeout on TE reconnection (where resource gateway cxn tries to create HB request on the main thread while the main thread is on "cxn.startAsync().awaitRunning()").
This might cause stale HB info to the control plane in a race scenario but should be recoverable on the control plane side with retry.

Checklist

./gradlew build compiles code correctly
Added new tests where applicable
./gradlew test passes all tests
Extended README or added javadocs where applicable

github-actions · 2023-11-13T21:47:02Z

Test Results

130 files +1 130 suites +1 8m 11s ⏱️ +46s
548 tests +2 539 ✔️ +1 8 💤 ±0 1 ❌ +1
549 runs +3 540 ✔️ +2 8 💤 ±0 1 ❌ +1

For more details on these failures, see this check.

Results for commit 94d2c5c. ± Comparison against base commit 81bce54.

♻️ This comment has been updated with latest results.

github-actions · 2023-11-13T21:49:54Z

Uploaded Artifacts

To use these artifacts in your Gradle project, paste the following lines in your build.gradle.

resolutionStrategy {
    force "io.mantisrx:mantis-client:0.1.0-20231114.042158-442"
    force "io.mantisrx:mantis-common:0.1.0-20231114.042158-441"
    force "io.mantisrx:mantis-common-serde:0.1.0-20231114.042158-441"
    force "io.mantisrx:mantis-discovery-proto:0.1.0-20231114.042158-441"
    force "io.mantisrx:mantis-network:0.1.0-20231114.042158-441"
    force "io.mantisrx:mantis-remote-observable:0.1.0-20231114.042158-442"
    force "io.mantisrx:mantis-runtime:0.1.0-20231114.042158-442"
    force "io.mantisrx:mantis-runtime-loader:0.1.0-20231114.042158-442"
    force "io.mantisrx:mantis-shaded:0.1.0-20231114.042158-440"
    force "io.mantisrx:mantis-testcontainers:0.1.0-20231114.042158-111"
    force "io.mantisrx:mantis-connector-iceberg:0.1.0-20231114.042158-440"
    force "io.mantisrx:mantis-connector-job:0.1.0-20231114.042158-442"
    force "io.mantisrx:mantis-connector-kafka:0.1.0-20231114.042158-442"
    force "io.mantisrx:mantis-connector-publish:0.1.0-20231114.042158-441"
    force "io.mantisrx:mantis-control-plane-client:0.1.0-20231114.042158-441"
    force "io.mantisrx:mantis-control-plane-core:0.1.0-20231114.042158-435"
    force "io.mantisrx:mantis-control-plane-server:0.1.0-20231114.042158-435"
    force "io.mantisrx:mantis-examples-core:0.1.0-20231114.042158-435"
    force "io.mantisrx:mantis-examples-groupby-sample:0.1.0-20231114.042158-435"
    force "io.mantisrx:mantis-examples-jobconnector-sample:0.1.0-20231114.042158-435"
    force "io.mantisrx:mantis-examples-mantis-publish-sample:0.1.0-20231114.042158-435"
    force "io.mantisrx:mantis-examples-sine-function:0.1.0-20231114.042158-435"
    force "io.mantisrx:mantis-examples-synthetic-sourcejob:0.1.0-20231114.042158-435"
    force "io.mantisrx:mantis-examples-twitter-sample:0.1.0-20231114.042158-435"
    force "io.mantisrx:mantis-examples-wordcount:0.1.0-20231114.042158-435"
    force "io.mantisrx:mantis-publish-core:0.1.0-20231114.042158-435"
    force "io.mantisrx:mantis-publish-netty:0.1.0-20231114.042158-434"
    force "io.mantisrx:mantis-publish-netty-guice:0.1.0-20231114.042158-435"
    force "io.mantisrx:mantis-server-agent:0.1.0-20231114.042158-435"
    force "io.mantisrx:mantis-server-worker:0.1.0-20231114.042158-435"
    force "io.mantisrx:mantis-server-worker-client:0.1.0-20231114.042158-435"
    force "io.mantisrx:mantis-source-job-kafka:0.1.0-20231114.042158-435"
    force "io.mantisrx:mantis-source-job-publish:0.1.0-20231114.042158-435"
}

sundargates · 2023-11-14T20:45:05Z

...er/mantis-server-agent/src/main/java/io/mantisrx/server/agent/ResourceManagerGatewayCxn.java

-                0,
-                heartBeatInterval.getSize(),
-                heartBeatInterval.getUnit());
+        return new CustomScheduler() {


Where is this defined?

it's from google common lib

This is really nice - I think this is probably what we should use for ExponentialBackoffAbstractScheduledService.

cc @fdc-ntflx

...er/mantis-server-agent/src/main/java/io/mantisrx/server/agent/ResourceManagerGatewayCxn.java

sundargates · 2023-11-15T17:30:21Z

...er/mantis-server-agent/src/main/java/io/mantisrx/server/agent/ResourceManagerGatewayCxn.java

-                0,
-                heartBeatInterval.getSize(),
-                heartBeatInterval.getUnit());
+        return new CustomScheduler() {


This is really nice - I think this is probably what we should use for ExponentialBackoffAbstractScheduledService.

sundargates · 2023-11-15T17:49:30Z

mantis-common/src/main/java/io/mantisrx/common/properties/DynamicProperty.java

+ * limitations under the License.
+ */
+
+package io.mantisrx.common.properties;


Can we move this to mantis-config instead?

what is "mantis-config"?

I am proposing moving these classes under a package that's specific to configuration such as io.mantisrx.config.dynamic. Currently, this class is included in the mantis-common package. However, I believe it would be more efficient to utilize a gradle module agnostic approach. This would enable us to refactor or divide mantis-common into multiple modules with ease. As a result, users of this class would only need to modify the location from which they retrieve the code, rather than modifying the import statement as well.

sundargates · 2023-11-15T17:49:46Z

...er/mantis-server-agent/src/main/java/io/mantisrx/server/agent/ResourceManagerGatewayCxn.java

@@ -60,6 +63,7 @@ class ResourceManagerGatewayCxn extends ExponentialBackoffAbstractScheduledServi
    @Getter
    private volatile boolean registered = false;

+    private boolean hasRan = false;


i think there is no actual multi-thread access for this one?

...er/mantis-server-agent/src/main/java/io/mantisrx/server/agent/ResourceManagerGatewayCxn.java

sundargates · 2023-11-15T20:08:27Z

mantis-server/mantis-server-agent/src/main/java/io/mantisrx/server/agent/TaskExecutor.java

@@ -194,6 +193,9 @@ public TaskExecutor(
        this.registeredState = new DurableBooleanState(
            new File(workerConfiguration.getRegistrationStoreDir(),
                "rmCxnState.txt").getAbsolutePath());
+        this.rpcCallTimeoutMsDp =
+            ConfigUtils.getDynamicPropertyLong("heartbeatTimeoutMs", WorkerConfiguration.class,
+                workerConfiguration.heartbeatTimeoutMs(), this.dynamicPropertiesLoader);


It looks like we define this property in both places. Wondering if we can just have this defined in one place to avoid confusion.

[Discussed offline]: leaving as it is to avoid duplicating the skife config annotations.

sundargates · 2023-11-15T20:08:40Z

mantis-server/mantis-server-agent/src/main/java/io/mantisrx/server/agent/TaskExecutor.java

@@ -307,13 +309,21 @@ private ResourceManagerGatewayCxn newResourceManagerCxn() {
        ResourceClusterGateway resourceManagerGateway = resourceClusterGatewaySupplier.getCurrent();

        // let's register ourselves with the resource manager
-        // todo: move timeout/retry to apply values from this.dynamicPropertiesLoader
+        LongDynamicProperty heartbeatIntervalDp =


Same as above.

sundargates · 2023-11-15T20:10:19Z

mantis-common/src/main/java/io/mantisrx/common/properties/MantisPropertiesLoader.java

-    public abstract void initalize();
+    void initalize();

-    public abstract void shutdown();
+    void shutdown();


Can we get rid of these methods in the interface.

sundargates · 2023-11-15T20:12:05Z

mantis-common/src/main/java/io/mantisrx/common/properties/DynamicProperty.java

+    protected final T defaultValue;
+    protected T lastValue;
+    protected Instant lastRefreshTime;
+    private final long refreshDuration;


nit: Use java::util::Duration or reflect the unit in the name.

sundargates · 2023-11-15T22:33:55Z

mantis-common/src/main/java/io/mantisrx/common/properties/DynamicProperty.java

+ * limitations under the License.
+ */
+
+package io.mantisrx.common.properties;


I am proposing moving these classes under a package that's specific to configuration such as io.mantisrx.config.dynamic. Currently, this class is included in the mantis-common package. However, I believe it would be more efficient to utilize a gradle module agnostic approach. This would enable us to refactor or divide mantis-common into multiple modules with ease. As a result, users of this class would only need to modify the location from which they retrieve the code, rather than modifying the import statement as well.

sundargates · 2023-11-15T22:34:51Z

mantis-common/src/main/java/io/mantisrx/common/properties/DynamicProperty.java

+    protected T lastValue;
+    protected Instant lastRefreshTime;
+    private final long refreshDuration;
+    private final Clock clock;


sundargates · 2023-11-15T22:35:15Z

mantis-common/src/main/java/io/mantisrx/common/properties/DynamicProperty.java

+            this.refreshDuration = Long.parseLong(
+                propertiesLoader.getStringValue(DYNAMICPROPERTY_REFRESH_SECONDS_KEY, "30"));
+        }
+        catch (NumberFormatException ex) {


nit: move this to the previous line.

sundargates · 2023-11-15T22:37:28Z

mantis-common/src/main/java/io/mantisrx/common/properties/DynamicProperty.java

+        return this.propertiesLoader.getStringValue(this.propertyName, this.lastValue.toString());
+    }
+
+    protected boolean shouldRefresh() {


Is there a specific reason why you want to expose 'shouldRefresh' to child classes? Instead, you can perform the check in this class and only reach out to the subclass when necessary to obtain the value. This approach eliminates the need to duplicate this logic in all subclasses.

sundargates · 2023-11-15T22:38:23Z

...plane/mantis-control-plane-core/src/main/java/io/mantisrx/server/core/utils/ConfigUtils.java

+import java.lang.reflect.Method;
+import org.skife.config.Config;
+
+public class ConfigUtils {


nit: I would move this under the config package too.

this one has a dep to skife (which is why it's not in common module in the first place) and it's better to keep the Skife-related parts on a higher level module so it doesn't pollute the whole hierarchy.

Andyz26 added 2 commits November 12, 2023 21:28

connection config FP

c072a9b

cleanup

9fddfd6

Andyz26 requested review from nickmahilani, jeffchao, piygoyal, calvin681, sundargates, hmitnflx, markcho, liuml07 and fdc-ntflx as code owners November 13, 2023 21:32

Andyz26 temporarily deployed to Integrate Pull Request November 13, 2023 21:32 — with GitHub Actions Inactive

Andyz26 changed the title ~~Andyz/te config on fp~~ Support TE connection dynamic config Nov 13, 2023

try fix reconnection

3507a61

Andyz26 temporarily deployed to Integrate Pull Request November 13, 2023 23:51 — with GitHub Actions Inactive

fix init load

4c89ba9

Andyz26 had a problem deploying to Integrate Pull Request November 14, 2023 00:37 — with GitHub Actions Failure

cleanup log

d39d6eb

Andyz26 had a problem deploying to Integrate Pull Request November 14, 2023 01:41 — with GitHub Actions Failure

Andyz26 had a problem deploying to Integrate Pull Request November 14, 2023 03:25 — with GitHub Actions Failure

refactor cxn startup

069aec3

Andyz26 force-pushed the andyz/teConfigOnFP branch from bbfe915 to 069aec3 Compare November 14, 2023 04:17

Andyz26 temporarily deployed to Integrate Pull Request November 14, 2023 04:17 — with GitHub Actions Inactive

sundargates reviewed Nov 14, 2023

View reviewed changes

Merge branch 'master' into andyz/teConfigOnFP

65fe0bd

Andyz26 had a problem deploying to Integrate Pull Request November 15, 2023 00:02 — with GitHub Actions Failure

sundargates reviewed Nov 15, 2023

View reviewed changes

Andyz26 changed the title ~~Support TE connection dynamic config~~ Support TE connection dynamic config + fix heartbeat deadlock Nov 15, 2023

comments

165a529

Andyz26 had a problem deploying to Integrate Pull Request November 15, 2023 20:02 — with GitHub Actions Failure

sundargates reviewed Nov 15, 2023

View reviewed changes

sundargates approved these changes Nov 15, 2023

View reviewed changes

Andyz26 had a problem deploying to Integrate Pull Request November 16, 2023 02:10 — with GitHub Actions Failure

rename + comments

94d2c5c

Andyz26 force-pushed the andyz/teConfigOnFP branch from 72786e6 to 94d2c5c Compare November 16, 2023 03:27

Andyz26 had a problem deploying to Integrate Pull Request November 16, 2023 03:27 — with GitHub Actions Failure

Andyz26 merged commit b8ceac2 into master Nov 16, 2023
3 of 5 checks passed

Andyz26 deleted the andyz/teConfigOnFP branch November 16, 2023 05:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support TE connection dynamic config + fix heartbeat deadlock #585

Support TE connection dynamic config + fix heartbeat deadlock #585

Andyz26 commented Nov 13, 2023 •

edited

Loading

github-actions bot commented Nov 13, 2023 •

edited

Loading

github-actions bot commented Nov 13, 2023 •

edited

Loading

sundargates Nov 14, 2023

Andyz26 Nov 14, 2023

sundargates Nov 15, 2023

sundargates Nov 15, 2023

sundargates Nov 15, 2023

sundargates Nov 15, 2023

Andyz26 Nov 15, 2023

sundargates Nov 15, 2023

sundargates Nov 15, 2023

Andyz26 Nov 15, 2023

sundargates Nov 15, 2023

Andyz26 Nov 16, 2023

sundargates Nov 15, 2023

sundargates Nov 15, 2023

sundargates Nov 15, 2023

sundargates Nov 15, 2023

sundargates Nov 15, 2023

sundargates Nov 15, 2023

sundargates Nov 15, 2023

sundargates Nov 15, 2023

Andyz26 Nov 16, 2023

Support TE connection dynamic config + fix heartbeat deadlock #585

Support TE connection dynamic config + fix heartbeat deadlock #585

Conversation

Andyz26 commented Nov 13, 2023 • edited Loading

Context

Checklist

github-actions bot commented Nov 13, 2023 • edited Loading

Test Results

github-actions bot commented Nov 13, 2023 • edited Loading

Uploaded Artifacts

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

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

Choose a reason for hiding this comment

Andyz26 commented Nov 13, 2023 •

edited

Loading

github-actions bot commented Nov 13, 2023 •

edited

Loading

github-actions bot commented Nov 13, 2023 •

edited

Loading