[SPARK-42090] Introduce sasl retry count in RetryingBlockTransferor #39611

tedyu · 2023-01-16T13:51:22Z

What changes were proposed in this pull request?

This PR introduces sasl retry count in RetryingBlockTransferor.

Why are the changes needed?

Previously a boolean variable, saslTimeoutSeen, was used. However, the boolean variable wouldn't cover the following scenario:

SaslTimeoutException
IOException
SaslTimeoutException
IOException

Even though IOException at #2 is retried (resulting in increment of retryCount), the retryCount would be cleared at step #4.
Since the intention of saslTimeoutSeen is to undo the increment due to retrying SaslTimeoutException, we should keep a counter for SaslTimeoutException retries and subtract the value of this counter from retryCount.

Does this PR introduce any user-facing change?

No

How was this patch tested?

New test is added, courtesy of Mridul.

tedyu · 2023-01-16T13:52:34Z

cc @mridulm

mridulm · 2023-01-16T17:03:09Z

+CC @akpatnam25, @otterc

tedyu · 2023-01-16T18:55:08Z

From https://github.com/tedyu/spark/actions/runs/3931766275/jobs/6723482156

FAIL [0.692s]: test_shuffle_data_with_multiple_locations (pyspark.tests.test_shuffle.MergerTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/__w/spark/spark/python/pyspark/tests/test_shuffle.py", line 84, in test_shuffle_data_with_multiple_locations
    self.assertTrue(
AssertionError: False is not true

It seems the above error was not related to the PR.

tedyu · 2023-01-16T19:05:16Z

In the previous test suite run, there was no test failure - only lint error:

https://github.com/tedyu/spark/actions/runs/3931003566

akpatnam25

Thanks for catching this corner case, I missed it in my PR. Just had a couple minor comments. LGTM overall.

akpatnam25 · 2023-01-16T21:28:11Z

...ork-shuffle/src/test/java/org/apache/spark/network/shuffle/RetryingBlockTransferorSuite.java

@@ -341,6 +343,35 @@ public void testBlockTransferFailureAfterSasl() throws IOException, InterruptedE
    assert(_retryingBlockTransferor.getRetryCount() == 1);
  }

+  @Test
+  public void testIOExceptionFailsConnectionEvenWithSaslException()


do you think we should also expose saslRetryCount as @VisibleForTesting and assert the count in the tests?

See the change w.r.t. your 3rd comment.
With that change, I think we don't need to expose saslRetryCount.

In the future, we can use a map (type of exception -> outstanding retry count for that type) where we can think more about asserting on retry count for each type.

makes sense. I had a similar idea here: 3440f12
But we ended up opting to go with a simple flag for now. I agree with using a map in the future where we might have retry counts for each type.

akpatnam25 · 2023-01-16T21:29:00Z

...ork-shuffle/src/test/java/org/apache/spark/network/shuffle/RetryingBlockTransferorSuite.java

+            ImmutableMap.of("b0", saslExceptionInitial),
+            ImmutableMap.of("b0", ioException),
+            ImmutableMap.of("b0", saslExceptionFinal),
+            // will not get invoked


Can you add to this comment to explain why it won't get invoked?

See if my addition should be enhanced.

akpatnam25 · 2023-01-16T21:30:50Z

.../network-shuffle/src/main/java/org/apache/spark/network/shuffle/RetryingBlockTransferor.java

+    // If this is a non SASL request failure, reduce earlier SASL failures from retryCount
+    // since some subsequent SASL attempt was successful
+    if (!isSaslTimeout && saslRetryCount > 0) {
+      retryCount -= saslRetryCount;


Should we also verify that retryCount >= saslRetryCount? Just so that we don't ever end up in some funky state where we end up with negative values (I don't think this can ever happen, but would good to add the check!). :)

I added a runtime exception.
But as you said, subset should never be bigger than the whole.

looks good, thanks

mridulm

