-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
fix(query): Respect the now-time of the compiled query if it's provided #17670
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.
Awesome and glad to see it was an clear cut issue 🎉
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.
Looks good!
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.
@brettbuddin This change looks good. I want to make sure I understand the context of the change. This is how I understand it, am I correct?
There are two HTTP API's for working with queries:
- The public one, that uses the
http.QueryRequest
structure - The private one, that uses the
query.ProxyRequest
structure
The public one is exposed by gateway and the private one by queryd. The private one already has support for specifying the now
time. This change adds support to the public API.
Since tasks now talks through the public API instead of the private API we need support in the public API.
If that is all correct then I have a few requested changes.
- We need to update the swagger spec to indicate that you can specify the
now
time as part of the API. - If this was broken since we migrated to using the gateway for tasks then clearly we do not have an integration test around this feature. Can we add an integration test case for this?
@nathanielc That is correct. I'll get on those changes. |
@nathanielc Think we're all good here. I'll be opening another PR in IDPE for the integration tests. |
We are seeing a mismatch between the expected
now()
time of a Task run and what is actually reported at the time the task is executed. Upon further investigation we found that proxied query requests (via HTTP) aren't using theNow
time provided by the Task Executor.This teaches the HTTP layer to use the
Now
time provided if it is presented. Otherwise, it falls back to calculating a time with the epoch.