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

fix(query): Respect the now-time of the compiled query if it's provided #17670

Merged
merged 3 commits into from
Apr 8, 2020

Conversation

brettbuddin
Copy link
Contributor

@brettbuddin brettbuddin commented Apr 8, 2020

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 the Now 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.

Copy link
Contributor

@stuartcarnie stuartcarnie left a 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 🎉

Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

Looks good!

@brettbuddin brettbuddin requested review from a team and ethanyzhang and removed request for a team April 8, 2020 16:08
Copy link
Contributor

@nathanielc nathanielc left a 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.

  1. We need to update the swagger spec to indicate that you can specify the now time as part of the API.
  2. 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?

@brettbuddin
Copy link
Contributor Author

@nathanielc That is correct. I'll get on those changes.

@brettbuddin brettbuddin requested a review from a team as a code owner April 8, 2020 17:32
@brettbuddin brettbuddin requested review from 121watts and nathanielc and removed request for a team April 8, 2020 17:32
@brettbuddin
Copy link
Contributor Author

@nathanielc Think we're all good here. I'll be opening another PR in IDPE for the integration tests.

@brettbuddin brettbuddin merged commit f994131 into master Apr 8, 2020
@brettbuddin brettbuddin deleted the bb/task-send-now branch April 8, 2020 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants