Skip to content

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

Merged
dhermes merged 3 commits intogoogleapis:masterfrom
dhermes:pubsub-fix-4575
Dec 19, 2017
Merged

Checking Pub / Sub Batch.will_accept in thread-safe way.#4616
dhermes merged 3 commits intogoogleapis:masterfrom
dhermes:pubsub-fix-4575

Conversation

@dhermes
Copy link
Copy Markdown
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
parthea pushed a commit that referenced this pull request Mar 2, 2026
@dhermes
Checking Pub / Sub Batch.will_accept in thread-safe way. (#4616)
da7f6a8 
Fixes #4575.
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