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

unused message arg in Batch.will_accept() #105

Closed
IlyaFaer opened this issue May 27, 2020 · 1 comment · Fixed by #108
Closed

unused message arg in Batch.will_accept() #105

IlyaFaer opened this issue May 27, 2020 · 1 comment · Fixed by #108
Assignees
Labels
api: pubsub Issues related to the googleapis/python-pubsub API. type: cleanup An internal cleanup or hygiene concern.

Comments

@IlyaFaer
Copy link

Looks like message arg is never used by will_accept() method:

def will_accept(self, message):
"""Return True if the batch is able to accept the message.
In concurrent implementations, the attributes on the current batch
may be modified by other workers. With this in mind, the caller will
likely want to hold a lock that will make sure the state remains
the same after the "will accept?" question is answered.
Args:
message (~.pubsub_v1.types.PubsubMessage): The Pub/Sub message.
Returns:
bool: Whether this batch can accept the message.
"""
# If this batch is not accepting messages generally, return False.
if self.status != BatchStatus.ACCEPTING_MESSAGES:
return False
# If this message will make the batch exceed the ``max_messages``
# setting, return False.
if len(self.messages) >= self.settings.max_messages:
return False
# Okay, everything is good.
return True

Most likely it should be deleted to avoid any confusion, but I suppose it should be done with deprecation. Batch() is not something pretending to be manually used by users, but who knows.

@pradn, please, comment

@IlyaFaer IlyaFaer added api: pubsub Issues related to the googleapis/python-pubsub API. type: cleanup An internal cleanup or hygiene concern. labels May 27, 2020
@IlyaFaer IlyaFaer self-assigned this May 27, 2020
@pradn
Copy link
Contributor

pradn commented May 27, 2020

will_accept is unnecessary, and partially duplicates the checks done in thread.Batch.publish. We should just incorporate it into publish().

I don't think people rely on being able to override Batch. We have made a bunch of changes to Batch that they would have to keep up with, so it's not advisable anyway. So it should be fine to just modify the interface without deprecation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the googleapis/python-pubsub API. type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants