Conversation
TestServiceRetryState skipped the expiration check whenever the deadline's nanos field was zero. The guard appears to have been meant to detect an unset expiration, but the constructor already replaces an unset expiration with Timestamps.MAX_VALUE, which never compares as expired. As a result, the guard only suppressed real deadlines that happened to fall exactly on a second boundary, so retries were scheduled past their expiration instead of returning RETRY_STATE_TIMEOUT. Drop the nanos guard and rely on the timestamp comparison alone. Add unit tests covering whole-second, subsecond, unset, and not-yet-reached expirations.
|
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TestServiceRetryState skipped the expiration check whenever the deadline's nanos field was zero. The guard appears to have been meant to detect an unset expiration, but the constructor already replaces an unset expiration with Timestamps.MAX_VALUE, which never compares as expired. As a result, the guard only suppressed real deadlines that happened to fall exactly on a second boundary, so retries were scheduled past their expiration instead of returning RETRY_STATE_TIMEOUT.
Drop the nanos guard and rely on the timestamp comparison alone. Add unit tests covering whole-second, sub-second, unset, and not-yet-reached expirations.
What was changed
Removed the
expirationTime.getNanos() != 0guard from the expiration check inTestServiceRetryState.getBackoffIntervalInSeconds, and addedTestServiceRetryStateTest.Why?
The guard only looked at the subsecond field of the protobuf
Timestamp. An unset expiration is already turned intoTimestamps.MAX_VALUEin the constructor, and that never compares as expired. So the guard never protected the "no deadline" case. Its only effect was to skip the timeout when a real deadline fell exactly on a whole second. In that case the test server kept scheduling retries past the deadline instead of returningRETRY_STATE_TIMEOUT.Checklist
How was this tested:
The new test
expirationOnWholeSecondTimesOutfails without the fix(expected:<RETRY_STATE_TIMEOUT> but was:<RETRY_STATE_IN_PROGRESS>) and passes with it. The full:temporal-test-server:testsuite passes.