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

vdk-jupyter: fix bug for failed requests and improve error handling #2916

Merged
merged 6 commits into from
Nov 20, 2023

Conversation

yonitoo
Copy link
Contributor

@yonitoo yonitoo commented Nov 17, 2023

What: Bug fix. Since the Task Runner change got in, if you tried to deploy a job without keytab, you would not have been informed about the error in any way and the Task Runner state wouldn't have been cleaned, therefore, not being able to handle any other request. Only a restart of the session would have solved this which is not good.
To address this I:

  • Modified the Task Runner start_task() method to proceed after 'failed' last request.
  • Improved the error handling - show the error of failing request which was not shown and made the backend handlers able to detect any errors.

Why: To make it work in a better way and not to have to restart the session on each bad request.

Testing Done: Introduced relevant tests. The rest of the tests are passing.

Copy link
Collaborator

@antoniivanov antoniivanov left a comment

Choose a reason for hiding this comment

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

I don't really see tests covering the scenario

Copy link
Contributor

@gabrielgeorgiev1 gabrielgeorgiev1 left a comment

Choose a reason for hiding this comment

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

looks good, add a test or two and go ahead

@yonitoo yonitoo merged commit 904bdb7 into main Nov 20, 2023
@yonitoo yonitoo deleted the topic/ysalambashev/vdk-jupyter-fix-task-runner-bug branch November 20, 2023 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants