-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
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
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
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
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 👍🏽
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 :)
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
BaseUpdateProcessor
on the architecture page. This would be a good chance to convert the graphic to a mermaid diagram
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>
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:: version
or.. deprecated:: version
to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)[ ] Added myself alphabetically toAUTHORS.rst
(optional)__all__
s