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

feat: Implement committer #13

Merged
merged 4 commits into from Sep 14, 2020
Merged

feat: Implement committer #13

merged 4 commits into from Sep 14, 2020

Conversation

dpcollins-google
Copy link
Collaborator

Also small fix to retrying connection so it doesn't leak reads/writes from previous connections.

Also small fix to retrying connection so it doesn't leak reads/writes from previous connections.

class Committer(AsyncContextManager):
"""
A Committer is able to commit subscribers completed offsets.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: "subscribers' "?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

google/cloud/pubsublite/internal/wire/committer_impl.py Outdated Show resolved Hide resolved
await self._stop_loopers()
await connection.write(StreamingCommitCursorRequest(initial=self._initial))
response = await connection.read()
if "initial" not in response:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we're not doing substrings to check for errors? This happens in a few places.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't substrings.... its checking for a subfield in the response. This is how you're supposed to check for oneof field presence in the new proto lib.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah gotcha!

setup.py Show resolved Hide resolved
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 14, 2020
@dpcollins-google dpcollins-google merged commit aa9aca8 into master Sep 14, 2020
@anguillanneuf anguillanneuf deleted the committer branch March 25, 2022 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants