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

Asynchronous code in this library #2059

Closed
waprin opened this issue Aug 5, 2016 · 6 comments
Closed

Asynchronous code in this library #2059

waprin opened this issue Aug 5, 2016 · 6 comments
Assignees
Labels
api: core priority: p2 Moderately-important priority. Fix may not be included in next release.

Comments

@waprin
Copy link
Contributor

waprin commented Aug 5, 2016

In #1942 I added an asynchronous logger to a feature branch using a background thread.

The background thread wants to do writes using the client, but it can't use the same client since it's not thread safe.

Since I couldn't copy the client object, I instead awkwardly copy the http object and try to use the same credentials to authorize it again, then rebiuld the Client from that.

@dhermes is trying to fix the copying issue here:
googleapis/oauth2client#549

although I'm unsure if any of this will work with GAX.

I am also working on adding the ability to write custom metrics to Monitoring (#1867), and I'm realizing that you can't write more than once per second to the same TimeSeries. Ideally, the client library could handle this by queueing the writes and delaying them if necessary, but that again requires asynchronous code so opens similar issues.

I am hoping the repo owners can give me a clearer idea of how they feel about all this so that I don't write a bunch of code that you don't want to merge.

  1. Is the current copying client code in the logging handler acceptable to get merged to master, or will the merge be blocked on the oauth2client work?

  2. Will it completely break with the GAX stuff?

  3. How concerned are we about problems with different versions of Python and different asynchronous models i.e. are there many platforms that this library targets where just starting a background thread won't work?

  4. Is it a good idea or a bad idea to try to make the client thread-safe? Would simplify some of this, and I think uncontested mutex locks are pretty cheap on most platforms.

  5. When there are rate limiting issues like 1 write/s for an API, does this library consider its own responsibility to protect the user from that?

@tseaver @dhermes @daspecster

/cc @jonparrott @rimey @supriyagarg

@waprin
Copy link
Contributor Author

waprin commented Aug 5, 2016

cc @jgeewax if he has time to weigh in

@waprin
Copy link
Contributor Author

waprin commented Aug 5, 2016

Just had someone point out, if someone's trying to write more than 1/s and wants a buffer to protect them, then that buffer will grow indefinitely, unless they sometimes stop writing for the buffer to catchup. Which sounds like an unusual use case so maybe it's not a concern.

@rimey
Copy link
Contributor

rimey commented Aug 8, 2016

For monitoring, you'd want the writes to the service to be performed serially, typically at most about once a minute. How an application developer would want to accomplish that might depend on how they have chosen to handle concurrency in their application.

I think it will be important first of all to offer simple, synchronous client libraries that avoid addressing concurrency. Then we can consider additionally offering some much fancier and highly opinionated thing as an option. It's not at all clear how much commonality there will be in this between monitoring and logging.

@waprin
Copy link
Contributor Author

waprin commented Aug 8, 2016

SGTM for monitoring first pass, still think it's something worth thinking about, esp given feedback we heard from people familiar with the other client libs.

@waprin
Copy link
Contributor Author

waprin commented Aug 8, 2016

I am wondering if the simplest approach is:

  1. Just merge the synchronous logging handler into this library
  2. Just do the simplest Monitoring code in this library

Any other work can be done on separate repos that pull this repo in, that are more experimental and less official.

@lukesneeringer lukesneeringer added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Apr 19, 2017
@lukesneeringer
Copy link
Contributor

Hello,
One of the challenges of maintaining a large open source project is that sometimes, you can bite off more than you can chew. As the lead maintainer of google-cloud-python, I can definitely say that I have let the issues here pile up.

As part of trying to get things under control (as well as to empower us to provide better customer service in the future), I am declaring a "bankruptcy" of sorts on many of the old issues, especially those likely to have been addressed or made obsolete by more recent updates.

My goal is to close stale issues whose relevance or solution is no longer immediately evident, and which appear to be of lower importance. I believe in good faith that this is one of those issues, but I am scanning quickly and may occasionally be wrong. If this is an issue of high importance, please comment here and we will reconsider. If this is an issue whose solution is trivial, please consider providing a pull request.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: core priority: p2 Moderately-important priority. Fix may not be included in next release.
Projects
None yet
Development

No branches or pull requests

4 participants