Looks good to me, just a couple of comments.
I will merge it later tonight once the updates and tests pass.

.../network-shuffle/src/main/java/org/apache/spark/network/shuffle/RetryingBlockTransferor.java

tedyu · 2023-01-17T03:29:46Z

@mridulm
Your comments have been addressed.

akpatnam25

LGTM

mridulm

Looks good to me

mridulm · 2023-01-17T06:50:03Z

Merged to master.
Thanks for fixing this @tedyu !
Thanks for the review @akpatnam25 :-)

mridulm · 2023-01-17T06:50:26Z

@akpatnam25, please do include this fix in your backport to 3.3 and 3.2.

### What changes were proposed in this pull request? This PR introduces sasl retry count in RetryingBlockTransferor. ### Why are the changes needed? Previously a boolean variable, saslTimeoutSeen, was used. However, the boolean variable wouldn't cover the following scenario: 1. SaslTimeoutException 2. IOException 3. SaslTimeoutException 4. IOException Even though IOException at apache#2 is retried (resulting in increment of retryCount), the retryCount would be cleared at step apache#4. Since the intention of saslTimeoutSeen is to undo the increment due to retrying SaslTimeoutException, we should keep a counter for SaslTimeoutException retries and subtract the value of this counter from retryCount. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? New test is added, courtesy of Mridul. Closes apache#39611 from tedyu/sasl-cnt. Authored-by: Ted Yu <yuzhihong@gmail.com> Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>

### What changes were proposed in this pull request? This PR introduces sasl retry count in RetryingBlockTransferor. ### Why are the changes needed? Previously a boolean variable, saslTimeoutSeen, was used. However, the boolean variable wouldn't cover the following scenario: 1. SaslTimeoutException 2. IOException 3. SaslTimeoutException 4. IOException Even though IOException at #2 is retried (resulting in increment of retryCount), the retryCount would be cleared at step #4. Since the intention of saslTimeoutSeen is to undo the increment due to retrying SaslTimeoutException, we should keep a counter for SaslTimeoutException retries and subtract the value of this counter from retryCount. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? New test is added, courtesy of Mridul. Closes #39611 from tedyu/sasl-cnt. Authored-by: Ted Yu <yuzhihonggmail.com> Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com> Closes #39710 from akpatnam25/SPARK-42090-backport-3.2. Authored-by: Ted Yu <yuzhihong@gmail.com> Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>

### What changes were proposed in this pull request? This PR introduces sasl retry count in RetryingBlockTransferor. ### Why are the changes needed? Previously a boolean variable, saslTimeoutSeen, was used. However, the boolean variable wouldn't cover the following scenario: 1. SaslTimeoutException 2. IOException 3. SaslTimeoutException 4. IOException Even though IOException at #2 is retried (resulting in increment of retryCount), the retryCount would be cleared at step #4. Since the intention of saslTimeoutSeen is to undo the increment due to retrying SaslTimeoutException, we should keep a counter for SaslTimeoutException retries and subtract the value of this counter from retryCount. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? New test is added, courtesy of Mridul. Closes #39611 from tedyu/sasl-cnt. Authored-by: Ted Yu <yuzhihonggmail.com> Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com> Closes #39709 from akpatnam25/SPARK-42090-backport-3.3. Authored-by: Ted Yu <yuzhihong@gmail.com> Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>

### What changes were proposed in this pull request? This PR introduces sasl retry count in RetryingBlockTransferor. ### Why are the changes needed? Previously a boolean variable, saslTimeoutSeen, was used. However, the boolean variable wouldn't cover the following scenario: 1. SaslTimeoutException 2. IOException 3. SaslTimeoutException 4. IOException Even though IOException at apache#2 is retried (resulting in increment of retryCount), the retryCount would be cleared at step apache#4. Since the intention of saslTimeoutSeen is to undo the increment due to retrying SaslTimeoutException, we should keep a counter for SaslTimeoutException retries and subtract the value of this counter from retryCount. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? New test is added, courtesy of Mridul. Closes apache#39611 from tedyu/sasl-cnt. Authored-by: Ted Yu <yuzhihonggmail.com> Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com> Closes apache#39710 from akpatnam25/SPARK-42090-backport-3.2. Authored-by: Ted Yu <yuzhihong@gmail.com> Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>

