Skip to content
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

add failure context to recordExecutorEvent #1280

Merged
merged 1 commit into from
Jun 30, 2021

Conversation

ImpSy
Copy link
Contributor

@ImpSy ImpSy commented Jun 7, 2021

Hello,

Since they were no traction on this issue: #1263

I've decided to implement solution 1 for executors

If i need to change anything do not hesitate

@jrj-d
Copy link
Contributor

jrj-d commented Jun 21, 2021

@liyinan926 can review this PR or tell us who else we should ask for a review?

switch state {
case v1beta2.ExecutorCompletedState:
c.recorder.Eventf(app, apiv1.EventTypeNormal, "SparkExecutorCompleted", "Executor %s completed", name)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the output look like for args?

Copy link
Contributor Author

@ImpSy ImpSy Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as before we use c.recorder.Eventf which already accept variadics parameters

per doc:

Eventf(object runtime.Object, eventtype, reason, messageFmt string, args ...interface{})

We already use something similar in recordSparkApplicationEvent for v1beta2.FailedState

@liyinan926 liyinan926 merged commit 92e8d78 into kubeflow:master Jun 30, 2021
ImpSy added a commit to datamechanics/spark-on-k8s-operator that referenced this pull request Aug 18, 2021
liyinan926 pushed a commit that referenced this pull request Aug 20, 2021
jbhalodia-slack pushed a commit to jbhalodia-slack/spark-operator that referenced this pull request Oct 4, 2024
jbhalodia-slack pushed a commit to jbhalodia-slack/spark-operator that referenced this pull request Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants