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: create experimental message dispatcher #2572

Closed

pongad wants to merge 3 commits into googleapis:master from pongad:pubsub-dup

Contributor

pongad commented Nov 2, 2017 •

edited

Loading

EXPERIMENTAL
I pulled it into a separate class so it may coexist with the original.
We can submit this, but it needs more testing before production use.

We emphasize simplicity, not optimal performance.
Of course, performance is still important,
hence the many concurrent data structures used here.

Major differences from the original and some justifications:

It modacks immediately after receiving messages
to take advantage of server-side duplicate-mitigation
feature that should soon be available.
In the original impl, we create many dispatchers.
They share some data structures and keep others separate.
I find this too confusing.
The new impl creates only one dispatcher, to be shared
among all connections. This obviously increases contention
but should be at least partially alleviated by some lock-free
algorithms used here.
It makes deadline a constant of 1 minute.
With the dup-mitigation feature, the server might need to
wait on the order of minutes before it could redeliver messages.
I opine that setting the deadline to 1 minute shouldn't drastically
worsen the redelivery latency.
Also unlike the original, it does not periodically adjust deadline.
I have some ideas on how this could be simply implemented;
we can add this feature back if necessary.
Modack time is also set to 1 minute and doesn't exponentially back
off. Since the deadline is already 1 minute, it seems silly to
bicker over a few extra seconds. [1]
Modacks run on fixed schedule, giving 15 seconds padding,
and modacks all pending messages, not just the ones about to expire.
While clearly suboptimal, it's not very expensive since it only
happens once every 45 seconds.

[1] This caused a bug. If the padding is set too large, we'd
schedule modacks to occur in the past, creating a modack storm.

I believe the benefits of reduced complexity outweighs the cost.
Load test shows the current implementation still has no trouble
catching up with the publisher.

pongad requested a review from garrettjonesgoogle as a code owner

November 2, 2017 04:51

googlebot added the cla: yes label

pongad requested a review from jba

November 2, 2017 04:51

pongad commented

View reviewed changes

google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/MessageDispatcher2.java

+                      || builder.getModifyDeadlineAckIdsCount() == MAX_CHANGE_PER_REQUEST;
+                }
+                void processReceivedMessages(List<ReceivedMessage> messages, Runnable doneCallback) {

This comment was marked as spam.

Sign in to view

google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/MessageDispatcher2.java

+                  workMessages();
+                }
+                private synchronized void workMessages() {

This comment was marked as spam.

Sign in to view

pongad added 3 commits

November 2, 2017 18:40


          pubsub: create experimental message dispatcher

f246124

We emphasize simplicity, not optimal performance.
Of course, performance is still important,
hence the many concurrent data structures used here.

Major differences from the original and some justifications:
- It modacks immediately after receiving messages
  to take advantage of server-side duplicate-mitigation
  feature that should soon be available.
- In the original impl, we create many dispatchers.
  They share some data structures and keep others separate.
  I find this too confusing.
  The new impl creates only one dispatcher, to be shared
  among all connections. This obviously increases contention
  but should be at least partially alleviated by some lock-free
  algorithms used here.
- It makes deadline a constant of 1 minute.
  With the dup-mitigation feature, the server might need to
  wait on the order of minutes before it could redeliver messages.
  I opine that setting the deadline to 1 minute shouldn't drastically
  worsen the redelivery latency.
  Also unlike the original, it does not periodically adjust deadline.
  I have some ideas on how this could be simply implemented;
  we can add this feature back if necessary.
- Modack time is also set to 1 minute and doesn't exponentially back
  off. Since the deadline is already 1 minute, it seems silly to
  bicker over a few extra seconds. [1]
- Modacks run on fixed schedule, giving 15 seconds padding,
  and modacks all pending messages, not just the ones about to expire.
  While clearly suboptimal, it's not very expensive since it only
  happens once every 45 seconds.

[1] This caused a bug. If the padding is set too large, we'd
schedule modacks to occur in the past, creating a modack storm.

I believe the benefits of reduced complexity outweighs the cost.
Load test shows the current implementation still has no trouble
catching up with the publisher.


          make codacy happy

a9c1812


          batch modack better

06fecbb

pongad commented

View reviewed changes

google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/MessageDispatcher2.java

+                  }
+                  for (ReceivedMessage message : messages) {
+                    modAcks.add(ModAckItem.create(message.getAckId(), DEADLINE_EXTENSION_SEC));

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

mdietz94 reviewed

View reviewed changes

google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/MessageDispatcher2.java

+                    builder.addModifyDeadlineSeconds(modAck.seconds());
+                  }
+                  return builder.getAckIdsCount() == MAX_CHANGE_PER_REQUEST

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

mdietz94 reviewed

View reviewed changes

google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/MessageDispatcher2.java

+                private void sendRequest(StreamingPullRequest request) {
+                  Connection connection = null;
+                  try {
+                    connection = connections.take();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

mdietz94 commented Nov 3, 2017 via email

It's unfortunately much more expensive server side to process messages from a different stream. We do support it so users can continue to process messages after a stream dies, but we won't be able to scale if every stream is doing this.

On Nov 2, 2017 9:36 PM, "Michael Darakananda" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/ v1/MessageDispatcher2.java <#2572 (comment)> : > + @OverRide + public void run() { + try { + receiver.receiveMessage(item.message.getMessage(), consumer); + } catch (Exception e) { + consumer.throwException(e); + } + } + }); + } + } + + private void sendRequest(StreamingPullRequest request) { + Connection connection = null; + try { + connection = connections.take(); I thought I was being smart round-robin-ing the connections :( Could you provide some context here? The current logic seems to work fine on both desktop and loadtest. Might it not work in the future? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2572 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvXakDzdWgDcEiKQLPS_-_XbJD2sSxNks5sym4MgaJpZM4QPKmh> .

Contributor Author

pongad commented Nov 6, 2017

superseded by #2580

pongad closed this

pongad deleted the pubsub-dup branch

May 11, 2018 21:20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels