[SPARK-44241][Core] Mistakenly set io.connectionTimeout/connectionCreationTimeout to zero or negative will cause incessant executor cons/destructions #41785

yaooqinn · 2023-06-29T08:18:13Z

What changes were proposed in this pull request?

This PR makes zero when io.connectionTimeout/connectionCreationTimeout is negative. Zero here means

connectionCreationTimeout = 0，an unlimited CONNNETION_TIMEOUT for connection establishment
connectionTimeout=0， IdleStateHandler for triggering IdleStateEvent is disabled.

Why are the changes needed?

This PR fixes a bug when connectionCreationTimeout is 0, which means unlimited to netty, but ChannelFuture.await(0) fails directly and inappropriately.
This PR fixes a bug when connectionCreationTimeout is less than 0, which causes meaningless transport client reconnections and endless executor reconstructions

Does this PR introduce any user-facing change?

no

How was this patch tested?

new unit tests

…ationTimeout to zero or negative will cause incessant executor cons/destructions

...etwork-common/src/test/java/org/apache/spark/network/client/TransportClientFactorySuite.java

yaooqinn · 2023-06-29T11:15:34Z

cc @cloud-fan @dongjoon-hyun thanks

dongjoon-hyun

Thank you for pinging me, @yaooqinn .

dongjoon-hyun · 2023-06-29T16:11:52Z

common/network-common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java

