Skip to content

Checking Pub / Sub Batch.will_accept in thread-safe way. #4616

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.

Sign up for GitHub

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

Jump to bottom
Merged
dhermes merged 3 commits into from
Dec 19, 2017
Merged

Checking Pub / Sub Batch.will_accept in thread-safe way. #4616

dhermes merged 3 commits into from
Dec 19, 2017

Conversation

@dhermes
Copy link
Contributor

@dhermes dhermes commented Dec 18, 2017

Unit tests will be coming in the next commit.

Fixes #4575.

@dhermes dhermes added the api: pubsub Issues related to the Pub/Sub API. label Dec 18, 2017
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 18, 2017
dhermes
dhermes commented Dec 18, 2017
View reviewed changes
pubsub/google/cloud/pubsub_v1/publisher/batch/thread.py
# in a separate thread.

# Sleep for however long we should be waiting.
time.sleep(self._settings.max_latency)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

pubsub/tests/unit/pubsub_v1/publisher/batch/test_thread.py

def test_publish_max_messages():
batch = create_batch(max_messages=4)
def test_publish_not_will_accept():

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes dhermes mentioned this pull request Dec 18, 2017
Merged
lukesneeringer
lukesneeringer reviewed Dec 19, 2017
View reviewed changes
pubsub/google/cloud/pubsub_v1/publisher/batch/base.py Outdated
# If this message will make the batch exceed the ``max_bytes``
# setting, return False.
if self.size + message.ByteSize() > self.settings.max_bytes:
if self.size + message.ByteSize() >= self.settings.max_bytes:

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

lukesneeringer
lukesneeringer reviewed Dec 19, 2017
View reviewed changes
pubsub/google/cloud/pubsub_v1/publisher/batch/thread.py
f = futures.Future()
self._futures.append(f)
return f
return future

This comment was marked as spam.

Sign in to view

lukesneeringer
lukesneeringer reviewed Dec 19, 2017
View reviewed changes
pubsub/tests/unit/pubsub_v1/publisher/batch/test_thread.py Outdated
name='Thread-CommitBatchPublisher',
target=batch._commit,
)
Thread.return_value.start.assert_called_once_with()

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes
Checking Pub / Sub Batch.will_accept in thread-safe way.
802ca1a 
Unit tests will be coming in the next commit.

Fixes googleapis#4575.
@dhermes
Adding unit tests for previous commit.
a8307d8
@dhermes
Review feedback.
cbaea8b 
- Changing back `>=` to `>` (I was misreading a `curr_size + EXTRA > MAX`
  as `curr_size > MAX`)
- Putting `assert_*()` calls on `Thread` mock back in the context
  manager
@dhermes dhermes merged commit 4a9a950 into googleapis:master Dec 19, 2017
@dhermes dhermes deleted the pubsub-fix-4575 branch December 19, 2017 16:16
@kir-titievsky kir-titievsky mentioned this pull request Dec 19, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@lukesneeringer lukesneeringer lukesneeringer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lukesneeringer lukesneeringer

@tseaver tseaver

Labels

api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dhermes @lukesneeringer @tseaver @googlebot