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.

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

pubsub large message #4870

Closed
wants to merge 1 commit into from
Closed

pubsub large message #4870

wants to merge 1 commit into from

Conversation

chemelnucfin
Copy link
Contributor

No description provided.

@chemelnucfin chemelnucfin added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. do not merge Indicates a pull request not ready for merge, due to either quality or timing. api: pubsub Issues related to the Pub/Sub API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. status: investigating The issue is under investigation, which is determined to be non-trivial. labels Feb 12, 2018
@chemelnucfin chemelnucfin self-assigned this Feb 12, 2018
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 12, 2018
@chemelnucfin chemelnucfin mentioned this pull request Feb 13, 2018
Closed
@chemelnucfin chemelnucfin reopened this Mar 3, 2018
@chemelnucfin chemelnucfin force-pushed the pubsub_large_message branch 3 times, most recently from cf8eada to 5117f56 Compare March 3, 2018 07:16
@chemelnucfin chemelnucfin removed the priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. label Mar 3, 2018
@chemelnucfin chemelnucfin added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. status: investigating The issue is under investigation, which is determined to be non-trivial. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Mar 4, 2018
@chemelnucfin chemelnucfin changed the title WIP: pubsub large message pubsub large message Mar 4, 2018
tseaver
tseaver suggested changes Mar 16, 2018
View reviewed changes
pubsub/google/cloud/pubsub_v1/publisher/batch/thread.py
@@ -97,7 +97,7 @@ def make_lock():
Returns:
_thread.Lock: A newly created lock.
"""
return threading.Lock()
return threading.RLock()

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

pubsub/google/cloud/pubsub_v1/publisher/batch/thread.py
@@ -204,6 +204,8 @@ def _commit(self):
self._messages,
)
end = time.time()
self._client._publish_count += len(self._messages)
self._client._commit_count += 1

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

pubsub/google/cloud/pubsub_v1/publisher/batch/thread.py
return self._commit()

if self._size:
return self._commit()

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

pubsub/google/cloud/pubsub_v1/publisher/batch/thread.py
@@ -278,7 +281,8 @@ def publish(self, message):

# Try to commit, but it must be **without** the lock held, since
# ``commit()`` will try to obtain the lock.
if num_messages >= self._settings.max_messages:
if (num_messages >= self._settings.max_messages
or self._size >= self._settings.max_bytes):

This comment was marked as spam.

Sign in to view

pubsub/google/cloud/pubsub_v1/publisher/client.py
~.pubsub_v1.batch.Batch: The batch object.
"""
return self._batch(topic, create, autocommit)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

pubsub/nox.py
session.run('py.test', '--quiet', 'tests/system.py')
session.run('py.test',
'--quiet',
'tests/system.py')

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

pubsub/google/cloud/pubsub_v1/publisher/client.py
future = None
while future is None:
future = batch.publish(message)
self._batches[topic] = backup

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
@@ -72,7 +72,7 @@ def test_init_infinite_latency():
assert batch._thread is None


@mock.patch.object(threading, 'Lock')
@mock.patch.object(threading, 'RLock')
def test_make_lock(Lock):

This comment was marked as spam.

Sign in to view

pubsub/tests/unit/pubsub_v1/publisher/batch/test_thread.py
@@ -203,6 +203,7 @@ def test_blocking__commit_wrong_messageid_length():

def test_monitor():
batch = create_batch(max_latency=5.0)
batch._size = 1

This comment was marked as spam.

Sign in to view

@chemelnucfin chemelnucfin mentioned this pull request Mar 16, 2018
Merged
@tseaver
Copy link
Contributor

tseaver commented May 17, 2018

This PR doesn't address the order-of-publishing concerns expressed by @kir-titievsky:

  • If a message itself exceeds max_bytes, then flush any messages in the batch (via commit) and send the oversize message in a separate commit.
  • Otherwise, if adding a message to a batch would cause the aggregate size to exceed max_bytes, the current messages should be flushed (via commit) before adding the new message to the now-empty batch.

@tseaver tseaver closed this May 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tseaver tseaver tseaver requested changes

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

Assignees

@chemelnucfin chemelnucfin

Labels
api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chemelnucfin @tseaver @googlebot