@@ -276,10 +277,19 @@ public void initChannel(SocketChannel ch) {
    // Connect to the remote server
    long preConnect = System.nanoTime();
    ChannelFuture cf = bootstrap.connect(address);
-    if (!cf.await(conf.connectionCreationTimeoutMs())) {
+
+    if (connCreateTimeout <= 0) {


Although negative cases are prevented already via connectionCreationTimeoutMs changes, this condition looks safer.

dongjoon-hyun · 2023-06-29T16:12:30Z

...etwork-common/src/test/java/org/apache/spark/network/client/TransportClientFactorySuite.java

@@ -237,4 +235,31 @@ public void fastFailConnectionInTimeWindow() {
    Assert.assertThrows("fail this connection directly", IOException.class,
      () -> factory.createClient(TestUtils.getLocalHost(), unreachablePort, true));
  }
+
+  @Test
+  public void unlimitedConnectionAndCreationTimeouts() throws IOException, InterruptedException {


Thank you for adding this.

dongjoon-hyun · 2023-06-29T16:14:39Z

cc @mridulm

yaooqinn · 2023-06-30T01:52:29Z

thanks @dongjoon-hyun for the check

mridulm · 2023-06-30T07:37:03Z

Instead of trying to work around the issue - why not simply fail when invalid config (<= 0 in this case) is specified ?
Within core, for example, invalid config values result in checkValue throwing IllegalArgumentException and so app failure.

yaooqinn · 2023-06-30T07:56:51Z

Hi @mridulm,

These network settings prefixed by different modules are not registered as TypedConfigBuilder for checkValue.

Usually, negative values for timeouts in spark are valid, e.g. spark.sql.streaming.stopTimeout(0 or negative values wait indefinitely) and here also at least 90% valid for the underlying network protocol.

mridulm · 2023-06-30T08:14:24Z

To clarify, I am not saying fail on all <= 0 values - but fail where relevant, instead of trying to workaround it.

If negative value or zero makes sense for a config - that is perfectly fine.

I used checkValue as an example to show the behavior, not impl :-)

yaooqinn · 2023-06-30T09:24:04Z

Thanks @mridulm

If negative value or zero makes sense for a config - that is perfectly fine.

I'm +1 w/ it.

As checking these networking settings is not very convenient on the driver side before scheduler starts, if you don't have strong option for this, I'd like to merge this.

yaooqinn · 2023-06-30T09:28:18Z

Just FYI, https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/timeout/IdleStateHandler.java#L198

For io.connectionTimeouts, this is also adjusted by netty. I guess it forgots to do this for io.connectionCreationTimeout, a.k.a CONNNETION_TIMEOUT

…ationTimeout to zero or negative will cause incessant executor cons/destructions ### What changes were proposed in this pull request? This PR makes zero when io.connectionTimeout/connectionCreationTimeout is negative. Zero here means - connectionCreationTimeout = 0，an unlimited CONNNETION_TIMEOUT for connection establishment - connectionTimeout=0， `IdleStateHandler` for triggering `IdleStateEvent` is disabled. ### Why are the changes needed? 1. This PR fixes a bug when connectionCreationTimeout is 0, which means unlimited to netty, but ChannelFuture.await(0) fails directly and inappropriately. 2. This PR fixes a bug when connectionCreationTimeout is less than 0, which causes meaningless transport client reconnections and endless executor reconstructions ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? new unit tests Closes #41785 from yaooqinn/SPARK-44241. Authored-by: Kent Yao <yao@apache.org> Signed-off-by: Kent Yao <yao@apache.org> (cherry picked from commit 38645fa) Signed-off-by: Kent Yao <yao@apache.org>

yaooqinn · 2023-06-30T10:34:01Z

thanks @dongjoon-hyun @mridulm @Hisoka-X for the check, merged to master/3.4/3.3

…ationTimeout to zero or negative will cause incessant executor cons/destructions ### What changes were proposed in this pull request? This PR makes zero when io.connectionTimeout/connectionCreationTimeout is negative. Zero here means - connectionCreationTimeout = 0，an unlimited CONNNETION_TIMEOUT for connection establishment - connectionTimeout=0， `IdleStateHandler` for triggering `IdleStateEvent` is disabled. ### Why are the changes needed? 1. This PR fixes a bug when connectionCreationTimeout is 0, which means unlimited to netty, but ChannelFuture.await(0) fails directly and inappropriately. 2. This PR fixes a bug when connectionCreationTimeout is less than 0, which causes meaningless transport client reconnections and endless executor reconstructions ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? new unit tests Closes #41785 from yaooqinn/SPARK-44241. Authored-by: Kent Yao <yao@apache.org> Signed-off-by: Kent Yao <yao@apache.org> (cherry picked from commit 38645fa) Signed-off-by: Kent Yao <yao@apache.org>

mridulm · 2023-07-01T03:36:54Z

common/network-common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java

@@ -245,12 +245,13 @@ TransportClient createClient(InetSocketAddress address)
    logger.debug("Creating new connection to {}", address);

    Bootstrap bootstrap = new Bootstrap();
+    int connCreateTimeout = conf.connectionCreationTimeoutMs();


I would have simply thrown an exception when connCreateTimeout <= 0 - and we can avoid the rest of the PR.
Will that not work ?

w/o this PR, here we simply throw an exception, it doesn't work

Yes, it will throw the exception and user will fix invalid config first time they run: exact same behavior as with checkValue

Not the same; checkvalue fails app in driver ahead and tells users what is invalid. He we fail during executor allocating. Users may not encounter exceptions, or exceptions and config are suppressed

This failure is in createClient (when introduced) - and will fail all invocations for that module - which will in turn fail the task/stage/job. This is similar in behavior to a configuration referenced in executor which fails in checkValue.
The failure reason will inform the user about the invalid configuration.

Do you have a scenario where an exception thrown in this codepath does not result in failure ?

Note that createClient failures of executor startup will not result in task fail. The executor will fail after max io retries, and then the app attempt will fail appMaster after the max number of executor failures is reached, and there are still many app attempts waiting for us to retry. It could be hours before you notice the app fail without the driver log telling what mistake we've made.

@yaooqinn we have discussed this quite a bit already and now going in circles :-)
Essentially my point is this - instead of rejecting invalid configurations, we are trying to workaround invalid user configs. Once released, this will be a pattern we have to then continue supporting - I am not seeing a good reason why we have to introduce this new behavior.

I would recommend simply rejecting the invalid config with an IllegalArgumentException (we do this in a bunch of other places as well for invalid user config), instead of introducing behavior which tries to workaround it [1]: as long as the user gets to see the exception, they can fix the issue and mitigate it.

Thoughts ?

+CC @dongjoon-hyun as well, since you reviewed the PR.

[1] If there is utility in introducing this, and not as a workaround for user config, that would be interesting to know more about - it is not coming out from the discussion here or in the jira.

JoshRosen · 2023-08-22T20:08:37Z

common/network-common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java

-    if (!cf.await(conf.connectionCreationTimeoutMs())) {
+
+    if (connCreateTimeout <= 0) {
+      cf.awaitUninterruptibly();


I think this should be a plain await() instead of awaitUninterruptibly(), since the latter might lead to uninterruptible canceled tasks. See #16866 for a past fix for a similar issue.

Thank you. Could you make a PR to fix it?

It seems that we need a new JIRA to fix that instead of a followup.

Filed https://issues.apache.org/jira/browse/SPARK-44920 and opened #42619 to fix this.

Thank you @JoshRosen. The fix looks good to me.

While considering the usage of await and awaitUninterruptibly, I consulted the Netty documentation (https://netty.io/4.0/api/io/netty/channel/ChannelFuture.html) which recommended the use of awaitUninterruptibly in all its GOOD examples. So...

…TransportClientFactory.createClient() ### What changes were proposed in this pull request? #41785 / SPARK-44241 introduced a new `awaitUninterruptibly()` call in one branch of `TrasportClientFactory.createClient()` (executed when the connection create timeout is non-positive). This PR replaces that call with an interruptible `await()` call. Note that the other pre-existing branches in this method were already using `await()`. ### Why are the changes needed? Uninterruptible waiting can cause problems when cancelling tasks. For details, see #16866 / SPARK-19529, an older PR fixing a similar issue in this same `TransportClientFactory.createClient()` method. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #42619 from JoshRosen/remove-awaitUninterruptibly. Authored-by: Josh Rosen <joshrosen@databricks.com> Signed-off-by: Kent Yao <yao@apache.org>

…TransportClientFactory.createClient() ### What changes were proposed in this pull request? #41785 / SPARK-44241 introduced a new `awaitUninterruptibly()` call in one branch of `TrasportClientFactory.createClient()` (executed when the connection create timeout is non-positive). This PR replaces that call with an interruptible `await()` call. Note that the other pre-existing branches in this method were already using `await()`. ### Why are the changes needed? Uninterruptible waiting can cause problems when cancelling tasks. For details, see #16866 / SPARK-19529, an older PR fixing a similar issue in this same `TransportClientFactory.createClient()` method. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #42619 from JoshRosen/remove-awaitUninterruptibly. Authored-by: Josh Rosen <joshrosen@databricks.com> Signed-off-by: Kent Yao <yao@apache.org> (cherry picked from commit 2137606) Signed-off-by: Kent Yao <yao@apache.org>

…ationTimeout to zero or negative will cause incessant executor cons/destructions ### What changes were proposed in this pull request? This PR makes zero when io.connectionTimeout/connectionCreationTimeout is negative. Zero here means - connectionCreationTimeout = 0，an unlimited CONNNETION_TIMEOUT for connection establishment - connectionTimeout=0， `IdleStateHandler` for triggering `IdleStateEvent` is disabled. ### Why are the changes needed? 1. This PR fixes a bug when connectionCreationTimeout is 0, which means unlimited to netty, but ChannelFuture.await(0) fails directly and inappropriately. 2. This PR fixes a bug when connectionCreationTimeout is less than 0, which causes meaningless transport client reconnections and endless executor reconstructions ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? new unit tests Closes apache#41785 from yaooqinn/SPARK-44241. Authored-by: Kent Yao <yao@apache.org> Signed-off-by: Kent Yao <yao@apache.org> (cherry picked from commit 38645fa) Signed-off-by: Kent Yao <yao@apache.org>

…TransportClientFactory.createClient() ### What changes were proposed in this pull request? apache#41785 / SPARK-44241 introduced a new `awaitUninterruptibly()` call in one branch of `TrasportClientFactory.createClient()` (executed when the connection create timeout is non-positive). This PR replaces that call with an interruptible `await()` call. Note that the other pre-existing branches in this method were already using `await()`. ### Why are the changes needed? Uninterruptible waiting can cause problems when cancelling tasks. For details, see apache#16866 / SPARK-19529, an older PR fixing a similar issue in this same `TransportClientFactory.createClient()` method. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#42619 from JoshRosen/remove-awaitUninterruptibly. Authored-by: Josh Rosen <joshrosen@databricks.com> Signed-off-by: Kent Yao <yao@apache.org> (cherry picked from commit 2137606) Signed-off-by: Kent Yao <yao@apache.org>

…tive celeborn.<module>.io.connectTimeout/connectionTimeout ### What changes were proposed in this pull request? `TransportClientFactory` should regard as zero for negative `celeborn.<module>.io.connectTimeout` and `celeborn.<module>.io.connectionTimeout`. ### Why are the changes needed? When `celeborn.<module>.io.connectionTimeout` is 0 that means unlimited to netty, `ChannelFuture.await(0)` fails directly and inappropriately. Meanwhile, whhen `celeborn.<module>.io.connectionTimeout` is less than 0 that causes meaningless transport client reconnections and endless reconstructions. Backport: - apache/spark#41785 - apache/spark#42619 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? `TransportClientFactorySuiteJ#unlimitedConnectAndConnectionTimeouts` Closes #2467 from SteNicholas/CELEBORN-1392. Authored-by: SteNicholas <programgeek@163.com> Signed-off-by: mingji <fengmingxiao.fmx@alibaba-inc.com>

[SPARK-44241][Core] Mistakenly set io.connectionTimeout/connectionCre…

4030175

…ationTimeout to zero or negative will cause incessant executor cons/destructions

github-actions bot added the CORE label Jun 29, 2023

Hisoka-X reviewed Jun 29, 2023

View reviewed changes

...etwork-common/src/test/java/org/apache/spark/network/client/TransportClientFactorySuite.java Show resolved Hide resolved

dongjoon-hyun reviewed Jun 29, 2023

View reviewed changes

dongjoon-hyun approved these changes Jun 29, 2023

View reviewed changes

yaooqinn closed this in 38645fa Jun 30, 2023

mridulm reviewed Jul 1, 2023

View reviewed changes

yaooqinn deleted the SPARK-44241 branch July 5, 2023 01:49

JoshRosen reviewed Aug 22, 2023

View reviewed changes

JoshRosen mentioned this pull request Aug 23, 2023

[SPARK-44920][CORE] Use await() instead of awaitUninterruptibly() in TransportClientFactory.createClient() #42619

Closed

SteNicholas mentioned this pull request Apr 19, 2024

[CELEBORN-1392] TransportClientFactory should regard as zero for negative celeborn.<module>.io.connectTimeout/connectionTimeout apache/celeborn#2467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-44241][Core] Mistakenly set io.connectionTimeout/connectionCreationTimeout to zero or negative will cause incessant executor cons/destructions #41785

[SPARK-44241][Core] Mistakenly set io.connectionTimeout/connectionCreationTimeout to zero or negative will cause incessant executor cons/destructions #41785

yaooqinn commented Jun 29, 2023 •

edited

Loading

yaooqinn commented Jun 29, 2023

dongjoon-hyun left a comment •

edited

Loading

dongjoon-hyun Jun 29, 2023

dongjoon-hyun Jun 29, 2023

dongjoon-hyun commented Jun 29, 2023

yaooqinn commented Jun 30, 2023

mridulm commented Jun 30, 2023 •

edited

Loading

yaooqinn commented Jun 30, 2023

mridulm commented Jun 30, 2023 •

edited

Loading

yaooqinn commented Jun 30, 2023

yaooqinn commented Jun 30, 2023

yaooqinn commented Jun 30, 2023

mridulm Jul 1, 2023

yaooqinn Jul 2, 2023

mridulm Jul 2, 2023 •

edited

Loading

yaooqinn Jul 3, 2023

mridulm Jul 5, 2023 •

edited

Loading

yaooqinn Jul 5, 2023

mridulm Jul 5, 2023 •

edited

Loading

JoshRosen Aug 22, 2023

dongjoon-hyun Aug 22, 2023

dongjoon-hyun Aug 22, 2023

JoshRosen Aug 23, 2023

yaooqinn Aug 23, 2023

[SPARK-44241][Core] Mistakenly set io.connectionTimeout/connectionCreationTimeout to zero or negative will cause incessant executor cons/destructions #41785

[SPARK-44241][Core] Mistakenly set io.connectionTimeout/connectionCreationTimeout to zero or negative will cause incessant executor cons/destructions #41785

Conversation

yaooqinn commented Jun 29, 2023 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

yaooqinn commented Jun 29, 2023

dongjoon-hyun left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Jun 29, 2023

yaooqinn commented Jun 30, 2023

mridulm commented Jun 30, 2023 • edited Loading

yaooqinn commented Jun 30, 2023

mridulm commented Jun 30, 2023 • edited Loading

yaooqinn commented Jun 30, 2023

yaooqinn commented Jun 30, 2023

yaooqinn commented Jun 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mridulm Jul 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mridulm Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mridulm Jul 5, 2023 • edited Loading

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

yaooqinn commented Jun 29, 2023 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading

mridulm commented Jun 30, 2023 •

edited

Loading

mridulm commented Jun 30, 2023 •

edited

Loading

mridulm Jul 2, 2023 •

edited

Loading

mridulm Jul 5, 2023 •

edited

Loading

mridulm Jul 5, 2023 •

edited

Loading