Max ack extension #1898
Max ack extension #1898
Conversation
…on to (googleapis#1645)". Compression is not fully supported in gRPC, can't have it in the library yet. This reverts commit a599972.
…Consumer interface, it really is no useful to be able to set an exception as ack reply, since the result is the same as nack, if we ever requ 8000 ire another result then we can just add one more value to the AckReply enum. Also adding a fail-safe catch so if the receiver ever throws an exception we will interpret that as a nack and keep going.
This prevents the library extending messages infinitevely if the user code has lost track of the ack consumer handle.
the next extension surpasses it.
|
Changes Unknown when pulling 4883e6f on davidtorres:max-ack-ext into ** on GoogleCloudPlatform:master**. |
| return new ModifyAckDeadline(ack, 2); // 2 seconds is the initial mod ack deadline | ||
| } | ||
| }); | ||
|
|
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.
| }); | ||
|
|
||
| int timeIncrement = 4; // Second time increment | ||
|
|
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.
|
|
||
| while (fakeExecutor.getClock().millisTime() + (timeIncrement * 1000) < 1000 * 60 * 60) { | ||
| fakeExecutor.advanceTime(Duration.standardSeconds(timeIncrement)); | ||
|
|
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.
* Adding units to new tests variables * Making a constant for the first time modify ack deadline extension
|
LGTM after tests pass |
|
@davidtorres I have a concern that the new test might be flaky - could you check the Travis failures? |
manifest only certain github testing environments.
|
Yes I'm pushing a change now to address the issue, the tests should/might have been flaky before as well (or the expectations too lax), this is an old bug in the tests because of thread scheduling and the latest changes to the tests just exposed the bug. |
|
Changes Unknown when pulling 09172a6 on davidtorres:max-ack-ext into ** on GoogleCloudPlatform:master**. |
|
Changes Unknown when pulling 6a21a54 on davidtorres:max-ack-ext into ** on GoogleCloudPlatform:master**. |
|
Tests are now passing PTAL |
|
|
||
| private Deque<Duration> expectedWorkQueue = new LinkedList<>(); | ||
|
|
||
| public void setupScheduleExpectation(Duration delay) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| // Send messages to be acked | ||
| List<String> testAckIdsBatch = ImmutableList.of("A", "B", "C"); | ||
| testReceiver.setExplicitAck(true); | ||
| // A modify ack deadline should be schedule for the next 9s |
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.
| pendingCallables.add(callable); | ||
| } | ||
| work(); | ||
| synchronized (expectedWorkQueue) { |
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.
| } | ||
| work(); | ||
| synchronized (expectedWorkQueue) { | ||
| if (!expectedWorkQueue.isEmpty() && expectedWorkQueue.peek().equals(callable.delay)) { |
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.
|
Aside from @garrettjonesgoogle's comments, I'm good with this. Thank you for the PR! |
FakeScheduledExecutor that allow you to set expectations on scheduled work.
|
Changes Unknown when pulling 204438c on davidtorres:max-ack-ext into ** on GoogleCloudPlatform:master**. |
Inadvertently (and my because my lack of knowledge of github PRs) I reset a change in PR #1869 and that caused it to drop it.
@pongad @garrettjonesgoogle