Simplify Blobstore Consistency Check in Tests #73992
Conversation
With work to make repo APIs more async incoming in elastic#73570 we need a non-blocking way to run this check. This adds that async check and removes the need to manually pass executors around as well.
original-brownbear
added
>test
elasticmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
test/framework/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreTestUtil.java
Outdated
Show resolved
Hide resolved
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
LGTM, one orthogonal comment. Also +1 to what Francisco said.
| assertSnapshotUUIDs(repository, repositoryData); | ||
| assertShardIndexGenerations(blobContainer, repositoryData.shardGenerations()); | ||
| return null; | ||
| } catch (AssertionError e) { |
There was a problem hiding this comment.
I believe we're catching the AssertionError here in order to re-throw it on the calling thread so that we can use this within assertBusy() for the third-party tests that allow for S3 to be eventually-consistent. Do we need that any more, now that S3 is properly consistent?
There was a problem hiding this comment.
Good question ... I guess you could make the same argument for other eventual consistency code (mainly EventuallyConsistentMockRepository and just drop that stuff as well. But I always figured that we may still have other S3 compatible implementations out there in the wild that are eventually consistent and so it's nice to have this stuff in tests for third party testing? (not sure ... I don't know of any and would also be happy to drop all of it to save some LoC :))
There was a problem hiding this comment.
I think we could reasonably say a third (fourth?) party implementation that has weaker consistency than the real S3 is not compatible. The repo analysis API fails on inconsistent listings:
There was a problem hiding this comment.
fair point, I'll open a follow-up that drops all these tests in a bit then :)
|
Thanks Francisco and David! |
With work to make repo APIs more async incoming in elastic#73570 we need a non-blocking way to run this check. This adds that async check and removes the need to manually pass executors around as well.
5 participants
With work to make repo APIs more async incoming in #73570
we need a non-blocking way to run this check. This adds that async
check and removes the need to manually pass executors around as well.