test(bigquerystorage): create sessions using await() to address flakiness#13489
test(bigquerystorage): create sessions using await() to address flakiness#13489whowes wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds the Awaitility library as a test dependency and updates the integration tests in both v1beta1 and v1beta2 versions of ITBigQueryStorageTest to poll for read session creation while ignoring NotFoundException. The review feedback suggests using the fully qualified java.time.Duration class name in the await() blocks to prevent potential compilation conflicts with other commonly imported Duration classes.
| ReadSession session = | ||
| await() | ||
| .atMost(Duration.ofSeconds(30)) | ||
| .pollInterval(Duration.ofSeconds(1)) | ||
| .ignoreException(NotFoundException.class) | ||
| .until(() -> client.createReadSession(request), Objects::nonNull); |
There was a problem hiding this comment.
To prevent potential compilation errors due to class name conflicts (e.g., with com.google.protobuf.Duration or org.threeten.bp.Duration which are commonly imported in client library tests), it is safer to use the fully qualified java.time.Duration class name.
| ReadSession session = | |
| await() | |
| .atMost(Duration.ofSeconds(30)) | |
| .pollInterval(Duration.ofSeconds(1)) | |
| .ignoreException(NotFoundException.class) | |
| .until(() -> client.createReadSession(request), Objects::nonNull); | |
| ReadSession session = | |
| await() | |
| .atMost(java.time.Duration.ofSeconds(30)) | |
| .pollInterval(java.time.Duration.ofSeconds(1)) | |
| .ignoreException(NotFoundException.class) | |
| .until(() -> client.createReadSession(request), Objects::nonNull); |
There was a problem hiding this comment.
I don't think this finding makes sense - the test class is only using one Duration class and it's imported, so it seems less readable to fully qualify it here.
| ReadSession session = | ||
| await() | ||
| .atMost(Duration.ofSeconds(30)) | ||
| .pollInterval(Duration.ofSeconds(1)) | ||
| .ignoreException(NotFoundException.class) | ||
| .until(() -> client.createReadSession(request), Objects::nonNull); |
There was a problem hiding this comment.
To prevent potential compilation errors due to class name conflicts (e.g., with com.google.protobuf.Duration or org.threeten.bp.Duration which are commonly imported in client library tests), it is safer to use the fully qualified java.time.Duration class name.
| ReadSession session = | |
| await() | |
| .atMost(Duration.ofSeconds(30)) | |
| .pollInterval(Duration.ofSeconds(1)) | |
| .ignoreException(NotFoundException.class) | |
| .until(() -> client.createReadSession(request), Objects::nonNull); | |
| ReadSession session = | |
| await() | |
| .atMost(java.time.Duration.ofSeconds(30)) | |
| .pollInterval(java.time.Duration.ofSeconds(1)) | |
| .ignoreException(NotFoundException.class) | |
| .until(() -> client.createReadSession(request), Objects::nonNull); |
There was a problem hiding this comment.
I don't believe so - same reasoning as the v1beta1 test
5125249 to
3a53327
Compare
30cc753 to
5f3c0f0
Compare
whowes
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
whowes
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
5f3c0f0 to
5860a04
Compare
| .pollInterval(Duration.ofSeconds(1)) | ||
| // retry if the newly-created table has not yet fully propagated | ||
| .ignoreException(NotFoundException.class) | ||
| .untilAsserted(() -> sessionRef.set(client.createReadSession(request))); |
There was a problem hiding this comment.
Would it make sense to replace this with the following?
.until(
() -> {
sessionRef.set(client.createReadSession(request));
return true;
});
| .pollInterval(Duration.ofSeconds(1)) | ||
| // retry if the newly-created table has not yet fully propagated | ||
| .ignoreException(NotFoundException.class) | ||
| .untilAsserted(() -> sessionRef.set(client.createReadSession(request))); |
There was a problem hiding this comment.
Would it make sense to replace this with the following?
.until(
() -> {
sessionRef.set(client.createReadSession(request));
return true;
});
| final CreateReadSessionRequest request = createSessionRequestBuilder.build(); | ||
| final AtomicReference<ReadSession> sessionRef = new AtomicReference<>(); |
There was a problem hiding this comment.
Are the final modifiers necessary?
There was a problem hiding this comment.
Appears that they are not - removed.
| final CreateReadSessionRequest request = createSessionRequestBuilder.build(); | ||
| final AtomicReference<ReadSession> sessionRef = new AtomicReference<>(); |
There was a problem hiding this comment.
Are the final modifiers necessary?
There was a problem hiding this comment.
Appears that they are not - removed.
5860a04 to
065689e
Compare
065689e to
b318fdf
Compare
3 participants
Recurring test flakes (#11964, #12981) have resulted when metadata for tables created via bigquery.googleapis.com are not immediately available for reading via bigquerystorage.googleapis.com. This PR introduces Awaitility to bigquerystorage and uses it in the flaky ITBigQueryStorageTest classes to retry creating ReadSessions when they fail with NotFoundExceptions.
Fixes #11964