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

Perform all remote fetches in parallel #1026

Merged
merged 4 commits into from Dec 22, 2014

Conversation

@bmhatfield
Copy link
Member

@bmhatfield bmhatfield commented Nov 12, 2014

This builds on the bulk fetch improvements in #1010

@bmhatfield
Copy link
Member Author

@bmhatfield bmhatfield commented Nov 12, 2014

@lamont I'm going to be checking out if this helps improve some still-slow performance I've been seeing. This may dramatically speed up your experience, given that you were previously querying from 10 hosts sequentially.

@bmhatfield
Copy link
Member Author

@bmhatfield bmhatfield commented Nov 12, 2014

I'm going to un-WIP this; I have deployed it to production and I can confirm the effect is as intended.

@bmhatfield bmhatfield changed the title WIP: Perform all remote fetches in parallel Perform all remote fetches in parallel Nov 12, 2014
@jraby
Copy link

@jraby jraby commented Nov 28, 2014

simply fyi, issue #795 had a different approach, using multiprocessing.pool.ThreadPool to do the fetches.

(I'm currently forward porting/testing this patch to the new bulk fetch code)

@deniszh
Copy link
Member

@deniszh deniszh commented Dec 7, 2014

Just tested this PR, works fine, I think we still need to merge it before 0.9.13 release. It's quite complimentary to #1010 too. @jraby's approach is nice too, maybe we need to port it to master though...

@brutasse
Copy link
Member

@brutasse brutasse commented Dec 8, 2014

If this gets merged in 0.9.x it really needs to go to master too. Usually the workflow is to commit in master first then backport to 0.9.x. It's easier to ensure branches don't diverge too much with this process.

@deniszh
Copy link
Member

@deniszh deniszh commented Dec 8, 2014

I'm suspecting that master is already too much diverged. At least fetch
code logic is completely different from 0.9.x
I tried to port carbon query bulk and failed.
My point that 0.9.13 is last release in 0.9.x, right? After 0.9.13 all
development will be switched to master and 0.9.x will be frozen and all
valuable PRs there will be frozen too. So, why not let users of current
production to get speed-up improvement now and not
"i-do-not-know-how-many-month-later-after-first-1.0.0-release"?
IMO current " branch split" situation is really poisonous for development
process.

8:32, пн, 08.12.2014, Bruno Renié notifications@github.com:

If this gets merged in 0.9.x it really needs to go to master too. Usually
the workflow is to commit in master first then backport to 0.9.x. It's
easier to ensure branches don't diverge too much with this process.


Reply to this email directly or view it on GitHub
#1026 (comment)
.

szibis added a commit to szibis/graphite-web that referenced this pull request Dec 9, 2014
@bmhatfield
Copy link
Member Author

@bmhatfield bmhatfield commented Dec 17, 2014

Bump on this?

I can confirm that Master is far too diverged for this to make sense in it's current form. I am happy to take the lead on carrying the concept over to master as we start thinking about how to make it the sole branch for new development, but it takes re-understanding behavior and redoing this set of work to make it happen.

EDIT: @jraby thanks for the heads up. Reviewing 795, it appears to be building on concepts pre-1010. There's no particular need for a thread pool at this stage, because in my opinion, even a huge cluster (100 nodes?) is not likely to incur what I would call concerning thread overhead (as compared with the performance gains).

@obfuscurity
Copy link
Member

@obfuscurity obfuscurity commented Dec 22, 2014

@bmhatfield I've invited you to the committers team. Merry Christmas. 😉🎄🎁

Your first official duty is to open an issue to port this functionality to master. 😁

obfuscurity added a commit that referenced this pull request Dec 22, 2014
Perform all remote fetches in parallel
@obfuscurity obfuscurity merged commit af0199e into graphite-project:0.9.x Dec 22, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@obfuscurity obfuscurity mentioned this pull request Dec 22, 2014
13 of 13 tasks complete
obfuscurity added a commit that referenced this pull request Dec 22, 2014
Adding #1026 to 0.9.13 release notes
obfuscurity added a commit that referenced this pull request Dec 22, 2014
Adding #1026 to 0.9.13 release notes (master branch)
jraby added a commit to datacratic/graphite-web that referenced this pull request Dec 30, 2014
jraby added a commit to datacratic/graphite-web that referenced this pull request Dec 30, 2014
@bmhatfield bmhatfield deleted the bmhatfield:0.9.x-threading branch Jun 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.