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

Add synchronous jobs #10

Merged
merged 6 commits into from
Jan 26, 2022
Merged

Add synchronous jobs #10

merged 6 commits into from
Jan 26, 2022

Conversation

xxorde
Copy link
Contributor

@xxorde xxorde commented Apr 25, 2017

We needed a possibility to execute some jobs / queries synchronous every time /metrics is requested.
So I added the infrastructure to execute jobs with an interval <=0 synchronous when the http-handler is called.

interval: '0s' # an interval <= 0 will make the queries synchronous

I hope my patch does not brings any drawback, just had to change some parts after importing your current version.

It would be great if you are interested in this feature and we could start a discussion.

@xxorde xxorde changed the title Add synchronous job Add synchronous jobs Apr 25, 2017
@dominikschulz dominikschulz self-requested a review April 25, 2017 13:14
@xxorde
Copy link
Contributor Author

xxorde commented Jun 19, 2017

@dominikschulz what do you think about the feature in general and my patch?

Update: I rebased all commits to only have one and removed the debug output by using NewNopLogger(). I hope the PR is much easier to review now.

@xxorde xxorde force-pushed the master branch 2 times, most recently from d32bdb5 to 643bea6 Compare June 30, 2017 13:16
@dominikschulz
Copy link
Contributor

dominikschulz commented Jul 1, 2017

Sorry for the long delay. I was thinking about this PR a few times already, but I don't really like the current approach. It feels to hacky, adding too many special cases.

We understand the need for synchronous queries, but I'd like to find a way that feels more natural.

@bobrik
Copy link
Contributor

bobrik commented Jul 2, 2017

I'd really love to see synchronous queries. Async queries can easily misalign with scraping intervals and cause steady rates to appear as sawtooth.

@xxorde
Copy link
Contributor Author

xxorde commented Jul 3, 2017

@dominikschulz I would really appreciate if you can find a more clean way to do this. For someone who is a more experienced developer and knows the involved libraries it might just be a few lines of code. There is not much logic involved.

What do you think about the way to identify a synchronous job by setting the interval to '0' ?

@bobrik that was our problem and the intention to add that feature!

@df7cb
Copy link

df7cb commented Jun 19, 2018

I just pushed a rebased and polished up version of this patch to the original location. (Sorry for deleting it some time ago.) It now also updates the README file to mention interval 0.
Could you maybe have another look to see if it fits your idea, or what would need changing?
We are running this patch in https://github.com/credativ/elephant-shed and would love to see it merged.
Thanks!

marthjod and others added 6 commits January 9, 2020 16:17
Fixes justwatchcom#19

Signed-off-by: Dominik Schulz <[email protected]>
- athena driver-based queries may break late (ie, not during connect)
they do not conform to the RFCs for web URLs, and Go 1.12.8+ enforce
this.

Instead of the parsed URL, keep the original connection string.

This could probably be generalized for other databases, but I don't know
if they have the same problem.

Signed-off-by: Matthias Rampke <[email protected]>
Signed-off-by: Matthias Rampke <[email protected]>
@df7cb df7cb deleted the branch justwatchcom:master January 11, 2022 15:48
@dewey dewey merged commit f625b2b into justwatchcom:master Jan 26, 2022
@df7cb
Copy link

df7cb commented Feb 4, 2022

Fwiw, #51 is about snowflake support, not synchronous jobs, so this PR should have been left open.

@dewey
Copy link
Member

dewey commented Feb 4, 2022

Thanks, I'll take a look what went wrong there :\

@mbanck-ntap
Copy link

@dewey Well, looks like this PR is still marked merged while in fact it has not been merged yet?

@mbanck-ntap
Copy link

I have re-submitted this as #131 now as untangling this one looks difficult.

@dewey
Copy link
Member

dewey commented Jul 17, 2024

Thank you, much appreciated! I had a hard time to figure out how to undo that.

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.

7 participants