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 2022/12/07 00:48:41 UTC
[GitHub] [spark] tedyu opened a new pull request, #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
tedyu opened a new pull request, #38948:
URL: https://github.com/apache/spark/pull/38948
### What changes were proposed in this pull request?
This is follow-up to commit cc55de33420335bd715720e1d9190bd5e8e2e9fc where `PVC_COUNTER` was introduced to track outstanding number of PVCs.
`PVC_COUNTER` should only be decremented when the pod deletion happens (in response to error).
### Why are the changes needed?
If the PVC isn't created successfully (where `PVC_COUNTER` isn't incremented), we shouldn't decrement the counter.
`success` tracks the progress of PVC creation:
value 0 means PVC is not created.
value 1 means PVC has been created.
value 2 means PVC has been created but due to subsequent error, the pod is deleted.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing tests.
--
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] tedyu commented on pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
tedyu commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340235685
If exception happens before we reach the following line:
```
kubernetesClient.persistentVolumeClaims().inNamespace(namespace).resource(pvc).create()
```
the counter shouldn't be decremented.
If exception happens after the following line:
```
PVC_COUNTER.incrementAndGet()
```
the counter should be decremented.
We need to make this code future proof.
--
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] dongjoon-hyun commented on pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340238163
This is handled properly by removing `decrement` line.
> the counter shouldn't be decremented.
--
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] tedyu commented on pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
tedyu commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340290773
If possible, can you elaborate a bit ?
If exception happens at `newlyCreatedExecutors(newExecutorId) =` (or later in the try block), the pod would be deleted.
Why shouldn't `PVC_COUNTER` be decremented ?
--
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] github-actions[bot] commented on pull request #38948: [SPARK-41419][K8S] Decrement PVC_COUNTER when the pod deletion happens
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1474538031
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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] dongjoon-hyun commented on pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340231350
In other words, please revert all changes and remove one line, `PVC_COUNTER.decrementAndGet()`.
--
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] dongjoon-hyun commented on pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340288819
It's totally fine because `spark.kubernetes.driver.reusePersistentVolumeClaim=true`. We can reuse that PVC later, @tedyu .
> e.g. the test can produce exception when the following is called:
>
> ```
> newlyCreatedExecutors(newExecutorId) = (resourceProfileId, clock.getTimeMillis())
> ```
--
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] tedyu commented on a diff in pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
tedyu commented on code in PR #38948:
URL: https://github.com/apache/spark/pull/38948#discussion_r1041703880
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -455,8 +457,13 @@ class ExecutorPodsAllocator(
.inNamespace(namespace)
.resource(createdExecutorPod)
.delete()
- PVC_COUNTER.decrementAndGet()
+ if (success == 1) {
+ success += 1
+ }
throw e
+ } finally {
+ val reuse = conf.get(KUBERNETES_DRIVER_OWN_PVC) && conf.get(KUBERNETES_DRIVER_REUSE_PVC)
+ if (!reuse && success == 2) PVC_COUNTER.decrementAndGet()
Review Comment:
The variable `podAllocOnPVC`
```
private val podAllocOnPVC = conf.get(KUBERNETES_DRIVER_OWN_PVC) &&
conf.get(KUBERNETES_DRIVER_REUSE_PVC) && conf.get(KUBERNETES_DRIVER_WAIT_TO_REUSE_PVC)
```
includes one more condition for `KUBERNETES_DRIVER_WAIT_TO_REUSE_PVC`.
That's why I think it doesn't hurt to check again.
--
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] tedyu commented on pull request #38948: [SPARK-41419][K8S] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
tedyu commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340342014
In ExecutorPodsAllocatorSuite.scala, the pair of configs always have the following values:
```
.set(KUBERNETES_DRIVER_OWN_PVC.key, "true")
.set(KUBERNETES_DRIVER_REUSE_PVC.key, "true")
```
If one of them is false, `requestNewExecutors` would proceed with creating pod and PVC.
@dongjoon-hyun
Do you know why there is no test with either of these configs as false ?
--
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] dongjoon-hyun commented on pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340236767
Please make a valid est case for your claim.
--
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] dongjoon-hyun commented on a diff in pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38948:
URL: https://github.com/apache/spark/pull/38948#discussion_r1041691297
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -455,8 +457,12 @@ class ExecutorPodsAllocator(
.inNamespace(namespace)
.resource(createdExecutorPod)
.delete()
- PVC_COUNTER.decrementAndGet()
+ if (success == 1) {
+ success += 1
+ }
throw e
+ } finally {
+ if (success == 2) PVC_COUNTER.decrementAndGet()
Review Comment:
In short, this is wrong because pod deletion doesn't change the number of PVC.
--
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] tedyu commented on a diff in pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
tedyu commented on code in PR #38948:
URL: https://github.com/apache/spark/pull/38948#discussion_r1041696221
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -455,8 +457,12 @@ class ExecutorPodsAllocator(
.inNamespace(namespace)
.resource(createdExecutorPod)
.delete()
- PVC_COUNTER.decrementAndGet()
+ if (success == 1) {
+ success += 1
+ }
throw e
+ } finally {
+ if (success == 2) PVC_COUNTER.decrementAndGet()
Review Comment:
I updated the logic.
Can you take another look ?
--
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] tedyu commented on pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
tedyu commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340242732
My point is: when exception happens, the exception may not come from this call:
```
kubernetesClient.persistentVolumeClaims().inNamespace(namespace).resource(pvc).create()
```
--
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] tedyu commented on pull request #38948: [SPARK-41419][K8S] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
tedyu commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340314428
@dongjoon-hyun @viirya
I have modified the subject and description of this PR.
Please take another look.
--
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] tedyu commented on pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
tedyu commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340231423
That's not the right way :-)
See https://github.com/apache/spark/pull/38943#issuecomment-1340229735
--
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] tedyu commented on pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
tedyu commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340279777
e.g.
the test can produce exception when the following is called:
```
newlyCreatedExecutors(newExecutorId) = (resourceProfileId, clock.getTimeMillis())
```
--
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] dongjoon-hyun commented on a diff in pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38948:
URL: https://github.com/apache/spark/pull/38948#discussion_r1041691297
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -455,8 +457,12 @@ class ExecutorPodsAllocator(
.inNamespace(namespace)
.resource(createdExecutorPod)
.delete()
- PVC_COUNTER.decrementAndGet()
+ if (success == 1) {
+ success += 1
+ }
throw e
+ } finally {
+ if (success == 2) PVC_COUNTER.decrementAndGet()
Review Comment:
In short, this is wrong because pod deletion doesn't change the number of PVCs.
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -455,8 +457,12 @@ class ExecutorPodsAllocator(
.inNamespace(namespace)
.resource(createdExecutorPod)
.delete()
- PVC_COUNTER.decrementAndGet()
+ if (success == 1) {
+ success += 1
+ }
throw e
+ } finally {
+ if (success == 2) PVC_COUNTER.decrementAndGet()
Review Comment:
In short, this is wrong because pod deletion doesn't change the total number of PVCs.
--
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] github-actions[bot] closed pull request #38948: [SPARK-41419][K8S] Decrement PVC_COUNTER when the pod deletion happens
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #38948: [SPARK-41419][K8S] Decrement PVC_COUNTER when the pod deletion happens
URL: https://github.com/apache/spark/pull/38948
--
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] tedyu commented on pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
tedyu commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340255065
e.g.
the test can produce exception when the following is called in `addOwnerReference`
```
originalMetadata.setOwnerReferences(Collections.singletonList(reference))
```
--
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] dongjoon-hyun commented on pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340232151
Okay. Since we don't agree, I will make my PR too. We can compare side-by-side, @tedyu . :)
--
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] tedyu commented on pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
tedyu commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340272860
@dongjoon-hyun
May I borrow your new test case to show that my PR covers that failure scenario ?
--
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] dongjoon-hyun commented on pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340291989
@tedyu . It seems that you forgot `spark.kubernetes.driver.ownPersistentVolumeClaim=true`. Pod deletion doesn't clean up PVCs. It's owned by Driver pod. This is not your bug. That was originated my bug. :)
> If exception happens at newlyCreatedExecutors(newExecutorId) = (or later in the try block), the pod would be deleted.
Why shouldn't PVC_COUNTER be decremented (note: it has been incremented after PVC creation) ?
--
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] dongjoon-hyun commented on a diff in pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38948:
URL: https://github.com/apache/spark/pull/38948#discussion_r1041700263
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -455,8 +457,13 @@ class ExecutorPodsAllocator(
.inNamespace(namespace)
.resource(createdExecutorPod)
.delete()
- PVC_COUNTER.decrementAndGet()
+ if (success == 1) {
+ success += 1
+ }
throw e
+ } finally {
+ val reuse = conf.get(KUBERNETES_DRIVER_OWN_PVC) && conf.get(KUBERNETES_DRIVER_REUSE_PVC)
+ if (!reuse && success == 2) PVC_COUNTER.decrementAndGet()
Review Comment:
`PVC_COUNTER` is only used by SPARK-41410 . `!reuse` is a misleading and useless code, isn't it?
https://github.com/apache/spark/blob/30957a992edb587e8f5046d7c04c848f285688e1/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala#L413
--
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] dongjoon-hyun commented on pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340234190
BTW, we need to add the test case to validate the ideas. I'll try to add to my PR. You may can reuse 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] tedyu commented on pull request #38948: [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens
Posted by GitBox <gi...@apache.org>.
tedyu commented on PR #38948:
URL: https://github.com/apache/spark/pull/38948#issuecomment-1340222202
@dongjoon-hyun
Please take a look.
I am trying to figure out how to add a test.
--
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