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: termination message now idempotent #909

Merged
merged 3 commits into from
Jul 25, 2022

Conversation

mrMoZ1
Copy link
Contributor

@mrMoZ1 mrMoZ1 commented Jul 21, 2022

what: termination message plugin now overrides the previous message.

why: Some data jobs that make use of the Standalone Data Job end up with double termination message, e.g.,

'{"vdk_version": "0.3.572339461", "status": "Success"}{"vdk_version": "0.3.572339461", "status": "Success"}'

This confuses the Control Service, which therefore classifies the execution as Platform Error, even if the job is executed successfully.

testing: added unit tests

Signed-off-by: Momchil Zhivkov [email protected]

@ivakoleva
Copy link
Contributor

Not sure ifidempotence is the word to use, the data job remains the same yet multiple different statuses are written sequentially during execution, yes?
Where in the workflow is the written status used?

mrMoZ1 added 2 commits July 22, 2022 16:23
Signed-off-by: mrMoZ1 <[email protected]>
Signed-off-by: mrMoZ1 <[email protected]>
Copy link
Contributor

@ivakoleva ivakoleva left a comment

Choose a reason for hiding this comment

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

If hook implementations have to be idempotent, the API docs should be clearly stating this. Does the API contract the idempotent implementation, or not?

I saw the idempotent hook implementation was a suggestion made, so I am unblocking the MR.

@mrMoZ1 mrMoZ1 merged commit ac1a8b8 into main Jul 25, 2022
@mrMoZ1 mrMoZ1 deleted the person/mzhivkov/writer-message-idempotence branch July 25, 2022 07:38
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.

4 participants