-
Notifications
You must be signed in to change notification settings - Fork 23
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
[core] ODC: Send SOR/EOR timestamps as FMQ properties #658
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have one question, but code itself looks legitimate
} | ||
|
||
arguments := make(map[string]string) | ||
arguments["run_end_time_ms"] = runEndTimeMs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question about this. Is this documented parameter that just wasn't used before, or are you introducing something new?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the question. This parameter was not sent before to ODC, thus it could not be used, so it is something new.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not being specific enough. I know that it wasn't used before. I am asking whether run_end_time_ms
is existing parameter of ODC or something totally new, that needs to be implemented in ODC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah no, these parameters are transparent to ODC, it just receives them and propagates to the tasks on EPNs. It's just key-values that tasks may use if they want to, e.g. QC is going to use it if it's available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, thanks
OCTRL-987
This involved extracting
setProperties
as a separate helper to share it betweenStart
andStop
, and adding it toStop
, which was not the case. Contrarily toStart
, we do not abandonodc.Stop
in casesetProperties
fails, so we still have an attempt to stop the run.Tested on staging, it looks like
setProperties
takes 50ms when using 2 EPNs. Looking at the past logs in production, it usually takes no more than 100ms, so I don't think anyone should complain, while this lets us use correct EOR time in QC.