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

removing kafka event in DCS plugin for EOF received from DCS #615

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

justonedev1
Copy link
Collaborator

The reason for this is described in ticket OCTRL-924

@knopers8 knopers8 marked this pull request as ready for review September 23, 2024 12:01
@knopers8 knopers8 merged commit c0e2133 into master Sep 23, 2024
2 checks passed
@knopers8 knopers8 deleted the remove_done_error branch September 23, 2024 12:02
Comment on lines -1419 to -1429
the.EventWriterWithTopic(TOPIC).WriteEvent(&pb.Ev_IntegratedServiceEvent{
Name: call.GetName(),
OperationName: call.Func,
OperationStatus: pb.OpStatus_ONGOING,
OperationStep: "perform DCS call: StartOfRun",
OperationStepStatus: pb.OpStatus_DONE_ERROR,
EnvironmentId: envId,
Payload: string(payloadJson[:]),
Error: logMsg,
})

Copy link
Member

@teo teo Sep 30, 2024

Choose a reason for hiding this comment

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

I agree with this change. Beyond the question of whether the stream io.EOF was timely or not from the DCS side, it could be argued that a stream being closed either way should not generate a Kafka event, because it concerns a technicality of DCS-ECS communication and says nothing about the actual state of the DCS.

However, the behaviour should be consistent across all DCS operations. @justonedev1 could you please make sure this change also applies to PFR and EOR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, I will do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants