-
Notifications
You must be signed in to change notification settings - Fork 5.9k
Customized concurrent handling #3654
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
Conversation
…am-bot into customized-concurrent-handling
Bibo-Joshi
left a comment
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.
…am-bot into customized-concurrent-handling
harshil21
left a comment
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.
initial review. Will review again after tests are added, implementation looks ok to me
Bibo-Joshi
left a comment
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 the updates! This will come out great :) I left a bunch of new comments, but nothing major IMO.
…am-bot into customized-concurrent-handling
harshil21
left a comment
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.
more comments. But looking much more refined now 👍🏽
Bibo-Joshi
left a comment
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 the new updates! I left smaller comments :) Also the docs build is failing …
Regarding the unit tests on the update processors: TBH I don't see them going in a right direction and I'm not sure how I can explain better what should be covered. I will try and write some tests myself, but please do feel free to ask anything about that if you have questions :)
Bibo-Joshi
left a comment
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.
LGTM :) @harshil21 do you have anything to add?
After merge, the dev-wiki should be updated to
- elaborate on the new processor on the page on concurrency
- mention the
BaseUpdateProcessoron the architecture page. This would be a good chance to convert the graphic to a mermaid diagram
harshil21
left a comment
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.
final review. Looks much more refined and ready. Good job @clot27. Just some tiny errors caught:
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
harshil21
left a comment
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.
nice upgrade!
|
Big thanks to @clot27 for your work and also your patience! 👏 |
Closes #3509 when done.
Checklist for PRs
.. versionadded:: version,.. versionchanged:: versionor.. deprecated:: versionto the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)[ ] Added myself alphabetically toAUTHORS.rst(optional)__all__s