You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "nija-at (via GitHub)" <gi...@apache.org> on 2023/02/17 15:26:40 UTC
[GitHub] [spark] nija-at opened a new pull request, #40066: [CONNECT] [PYTHON] reduce spark connect service retries
nija-at opened a new pull request, #40066:
URL: https://github.com/apache/spark/pull/40066
### What changes were proposed in this pull request?
* Reduce the number of retries when sending a request to spark connect service.
* Slightly increased debug logging to help better debugging
### Why are the changes needed?
Currently, 15 retries with the current backoff strategy result in the client sitting in
the retry loop for ~400 seconds. This means, applications and users using the spark
connect client will hang for 6ish minutes with no response.
Instead, simply reduce the retry to 4, which reduces the retry loop in the worst case
to a reasonable ~4 seconds.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Manual testing
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #40066: [CONNECT] [PYTHON] reduce spark connect service retries
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40066:
URL: https://github.com/apache/spark/pull/40066#discussion_r1111356494
##########
python/pyspark/sql/connect/client.py:
##########
@@ -772,6 +772,7 @@ def __exit__(
if isinstance(exc_val, BaseException):
# Swallow the exception.
if self._can_retry(exc_val):
+ logger.debug(f"Server responded with retryable error: {exc_val}")
Review Comment:
Shall we also mention that we're trying from this error? The message itself looks like it has to be either error or warning.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #40066: [CONNECT] [PYTHON] reduce spark connect service retries
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40066:
URL: https://github.com/apache/spark/pull/40066#discussion_r1111356716
##########
python/pyspark/sql/connect/client.py:
##########
@@ -409,7 +409,7 @@ def __init__(
self._builder = ChannelBuilder(connectionString, channelOptions)
self._user_id = None
self._retry_policy = {
- "max_retries": 15,
+ "max_retries": 4,
Review Comment:
I don't mind about this number but just curious. Is there a reason to pick 4? I was thinking 3 is sort of more common :-).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] nija-at commented on a diff in pull request #40066: [CONNECT][PYTHON] Reduce spark connect service retries
Posted by "nija-at (via GitHub)" <gi...@apache.org>.
nija-at commented on code in PR #40066:
URL: https://github.com/apache/spark/pull/40066#discussion_r1111573178
##########
python/pyspark/sql/connect/client.py:
##########
@@ -772,6 +772,7 @@ def __exit__(
if isinstance(exc_val, BaseException):
# Swallow the exception.
if self._can_retry(exc_val):
+ logger.debug(f"Server responded with retryable error: {exc_val}")
Review Comment:
If the service is unreachable for a moment and if the retry is going to fix it, it's not an error.
It could be a warn but that'll pollute real warnings. I think it's debug.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #40066: [CONNECT] [PYTHON] reduce spark connect service retries
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40066:
URL: https://github.com/apache/spark/pull/40066#discussion_r1111356271
##########
python/pyspark/sql/connect/client.py:
##########
@@ -632,7 +632,7 @@ def _execute(self, req: pb2.ExecutePlanRequest) -> None:
Proto representation of the plan.
"""
- logger.info("Execute")
+ logger.debug(f"Execute request: » {os.linesep}{req}{os.linesep} «")
Review Comment:
I would avoid the characters such as `»` that's not used in the current codebase.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] nija-at commented on a diff in pull request #40066: [CONNECT][PYTHON] Reduce spark connect service retries
Posted by "nija-at (via GitHub)" <gi...@apache.org>.
nija-at commented on code in PR #40066:
URL: https://github.com/apache/spark/pull/40066#discussion_r1111573491
##########
python/pyspark/sql/connect/client.py:
##########
@@ -409,7 +409,7 @@ def __init__(
self._builder = ChannelBuilder(connectionString, channelOptions)
self._user_id = None
self._retry_policy = {
- "max_retries": 15,
+ "max_retries": 4,
Review Comment:
See PR description.
4 retries keeps the retry loop to 4 seconds which feels more reasonable than going down to 3.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] nija-at commented on pull request #40066: [SPARK-42498] [CONNECT][PYTHON] Reduce spark connect service retries
Posted by "nija-at (via GitHub)" <gi...@apache.org>.
nija-at commented on PR #40066:
URL: https://github.com/apache/spark/pull/40066#issuecomment-1437189282
Discussed this with @grundprinzip. Apparently the retries are kept so long intentionally. The fix here needs to occur differently. Closing this PR for now.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] nija-at commented on a diff in pull request #40066: [SPARK-42498] [CONNECT][PYTHON] Reduce spark connect service retries
Posted by "nija-at (via GitHub)" <gi...@apache.org>.
nija-at commented on code in PR #40066:
URL: https://github.com/apache/spark/pull/40066#discussion_r1111573178
##########
python/pyspark/sql/connect/client.py:
##########
@@ -772,6 +772,7 @@ def __exit__(
if isinstance(exc_val, BaseException):
# Swallow the exception.
if self._can_retry(exc_val):
+ logger.debug(f"Server responded with retryable error: {exc_val}")
Review Comment:
If the service is unreachable for a moment and if the retry is going to fix it, it's not an "error".
It could be a warn but that'll pollute real warnings. I think debug makes the most sense.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] nija-at commented on a diff in pull request #40066: [CONNECT][PYTHON] Reduce spark connect service retries
Posted by "nija-at (via GitHub)" <gi...@apache.org>.
nija-at commented on code in PR #40066:
URL: https://github.com/apache/spark/pull/40066#discussion_r1111573771
##########
python/pyspark/sql/connect/client.py:
##########
@@ -632,7 +632,7 @@ def _execute(self, req: pb2.ExecutePlanRequest) -> None:
Proto representation of the plan.
"""
- logger.info("Execute")
+ logger.debug(f"Execute request: » {os.linesep}{req}{os.linesep} «")
Review Comment:
curious: why?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] nija-at commented on a diff in pull request #40066: [CONNECT][PYTHON] Reduce spark connect service retries
Posted by "nija-at (via GitHub)" <gi...@apache.org>.
nija-at commented on code in PR #40066:
URL: https://github.com/apache/spark/pull/40066#discussion_r1111573491
##########
python/pyspark/sql/connect/client.py:
##########
@@ -409,7 +409,7 @@ def __init__(
self._builder = ChannelBuilder(connectionString, channelOptions)
self._user_id = None
self._retry_policy = {
- "max_retries": 15,
+ "max_retries": 4,
Review Comment:
See PR description.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] nija-at commented on a diff in pull request #40066: [SPARK-42498] [CONNECT][PYTHON] Reduce spark connect service retries
Posted by "nija-at (via GitHub)" <gi...@apache.org>.
nija-at commented on code in PR #40066:
URL: https://github.com/apache/spark/pull/40066#discussion_r1111573178
##########
python/pyspark/sql/connect/client.py:
##########
@@ -772,6 +772,7 @@ def __exit__(
if isinstance(exc_val, BaseException):
# Swallow the exception.
if self._can_retry(exc_val):
+ logger.debug(f"Server responded with retryable error: {exc_val}")
Review Comment:
If the service is unreachable for a moment and if the retry is going to fix it, it's not an "error".
It could be a warn but that'll pollute real warnings. I think it's debug.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] nija-at commented on a diff in pull request #40066: [CONNECT][PYTHON] Reduce spark connect service retries
Posted by "nija-at (via GitHub)" <gi...@apache.org>.
nija-at commented on code in PR #40066:
URL: https://github.com/apache/spark/pull/40066#discussion_r1111576506
##########
python/pyspark/sql/connect/client.py:
##########
@@ -632,7 +632,7 @@ def _execute(self, req: pb2.ExecutePlanRequest) -> None:
Proto representation of the plan.
"""
- logger.info("Execute")
+ logger.debug(f"Execute request: » {os.linesep}{req}{os.linesep} «")
Review Comment:
Can you suggest a better alternative?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #40066: [SPARK-42498] [CONNECT][PYTHON] Reduce spark connect service retries
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40066:
URL: https://github.com/apache/spark/pull/40066#discussion_r1111999039
##########
python/pyspark/sql/connect/client.py:
##########
@@ -632,7 +632,7 @@ def _execute(self, req: pb2.ExecutePlanRequest) -> None:
Proto representation of the plan.
"""
- logger.info("Execute")
+ logger.debug(f"Execute request: » {os.linesep}{req}{os.linesep} «")
Review Comment:
Maybe just `Execute request: {os.linesep}{req}{os.linesep}`? I would stick to the existing codebase unless it needs a reason.
It's also sort of documented:
> If you’re not sure about the right style for something, try to follow the style of the existing codebase
https://spark.apache.org/contributing.html
So I wouldn't do something new unless there's a reason.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #40066: [CONNECT] [PYTHON] reduce spark connect service retries
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40066:
URL: https://github.com/apache/spark/pull/40066#discussion_r1111356716
##########
python/pyspark/sql/connect/client.py:
##########
@@ -409,7 +409,7 @@ def __init__(
self._builder = ChannelBuilder(connectionString, channelOptions)
self._user_id = None
self._retry_policy = {
- "max_retries": 15,
+ "max_retries": 4,
Review Comment:
I don't mind about this number but just curious. Is there a reason to pick 4?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #40066: [CONNECT][PYTHON] Reduce spark connect service retries
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #40066:
URL: https://github.com/apache/spark/pull/40066#issuecomment-1436149452
I think we should better file a JIRA.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #40066: [SPARK-42498] [CONNECT][PYTHON] Reduce spark connect service retries
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40066:
URL: https://github.com/apache/spark/pull/40066#discussion_r1112001886
##########
python/pyspark/sql/connect/client.py:
##########
@@ -409,7 +409,7 @@ def __init__(
self._builder = ChannelBuilder(connectionString, channelOptions)
self._user_id = None
self._retry_policy = {
- "max_retries": 15,
+ "max_retries": 4,
Review Comment:
Saw PR description but it doesn't explain why it picks 4 vs 3.
We have many configurations that defaulting to 3, e.g. `spark.rpc.numRetries`, `spark.shuffle.io.maxRetries` that all default to 3. Can we match it if there isn't a reason for that?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] nija-at closed pull request #40066: [SPARK-42498] [CONNECT][PYTHON] Reduce spark connect service retries
Posted by "nija-at (via GitHub)" <gi...@apache.org>.
nija-at closed pull request #40066: [SPARK-42498] [CONNECT][PYTHON] Reduce spark connect service retries
URL: https://github.com/apache/spark/pull/40066
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org