[SPARK-23747][Structured Streaming] Add EpochCoordinator unit tests #20983
Conversation
|
@jose-torres this PR is ready for a review |
| makeSynchronousCall() | ||
|
|
||
| verifyCommit(startEpoch) | ||
| } |
There was a problem hiding this comment.
I'd suggest adding test cases where all but one writer partition has committed, or all but one reader partition has reported an offset. In those cases we should verify that the StreamWriter and query commits haven't happened.
| setWriterPartitions(2) | ||
| setReaderPartitions(2) | ||
|
|
||
| val epochs = startEpoch to (startEpoch + 1) |
There was a problem hiding this comment.
This is a personal preference thing, but I think it might be more readable to just use the raw numbers. Something like:
commitPartitionEpoch(0, 1)
commitPartitionEpoch(1, 1)
[...]
commitPartitionEpoch(0, 2)
commitPartitionEpoch(1, 2)
[...]
verifyCommitsInOrderOf(1, 2)
There was a problem hiding this comment.
I agree that it would be more readable, however the fact that we test for the start epoch first might be not as obvious then since it'd be hardcoded in before. Still pretty obvious though I guess.. and probably there will be no need to change start epoch in tests so hardcoding it is fine, and readability would increase. Will change this
|
@jose-torres ready for a review |
|
|
||
| makeSynchronousCall() | ||
|
|
||
| verifyCommitHasntHappened(1) |
There was a problem hiding this comment.
nit: maybe verifyNoCommitFor
There was a problem hiding this comment.
+1
verifyNoCommitFor is less verbose. will merge as soon as this is changed.
|
jenkins ok to test |
|
jenkins test this please |
|
Test build #89297 has finished for PR 20983 at commit
|
|
Test build #89324 has finished for PR 20983 at commit
|
|
@tdas @jose-torres should we merge this now that it passes tests and test SPARK-23503 changes? |
|
Yeah. Merging this to master. |
|
@jose-torres Should I cherry-pick it to 2.3 as well?? |
What changes were proposed in this pull request?
Unit tests for EpochCoordinator that test correct sequencing of committed epochs. Several tests are ignored since they test functionality implemented in SPARK-23503 which is not yet merged, otherwise they fail.