Parallel requests

[keolo]

Determine if parallel requests should be abstracted to another gem, leave as is, or force every request to use hydra?

[whatthewhat]

We're not using the parallel feature, so I can't really give feedback on that, but it would have been great if the typhoeus dependency was optional. I think typhoeus is great, but when every API client gem brings it's own HTTP client it can become a mess quickly 😄

I'd be happy to help with this, whether it's replacing typhoeus completely or extracting a mixpanel_client-parralel gem and making it an optional dependency.

[keolo]

I completely agree. Having that extra dependency in there has bothered me for quite some time. Let me know if you'd like to tackle this. I'm open to your suggestions for an implementation.

[whatthewhat]

At first I was thinking that it might be enough to add a very simple adapter layer for http clients. e.g. something like this:

client = Mixpanel::Client.new(
  api_secret: 'secret',
  http_client: DefaultHttpClient # or some ParallelHttpClient that would be the default if you install mixpanel_client-typhoeus
)

client.request(...)

But that does not actually work for parallel requests without monkeypatching the run_parallel_requests method into Mixpanel::Client which is not ideal.

A cleaner solution could be introducing separate Mixpanel::Client and Mixpanel::ParallelClient - but that's much more of a breaking change. What are your thoughts on introducing breaking changes in general? With a bit more work we could also preserve the current public API of

Mixpanel::Client.new(
  api_secret: 'changeme',
  parallel:   true
)

And the only thing one would need to do is to add another gem to their Gemfile to get parallel support back.

I'll probably have some time in January to play around with this, anyway.

[keolo]

I actually like the idea of using the adapter pattern. Could we not require gems that extend this library to implement an agreed upon method e.g. get?

Guard does this in a nice way with it's plugins: https://github.com/guard/guard

I don't mind breaking changes. The 4.x version should work well for some time if people really need it. I think there's other breaking changes to make anyway. E.g. this probably should be named Mixpanel::DataClient to make it clearer that this client is for the data api.

[whatthewhat]

Could we not require gems that extend this library to implement an agreed upon method e.g. get?

Yep, this is what I was thinking at first. The slight complication is that adapters with parallel support (typhoeus) would need a broader API surface then just get.

I think we would need these 3 methods as the contract between the gem and adapters to support parallel requests:

• a method to run a GET request synchronously (get?)
• a method to add a request to a queue to be executed in parallel (enqueue/add_parallel_request?)
• a method to add execute all the request in the queue (commit/run/flush?)

If an adapter does not support the last 2 methods we can raise an exception that explains that..

And then in terms of actually using the gem it could look something like this:

# With the default Net::HTTP adapter:
client = Mixpanel::Client.new(api_secret: 'secret')
client.request(...) # makes a synchronous request

# With a typhoeus adapter:
parallel_client = Mixpanel::Client.new(
  api_secret: 'secret',
  http_adapter: TyphoeusAdapter # comes from a gem
)

parallel_client.request(...) # makes a synchronous request

# or
parallel_client.enqueue(...)
parallel_client.enqueue(...)
parallel_client.flush # runs 2 requests in parallel

# or could also add some syntactic sugar to make sure parallel requests are always flushed
parallel_client.parallel do |client|
  client.request(...)
  client.request(...)
  client.request(...)
end # runs 3 requests in parallel

As for the class name, what about MixpanelData::Client? Could prevent potential namespace issues with https://github.com/mixpanel/mixpanel-ruby

[keolo]

Hmm, what if only the Parallel adapter implemented the enqueue and flush methods? This way those methods would return a NoMethodError if client.class == MixpanelData. E.g.

client = MixpanelData::Client.new(api_secret: 'secret')
client.enqueue(...) # => NoMethodError
client.flush(...) # => NoMethodError

Those are all great ideas by the way!

[whatthewhat]

Just a note that this is still in my TODO list, just haven't had much time lately :)

[keolo]

Hi Mikhail,

I appreciate the update. No worries!! It's been on my mind to work on it too but sometimes life has other plans. :D

This month has been especially busy for me at work (we're launching a completely revamped platform). But I should have more free time in a couple of weeks.

On Tue, Feb 28, 2017 at 9:36 AM, Mikhail Topolskiy <notifications@github.com

wrote:

Just a note that this is still in my TODO list, just haven't had much time lately :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/keolo/mixpanel_client/issues/58#issuecomment-283109987, or mute the thread https://github.com/notifications/unsubscribe-auth/AAASzhfu8zUrbqh1Tmqo4duylOCh9lRDks5rhFsqgaJpZM4LYR7e .

-- Best, Keolo 213.325.1033