-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-25828][K8S] Bumping Kubernetes-Client version to 4.1.0 #22820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@erikerlandson @skonto @liyinan926 for review |
|
Test build #97992 has finished for PR 22820 at commit
|
| <properties> | ||
| <sbt.project.name>kubernetes</sbt.project.name> | ||
| <kubernetes.client.version>3.0.0</kubernetes.client.version> | ||
| <kubernetes.client.version>4.0.0</kubernetes.client.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4.1.0? https://mvnrepository.com/artifact/io.fabric8/kubernetes-client/4.1.0 what are the differences compared to 4.0.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, agreed. Maybe 4.1.0 is a more stable version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah just wondering I don't know. What matters is what each version supports. Still no version supports 1.11 fabric8io/kubernetes-client#1235
|
Test build #97994 has finished for PR 22820 at commit
|
...ce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
Show resolved
Hide resolved
...netes/core/src/main/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStep.scala
Show resolved
Hide resolved
felixcheung
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should there be some doc changes on supported client version
|
Test build #98037 has finished for PR 22820 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. But looks like the build failed on dependency tests. Please take a look.
|
Kubernetes integration test starting |
@liyinan926 that is In reference to manifests that need to be updated in |
|
Kubernetes integration test status success |
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
docs/running-on-kubernetes.md
Outdated
|
|
||
| The driver and executor pod scheduling is handled by Kubernetes. It is possible to schedule the | ||
| The driver and executor pod scheduling is handled by Kubernetes. Communication to the Kubernetes API is done via fabric8, and we are | ||
| currently running <code>kubernetes-client</code> version <code>4.1.0</code>. Make sure that any infrastructure additions are weary of said version. It is possible to schedule the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "wary" ? or "aware of " ?
...ce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
Show resolved
Hide resolved
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #98041 has finished for PR 22820 at commit
|
|
Test build #98054 has finished for PR 22820 at commit
|
|
retest this please |
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #98064 has finished for PR 22820 at commit
|
rvesse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 Would like to get this in ASAP so I can rebase PR #22805 and avoid mixing unrelated changes into the integration testing enhancement PR per @liyinan926's request
| if (time != null) time.getTime else "N/A" | ||
| def formatTime(time: String): String = { | ||
| // TODO: Investigate form `time` comes in as there was a DataType | ||
| // change from Time to String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string returned is ISO8601 format so no need to do anything special here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. a sample from the logs from my PR where I had made a similar change:
creation time: 2018-10-24T17:17:03Z
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #98102 has finished for PR 22820 at commit
|
|
@felixcheung and @erikerlandson and @liyinan926 for merge |
|
merging |
## What changes were proposed in this pull request? Changed the `kubernetes-client` version and refactored code that broke as a result ## How was this patch tested? Unit and Integration tests Closes apache#22820 from ifilonenko/SPARK-25828. Authored-by: Ilan Filonenko <[email protected]> Signed-off-by: Erik Erlandson <[email protected]>
What changes were proposed in this pull request?
Changed the
kubernetes-clientversion and refactored code that broke as a resultHow was this patch tested?
Unit and Integration tests