Skip to content

fix(pubsub): split large (mod)ACK requests into smaller ones - #9594

Merged
plamut merged 3 commits into
googleapis:masterfrom
plamut:iss-9103
Nov 22, 2019
Merged

fix(pubsub): split large (mod)ACK requests into smaller ones#9594
plamut merged 3 commits into
googleapis:masterfrom
plamut:iss-9103

Conversation

@plamut

@plamutplamut commented Nov 4, 2019

Copy link
Copy Markdown
Contributor

Fixes#9103.

There is a server-side limit on the maximum size of ACK and modACK requests, which can be hit if the leaser tries to manage too many messages in a single requests. This PR avoids the problem by splitting such large requests into multiple smaller requests.

How to test

Steps to reproduce:

  • publish a lot of messages to a topic (significantly more than 3000)
  • subscribe to that topic using a streaming pull and a callback that processes messages for a non-negligible amount of time (e.g. 1 second)
  • observe the DEBUG log

Actual result (before the fix):
The following error can be observed in the log output:

google.api_core.exceptions.InvalidArgument: 400 Request payload size exceeds the limit: 524288 bytes.

Expected result (after the fix):
There is no error, mass-acknowledging leased messages works just fine.

The following script might come helpful:

reproduce_9103.py
importitertoolsimportloggingimportosimportthreadingimporttimefromargparseimportArgumentParserfromgoogleimportapi_corefromgoogle.cloudimportpubsub_v1logging.basicConfig(
format='%(asctime)s %(levelname)s %(threadName)s: %(message)s', level=logging.INFO)
logger=logging.getLogger()
logger.setLevel("DEBUG")
log_format= (
"%(levelname)-8s [%(asctime)s] %(threadName)-33s ""[%(name)s] [%(filename)s:%(lineno)d][%(funcName)s] %(message)s"
)
logging.basicConfig(
level=logging.DEBUG, format=log_format
)
defmsg_handler(message):
"""Message handler."""SLEEP_FOR=1logging.info(f"\x1b[1m[{message.message_id}] got message with content: {str(message.data)} (ack_id: {message.ack_id})\x1b[0m")
logging.info(f"\x1b[1m[{message.message_id}] sleeping for {SLEEP_FOR} seconds\x1b[0m")
time.sleep(SLEEP_FOR)
logging.info(f"\x1b[1m[[{message.message_id}] done sleeping, sending ack()\x1b[0m")
message.ack()
def_publish_messages(publisher, topic_path, batch_sizes):
"""Publish ``count`` messages in batches and wait until completion."""publish_futures= []
msg_counter=itertools.count(start=1)
forbatch_sizeinbatch_sizes:
msg_batch=_make_messages(count=batch_size)
formsginmsg_batch:
future=publisher.publish(
topic_path, msg, seq_num=str(next(msg_counter))
)
publish_futures.append(future)
time.sleep(0.1)
# wait untill all messages have been successfully publishedforfutureinpublish_futures:
future.result(timeout=30)
def_make_messages(count):
messages= [
u"message {}/{}".format(i, count).encode("utf-8")
foriinrange(1, count+1)
]
returnmessagesdefmain():
PROJECT_ID="TODO: set"SUBSCRIPTION_NAME="TODO: set"TOPIC_NAME="TODO: set"publisher=pubsub_v1.PublisherClient()
subscriber=pubsub_v1.SubscriberClient()
topic_path=publisher.topic_path(PROJECT_ID, TOPIC_NAME)
subscription_path=subscriber.subscription_path(PROJECT_ID, SUBSCRIPTION_NAME)
try:
publisher.create_topic(topic_path)
subscriber.create_subscription(subscription_path, topic_path)
exceptapi_core.exceptions.AlreadyExistsasexc:
pass#batch_sizes = (400, 600, 700, 300, 500, 500, 250, 750) # total: 4000batch_sizes= (4000,)
_publish_messages(publisher, topic_path, batch_sizes=batch_sizes)
# now subscribe and do the main part, check for max pending messagestotal_messages=sum(batch_sizes)
flow_control=pubsub_v1.types.FlowControl(max_messages=total_messages)
subscription_future=subscriber.subscribe(
subscription_path, msg_handler, flow_control=flow_control
)
logging.info("Starting streaming pull...") future=subscriber.subscribe(
subscription_path, msg_handler, flow_control=flow_control
)
logging.info("Streming pull started")
time.sleep(5)
try:
future.result()
exceptKeyboardInterrupt:
logging.info("Interrupted, cancelling")
future.cancel()
if__name__=='__main__':
main()

@plamutplamut added the api: pubsub Issues related to the Pub/Sub API. label Nov 4, 2019
@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 4, 2019
There is a server-side limit on the maximum size of ACK and modACK
requests, which can be hit if the leaser tries to manage too many
messages in a single requests.
This commit assures that such large requests are split into multiple
smaller requests.
@plamut

plamut commented Nov 4, 2019

Copy link
Copy Markdown
ContributorAuthor

There might still be an issue here, do not merge yet...

Update: Contrary to Node.js, 3000 appears is too high a limit in Python, thus had to reduce it.

@plamutplamut added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 4, 2019
The previous limit of 3000 seems to be too optimistic, and the request
size limit is still hit. Reducing the batch size to 2500 fixes the
problem.
@plamutplamut removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 4, 2019
Comment threadpubsub/tests/unit/pubsub_v1/subscriber/test_dispatcher.py
Comment threadpubsub/tests/unit/pubsub_v1/subscriber/test_dispatcher.py

@pradnpradn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think there's a bug in splitting. Otherwise, looks good.

The tests should also check that each message is (MOD)ACK-ed exactly
once.
@plamut
plamut merged commit 2439560 into googleapis:masterNov 22, 2019
@plamut
plamut deleted the iss-9103 branch November 22, 2019 20:23
parthea pushed a commit that referenced this pull request Mar 2, 2026
* fix(pubsub): split large (mod)ACK requests into smaller ones
There is a server-side limit on the maximum size of ACK and modACK
requests, which can be hit if the leaser tries to manage too many
messages in a single requests.
This commit assures that such large requests are split into multiple
smaller requests.
* Decrease max ACK batch size to 2500
The previous limit of 3000 seems to be too optimistic, and the request
size limit is still hit. Reducing the batch size to 2500 fixes the
problem.
* Add additional test assertions about sent ACK IDs
The tests should also check that each message is (MOD)ACK-ed exactly
once.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: pubsubIssues related to the Pub/Sub API.cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pub/Sub: Send modify_ack_deadline requests in chunks

3 participants

@plamut@pradn@googlebot