github-actions bot added the CORE label Jan 16, 2023

tedyu changed the title ~~Introduce sasl retry count in RetryingBlockTransferor~~ [SPARK-42090] Introduce sasl retry count in RetryingBlockTransferor Jan 16, 2023

tedyu force-pushed the sasl-cnt branch from d392503 to 813b1d1 Compare January 16, 2023 15:23

akpatnam25 reviewed Jan 16, 2023

View reviewed changes

tedyu force-pushed the sasl-cnt branch 2 times, most recently from d9d78a9 to ea357f4 Compare January 16, 2023 23:20

mridulm approved these changes Jan 17, 2023

View reviewed changes

.../network-shuffle/src/main/java/org/apache/spark/network/shuffle/RetryingBlockTransferor.java Outdated Show resolved Hide resolved

.../network-shuffle/src/main/java/org/apache/spark/network/shuffle/RetryingBlockTransferor.java Show resolved Hide resolved

[SPARK-42090] Introduce sasl retry count in RetryingBlockTransferor

7a3e088

tedyu force-pushed the sasl-cnt branch from ea357f4 to 7a3e088 Compare January 17, 2023 03:29

akpatnam25 approved these changes Jan 17, 2023

View reviewed changes

mridulm approved these changes Jan 17, 2023

View reviewed changes

mridulm closed this in f1a3f4a Jan 17, 2023

akpatnam25 mentioned this pull request Jan 17, 2023

SPARK-41415/SPARK-42090 Backport to 3.2 #39632

Closed

akpatnam25 mentioned this pull request Jan 17, 2023

SPARK-41415/SPARK-42090 Backport to 3.3 #39634

Closed

akpatnam25 mentioned this pull request Jan 23, 2023

[SPARK-42090][3.3] Introduce sasl retry count in RetryingBlockTransferor #39709

Closed

akpatnam25 mentioned this pull request Jan 23, 2023

[SPARK-42090][3.2] Introduce sasl retry count in RetryingBlockTransferor #39710

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-42090] Introduce sasl retry count in RetryingBlockTransferor #39611

[SPARK-42090] Introduce sasl retry count in RetryingBlockTransferor #39611

tedyu commented Jan 16, 2023

tedyu commented Jan 16, 2023

mridulm commented Jan 16, 2023

tedyu commented Jan 16, 2023

tedyu commented Jan 16, 2023

akpatnam25 left a comment

akpatnam25 Jan 16, 2023

tedyu Jan 16, 2023

akpatnam25 Jan 17, 2023

akpatnam25 Jan 16, 2023

tedyu Jan 16, 2023

akpatnam25 Jan 16, 2023

tedyu Jan 16, 2023

akpatnam25 Jan 17, 2023

mridulm left a comment

tedyu commented Jan 17, 2023

akpatnam25 left a comment

mridulm left a comment

mridulm commented Jan 17, 2023

mridulm commented Jan 17, 2023

[SPARK-42090] Introduce sasl retry count in RetryingBlockTransferor #39611

[SPARK-42090] Introduce sasl retry count in RetryingBlockTransferor #39611

Conversation

tedyu commented Jan 16, 2023

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?

tedyu commented Jan 16, 2023

mridulm commented Jan 16, 2023

tedyu commented Jan 16, 2023

tedyu commented Jan 16, 2023

akpatnam25 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

mridulm left a comment

Choose a reason for hiding this comment

tedyu commented Jan 17, 2023

akpatnam25 left a comment

Choose a reason for hiding this comment

mridulm left a comment

Choose a reason for hiding this comment

mridulm commented Jan 17, 2023

mridulm commented Jan 17, 2023