-
Notifications
You must be signed in to change notification settings - Fork 2
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
Buffer refactor #132
Buffer refactor #132
Conversation
3d17198
to
a912cf2
Compare
In addition to main PR descriptions, here are more stuff done:
|
And another update:
|
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.
whew... that's a lot of code ... I don't feel I have finished the review and probably it would be good to touch base on a call anyway but here are some comments that I have had...
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.
please see my comments... otherwise, lgtm afaict
"Target items is now `%s` which is not below min size `%d`, requesting fill not" | ||
" needed", | ||
items_count, | ||
"Proposals count %d is not below minimum size %d, skipping fill", |
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.
why "not below" instead of "above"?
"Proposals count %d is not below minimum size %d, skipping fill", | |
"Proposals count %d is above minimum size %d, skipping fill", |
Co-authored-by: shadeofblue <[email protected]>
What I've done:
golem.managers.proposals.plugins
Waiter
to take the heavy lifting from simple bufferSingleUseSemaphore
to take the heavy lifting from bacground fill buffergolem.utils
to single name spacegolem.utils.asyncio
, as stuff started to be mixed with other utilsNotable remarks:
SimpleBuffer
covers quite fully functionality fromErrorReportingQueue
, I would remove the second one in favor of the first, but team have different opinions about this, so it will be left as is