Fix assertion in SnapshotShutdownIT.testSnapshotShutdownProgressTracker()
#127998
Conversation
Relaxes a log expectation assertion from an exact test of numShards running snapshots to 1-numShards, since it is possible for some of the shard snapshot statuses to already be in stage=PAUSED. Closes elastic#127690
JeremyDahlgren
added
>test
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
| @@ -540,12 +540,13 @@ public void testSnapshotShutdownProgressTracker() throws Exception { | |||
| mockLog.awaitAllExpectationsMatched(); | |||
| resetMockLog(); | |||
|
|
|||
| assert 1 <= numShards && numShards <= 10; | |||
There was a problem hiding this comment.
| assert 1 <= numShards && numShards <= 10; | |
| // At least one shard reached the MockRepository's blocking code when waitForBlock was called. However, there's no guarantee that | |
| // the other shards got that far before the shutdown flag was put in place, in which case the other shards may be paused instead. | |
| assert 1 <= numShards && numShards <= 10; |
There was a problem hiding this comment.
I'm wondering why we are asserting this because final int numShards = randomIntBetween(1, 10); already says it's going to be between 1 and 10?
There was a problem hiding this comment.
I added the code comment. In the chance the thread bounds were changed it seemed clearer to enforce the restriction there instead of waiting for the seen event timeout. I can see it doesn't add value, the assertion is removed now.
| "SnapshotShutdownProgressTracker running number of snapshots", | ||
| SnapshotShutdownProgressTracker.class.getCanonicalName(), | ||
| Level.INFO, | ||
| "*Number shard snapshots running [" + numShards + "].*" | ||
| ".+Number shard snapshots running \\[" + (numShards < 10 ? "[1-" + numShards + "]" : "([1-9]|10)") + "].+" |
There was a problem hiding this comment.
Can we change the final int numShards = randomIntBetween(1, 10); to final int numShards = randomIntBetween(1, 9); to make this easier? The random number range is arbitrary, so I think that would be okay.
There was a problem hiding this comment.
++ (or else maybe leave this as a SeenEventExpectation and parse out the numShards by overriding innerMatch rather than trying to do anything overly clever with a regex)
Relaxes a log expectation assertion from an exact test of
numShardsrunning snapshots to1-numShards, since itis possible for some of the shard snapshot statuses to already be in
stage=PAUSED.Closes #127690