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-core: added default values to write termination message method #1185

Conversation

mivanov1988
Copy link
Collaborator

@mivanov1988 mivanov1988 commented Sep 19, 2022

Currently, we have an internal customer experiencing concurrent executions of the same data job. The issue is in the execution logic because of missing default values of the write_termination_message method.

Exception:

execution_skip.py:130  _skip_job_if_nec[id:moonshine-stage-cost-optimization-strategies-1663579200-qfxw2]- Error while checking for another data job excecution: write_termination_message() missing 2 required positional arguments: 'error_overall' and 'user_error' 

This change aims to fix the issue ASAP since the customer may have duplicate data because of it.

@antoniivanov
Copy link
Collaborator

antoniivanov commented Sep 19, 2022

I don't understand the issue? Why does setting them explicitly fixes it ? Don't they have defaults ?

There are tests missing (tests would make it easier to understand the problem)

@mivanov1988
Copy link
Collaborator Author

I don't understand the issue? Why does setting them explicitly fixes it ? Don't they have defaults ?

There are tests missing (tests would make it easier to understand the problem)

Currently, they don't have default values.

@mivanov1988
Copy link
Collaborator Author

I don't understand the issue? Why does setting them explicitly fixes it ? Don't they have defaults ?

There are tests missing (tests would make it easier to understand the problem)

Do you think it is better to add defaults like that?

def write_termination_message(
        self, error_overall=None, user_error=None, configuration=None, execution_skipped=False
    ):

@antoniivanov
Copy link
Collaborator

I don't understand the issue? Why does setting them explicitly fixes it ? Don't they have defaults ?
There are tests missing (tests would make it easier to understand the problem)

Do you think it is better to add defaults like that?

def write_termination_message(
        self, error_overall=None, user_error=None, configuration=None, execution_skipped=False
    ):

Yes, the methods should set sensible defaults for optional arguments IMO.

@mivanov1988 mivanov1988 merged commit 0ca7d19 into main Sep 19, 2022
@mivanov1988 mivanov1988 deleted the person/miroslavi/add-defaults-to-write-termination-message-method branch September 19, 2022 12:57
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