pubsub: send message receipts #2580
Conversation
googlebot
added
the
cla: yes
| } | ||
|
|
||
| // @Test |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| pendingReceipts.drainTo(receiptsToSend.ackIds); | ||
| logger.log(Level.FINER, "Sending {0} receipts", receiptsToSend.ackIds.size()); | ||
| if (!receiptsToSend.ackIds.isEmpty()) { | ||
| modifyAckDeadlinesToSend.add(receiptsToSend); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@mdietz94 please take a look |
| synchronized (pendingAcks) { | ||
| pendingAcks.add(ackId); | ||
| } | ||
| destination = pendingAcks; | ||
| // Record the latency rounded to the next closest integer. | ||
| ackLatencyDistribution.record( |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| public void run() { | ||
| try { | ||
| processOutstandingAckOperations(); | ||
| } catch (Exception e) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This commit also simplifies acks/nacks logic.
It is accessed from executor threads and must to synced to guarantee property updates are observed.
If the user code throws an exception, we catch this exception and nack the message. This logic cannot be tested correctly. The test uses a MessageReceiver that can notify us that messages have been processed, so that we can "advanceTime". The receiver can only notify us that it's throwing an exception BEFORE the exception is actually thrown out of the method. (Even with finally-clause, the statements in the clause runs before the giving control back to the caller.) Consequently, we're notifying too soon: the message has not been processed yet as the exception might not have been caught.
|
@mdietz94 Thank you for the review, PTAL.
I plan to soon make extension job run on schedule, kind of like what's ack/nack/receipt is doing in this PR. I found a pretty simple way to make it work with periodic stream deadline modification. However, I don't think I can make it work with immediate modack scheduling that receipt would need. If this PR looks "right but sub-optimal" to you, do you think we can submit this and follow up in another PR? I think this will give us better throughput; timezone makes code review latency rather high. I had to add a few changes to address some flakes. Looks like there's still one flakey test. I after analyzing the logs, I believe the problem is in the test itself, not the library, though I can't pinpoint where. I think the best course of action is to finish simplifying the library, then we can simplify the test to have fewer moving parts. I can take this on later as well. |
|
ack. (no pun intended) |
This commit also simplifies acks/nacks logic.