You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/10/19 15:30:10 UTC
[GitHub] [spark] mohamadrezarostami opened a new pull request #34331: [SPARK-37060][Core] handle driver status response from backup masters
mohamadrezarostami opened a new pull request #34331:
URL: https://github.com/apache/spark/pull/34331
### What changes were proposed in this pull request?
After a improvement in SPARK-31486, contributor uses 'asyncSendToMasterAndForwardReply' method instead of 'activeMasterEndpoint.askSync' to get the status of driver. Since the driver's status is only available inactive master and the 'asyncSendToMasterAndForwardReply' method iterate over all of the masters, we have to handle the response from the backup masters in the client, which the developer did not consider in the SPARK-31486 change. So drivers running in cluster mode and on a cluster with multi-master affected by this bug. I created the patch for this bug and will soon be sent the pull request.
### Why are the changes needed?
We need to find if the response received from a backup master client must ignore it.
### Does this PR introduce _any_ user-facing change?
A new filed -message: Option[String]- added to the DriverStatusResponse class.
### How was this patch tested?
--
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] Ngone51 commented on a change in pull request #34331: [SPARK-37060][CORE] Handle driver status response from backup masters
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #34331:
URL: https://github.com/apache/spark/pull/34331#discussion_r766740564
##########
File path: core/src/main/scala/org/apache/spark/deploy/Client.scala
##########
@@ -190,13 +190,15 @@ private class ClientEndpoint(
logDebug(s"State of driver $submittedDriverID is ${state.get}, " +
s"continue monitoring driver status.")
}
- }
- }
- } else {
+ }
+ }
+ } else if (!exception.isEmpty && Utils.responseFromBackup(exception.get.getMessage())) {
Review comment:
We could simplify this a bit:
```suggestion
} else if (exception.exists(e => Utils.responseFromBackup(e.getMessage))) {
```
--
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] mohamadrezarostami commented on pull request #34331: [SPARK-37060][CORE] Handle driver status response from backup masters
Posted by GitBox <gi...@apache.org>.
mohamadrezarostami commented on pull request #34331:
URL: https://github.com/apache/spark/pull/34331#issuecomment-994780341
@Ngone51
Thanks for review.
Yes, I will create PRs for branch-3.2 and branch-3.1, too.
--
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] Ngone51 commented on a change in pull request #34331: [SPARK-37060][CORE] Handle driver status response from backup masters
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #34331:
URL: https://github.com/apache/spark/pull/34331#discussion_r747933552
##########
File path: core/src/main/scala/org/apache/spark/deploy/Client.scala
##########
@@ -190,13 +191,13 @@ private class ClientEndpoint(
logDebug(s"State of driver $submittedDriverID is ${state.get}, " +
s"continue monitoring driver status.")
}
- }
- }
- } else {
+ }
+ }
+ } else if (!Utils.responseFromBackup(message)) {
Review comment:
Couldn't we check `expception.getMessage` instead?
--
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] mohamadrezarostami commented on pull request #34331: [SPARK-37060][CORE] Handle driver status response from backup masters
Posted by GitBox <gi...@apache.org>.
mohamadrezarostami commented on pull request #34331:
URL: https://github.com/apache/spark/pull/34331#issuecomment-994516279
> @mohamadrezarostami you may need to rebase your branch to pass GA.
Done!
--
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] Ngone51 commented on pull request #34331: [SPARK-37060][CORE] Handle driver status response from backup masters
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #34331:
URL: https://github.com/apache/spark/pull/34331#issuecomment-967131266
cc @akshatb1 @srowen who involved in the previous PR: https://github.com/apache/spark/pull/28258
--
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] Ngone51 commented on a change in pull request #34331: [SPARK-37060][CORE] Handle driver status response from backup masters
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #34331:
URL: https://github.com/apache/spark/pull/34331#discussion_r748022121
##########
File path: core/src/main/scala/org/apache/spark/deploy/Client.scala
##########
@@ -190,13 +190,13 @@ private class ClientEndpoint(
logDebug(s"State of driver $submittedDriverID is ${state.get}, " +
s"continue monitoring driver status.")
}
- }
- }
- } else {
+ }
+ }
+ } else if (exception.isEmpty || !Utils.responseFromBackup(a.get.getMessage())) {
Review comment:
I mean, I didn't see `a` is defined in the scope. It should be `exception.get.getMessage`.
--
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] Ngone51 commented on pull request #34331: [SPARK-37060][CORE] Handle driver status response from backup masters
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #34331:
URL: https://github.com/apache/spark/pull/34331#issuecomment-994765502
Thanks, merged to master.
@mohamadrezarostami Could you create PRs to backport this to branch-3.2/branch-3.1?
--
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] Ngone51 commented on pull request #34331: [SPARK-37060][CORE] Handle driver status response from backup masters
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #34331:
URL: https://github.com/apache/spark/pull/34331#issuecomment-993136916
@mohamadrezarostami you may need to rebase your branch to pass GA.
--
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] mohamadrezarostami commented on a change in pull request #34331: [SPARK-37060][CORE] Handle driver status response from backup masters
Posted by GitBox <gi...@apache.org>.
mohamadrezarostami commented on a change in pull request #34331:
URL: https://github.com/apache/spark/pull/34331#discussion_r767797304
##########
File path: core/src/main/scala/org/apache/spark/deploy/Client.scala
##########
@@ -190,13 +190,15 @@ private class ClientEndpoint(
logDebug(s"State of driver $submittedDriverID is ${state.get}, " +
s"continue monitoring driver status.")
}
- }
- }
- } else {
+ }
+ }
+ } else if (!exception.isEmpty && Utils.responseFromBackup(exception.get.getMessage())) {
Review comment:
Yes, I agree with it.
--
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] mohamadrezarostami commented on a change in pull request #34331: [SPARK-37060][CORE] Handle driver status response from backup masters
Posted by GitBox <gi...@apache.org>.
mohamadrezarostami commented on a change in pull request #34331:
URL: https://github.com/apache/spark/pull/34331#discussion_r748021561
##########
File path: core/src/main/scala/org/apache/spark/deploy/Client.scala
##########
@@ -190,13 +190,13 @@ private class ClientEndpoint(
logDebug(s"State of driver $submittedDriverID is ${state.get}, " +
s"continue monitoring driver status.")
}
- }
- }
- } else {
+ }
+ }
+ } else if (exception.isEmpty || !Utils.responseFromBackup(a.get.getMessage())) {
Review comment:
I am new in Scala. The exception is an Option[Exception]. As I know we can't call getMessage() directly! Am I right?
e.g. https://scastie.scala-lang.org/Zs5zrPU1SqulmCRthPHDXg
--
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] AmplabJenkins commented on pull request #34331: [SPARK-37060][Core] Handle driver status response from backup masters
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #34331:
URL: https://github.com/apache/spark/pull/34331#issuecomment-946849836
Can one of the admins verify this patch?
--
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] mohamadrezarostami commented on a change in pull request #34331: [SPARK-37060][CORE] Handle driver status response from backup masters
Posted by GitBox <gi...@apache.org>.
mohamadrezarostami commented on a change in pull request #34331:
URL: https://github.com/apache/spark/pull/34331#discussion_r748029609
##########
File path: core/src/main/scala/org/apache/spark/deploy/Client.scala
##########
@@ -190,13 +190,13 @@ private class ClientEndpoint(
logDebug(s"State of driver $submittedDriverID is ${state.get}, " +
s"continue monitoring driver status.")
}
- }
- }
- } else {
+ }
+ }
+ } else if (exception.isEmpty || !Utils.responseFromBackup(a.get.getMessage())) {
Review comment:
I also changed the condition handling to keep compatible with previous assumptions.
--
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] Ngone51 closed pull request #34331: [SPARK-37060][CORE] Handle driver status response from backup masters
Posted by GitBox <gi...@apache.org>.
Ngone51 closed pull request #34331:
URL: https://github.com/apache/spark/pull/34331
--
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] mohamadrezarostami commented on pull request #34331: [SPARK-37060][CORE] Handle driver status response from backup masters
Posted by GitBox <gi...@apache.org>.
mohamadrezarostami commented on pull request #34331:
URL: https://github.com/apache/spark/pull/34331#issuecomment-971385831
Could someone please review these changes? CC: @cloud-fan @Ngone51 @HeartSaVioR @jiangxb1987
--
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] Ngone51 commented on a change in pull request #34331: [SPARK-37060][CORE] Handle driver status response from backup masters
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #34331:
URL: https://github.com/apache/spark/pull/34331#discussion_r748019724
##########
File path: core/src/main/scala/org/apache/spark/deploy/Client.scala
##########
@@ -190,13 +190,13 @@ private class ClientEndpoint(
logDebug(s"State of driver $submittedDriverID is ${state.get}, " +
s"continue monitoring driver status.")
}
- }
- }
- } else {
+ }
+ }
+ } else if (exception.isEmpty || !Utils.responseFromBackup(a.get.getMessage())) {
Review comment:
`a.get`? typo?
--
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