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

Add clarification on multithreading and multiprocessing for resources #2848

Merged
merged 1 commit into from
May 19, 2021

Conversation

ryansonshine
Copy link
Contributor

Addresses boto/botocore#1246

@mattsb42-aws
Copy link
Contributor

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."

@ryansonshine
Copy link
Contributor Author

Thanks @mattsb42-aws for your feedback, I definitely agree! I'll put in a revision today.

@ryansonshine ryansonshine force-pushed the botocore-1246 branch 2 times, most recently from abb7656 to 55306e5 Compare May 13, 2021 18:28
@robeady
Copy link

robeady commented May 13, 2021

assuming the clients are not used to interact with the underlying Session

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

@ryansonshine
Copy link
Contributor Author

@robeady Great callout, thanks for the feedback! I've added additional clarification on this. Let me know if that makes it clear.

@robeady
Copy link

robeady commented May 17, 2021

@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

Copy link
Contributor

@nateprewitt nateprewitt left a 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.

@ryansonshine
Copy link
Contributor Author

ryansonshine commented May 18, 2021

@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?

@ryansonshine ryansonshine requested a review from nateprewitt May 18, 2021 13:51
Copy link
Contributor

@nateprewitt nateprewitt left a 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.

@nateprewitt nateprewitt merged commit 12f5016 into boto:develop May 19, 2021
@ryansonshine ryansonshine deleted the botocore-1246 branch May 19, 2021 18:57
aws-sdk-python-automation added a commit that referenced this pull request May 19, 2021
* release-1.17.76:
  Bumping version to 1.17.76
  Add changelog entries from botocore
  Add clarification on multithreading and multiprocessing for resources (#2848)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants