dashbitco / broadway_cloud_pub_sub

A Broadway producer for Google Cloud Pub/Sub
Apache License 2.0
70 stars 24 forks source link

Substitute Hackney by Mint #62

Closed v0idpwn closed 2 years ago

v0idpwn commented 3 years ago

See #51

Still got to do some work on testing and reviewing docs and dead code. I'd like to benchmark this to see performance implications, do you have any tip on how I can do it?

josevalim commented 3 years ago

Is Tesla+Mint keeping a connection open in this case? Or does it spawn a new connection per request?

v0idpwn commented 3 years ago

Nope. And, unless I'm wrong, I think we can't :(

To create a connection, we could: a) do the first request without a connection and store the connection it returns for subsequent requests. b) call Mint.HTTP.connection on init, and store this connection on producer state

To do a), we would need the GCP library to return the conn after it makes requests, and also update the Client behaviour, so it allows us to return the connection along with the messages (possibly breaking change). Unfortunately, the GCP library returns us a "parsed response" which doesn't include the HTTP connection. :|

To do b), we need request info so we can open the connection. I don't think we have it available, since its all abstracted out in the GCP library. Even if we managed to do that, I think it would not work, since Mint returns a new, updated connection after requests, and we would be ignoring it

I think opening a new connection for every request is a big problem and we shouldn't merge the PR this way.

josevalim commented 3 years ago

Exactly. If we can't address this, then we should either default to Finch or continue with Hackney. I like Finch because it is more efficient and does less copying and we could move both this and SQS to it.

v0idpwn commented 3 years ago

I like this option. I'll try to port this PR to finch and, if it works well, I can do the equivalent on sqs.

Still, its unfortunate that the library limits us so much :(

mcrumm commented 3 years ago

There is also the option of writing a new client that does not use the GoogleApi abstractions. Honestly I am not sure how much work it would be, but it could remove 2 layers of indirection (Connection + Tesla) so maybe worth exploring? @josevalim @wojtekmach

josevalim commented 3 years ago

I am all in for someone exploring. I assume/hope it is rather straightforward given we are using few endpoints?

v0idpwn commented 3 years ago

I'm working on porting to mint without the Google APIs lib. I'll let you know how it goes.

v0idpwn commented 3 years ago

Question: as mint requires a bit of state for maintaining the connection, I see three immediate options:

  1. Changing the Client behaviour significantly to allow returning this state in the calls: changing it without explicit compatibility break is quite dangerous, imo, as some callbacks return values were labeled as any() (they weren't used), and someone's else client may be incompatible with the new behaviour, even if we'd try not to introduce compatibility breaks
  2. Using another process to handle this state, which implies copying
  3. Using the process dictionary, which is probably a terrible idea 👀

What do you think?

josevalim commented 3 years ago

It would be 1. We can still do breaking changes before 1.0.