-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 clarification on multithreading and multiprocessing for resources #2848
Conversation
1c0ae8d
to
354ad25
Compare
354ad25
to
ef48773
Compare
Love the attention to detail on calling out each kind of thing that people encounter and detailing what its situation is and why. That's great for answering the question of "what happens if I...", but I think it would be useful to also call out in the intro to the threading section that the simplest answer is to create a new instance for each thread. The current intro talks about Resources specifically, but then the updated note talks about Sessions and Clients as well. Lead with what you recommend people do, then explain why. :) As a developer reading through the full explanation, my take-away is "just assume none of these are thread-safe, even though that's not universally true." |
Thanks @mattsb42-aws for your feedback, I definitely agree! I'll put in a revision today. |
abb7656
to
55306e5
Compare
Would it be possible to clarify what this means? If a client has a reference to a session, it would seem that any interaction with a client could potentially interact with the session... but I assume that's not quite what's intended here |
55306e5
to
e0bde37
Compare
@robeady Great callout, thanks for the feedback! I've added additional clarification on this. Let me know if that makes it clear. |
excellent, that's much clearer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting this PR, @ryansonshine! I've left some feedback about organization with some outlines as possible ideas. Let me know if you have any thoughts or concerns.
e0bde37
to
e46955a
Compare
@nateprewitt Thanks for the feedback, I agree this definitely has a much better structure. I've updated the PR with your suggestions. Do you think it would be helpful for us to include cross references to the Multithreading and multiprocessing sections between the resources, sessions, and clients or do you think this would add too much noise? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ryansonshine! I think this looks like it's ready to ship.
As for linking to the other sections, I think since the guides are all under the same top level section, they should be easy to find for now. We'll wait to see if there's additional feedback with the new approach and can revisit cross-referencing later.
* release-1.17.76: Bumping version to 1.17.76 Add changelog entries from botocore Add clarification on multithreading and multiprocessing for resources (#2848)
Addresses boto/botocore#1246