chore(spanner): clean up pre-existing backup schedules in samples and increase wait timeout - #14221
chore(spanner): clean up pre-existing backup schedules in samples and increase wait timeout#14221sakthivelmanii wants to merge 2 commits into
Conversation
… increase replica harness wait timeout
![gemini-code-assist[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request increases the timeout from 10 to 30 seconds in LocationAwareSharedBackendReplicaHarnessTest to prevent test flakiness. It also introduces a pre-test cleanup mechanism for backup schedules across several integration tests by adding a @before hook that invokes a new helper method cleanUpPreExistingBackupSchedules in SampleTestBaseV2. I have no feedback to provide as there are no review comments.
…SPANNER_EMULATOR_HOST
olavloite
changed the title
|
|
||
| @Before | ||
| public void cleanUpPreExistingBackupSchedules() { | ||
| cleanUpPreExistingBackupSchedules(instanceId, databaseId); |
There was a problem hiding this comment.
Aren't these integration tests configured to run concurrently? If so, then we risk that any backup test might delete all backup schedules, which again will interfere with any test that verifies that backup schedules can be created / deleted.
|
|
||
| private static int waitForReplicaRoutedRead( | ||
| DatabaseClient client, SharedBackendReplicaHarness harness) throws InterruptedException { | ||
| long deadlineNanos = System.nanoTime() + TimeUnit.SECONDS.toNanos(10); |
There was a problem hiding this comment.
Should we also update this timeout? And check if there are more places where we should consider increasing the timeout? (But see also the comment below)
| DatabaseClient client, SharedBackendReplicaHarness harness, int expectedReplicaIndex) | ||
| throws InterruptedException { | ||
| long deadlineNanos = System.nanoTime() + TimeUnit.SECONDS.toNanos(10); | ||
| long deadlineNanos = System.nanoTime() + TimeUnit.SECONDS.toNanos(30); |
There was a problem hiding this comment.
A timeout of 30 seconds (but also 10 seconds...) in something that is intended to be a unit test sounds a bit like a code smell. Why do we need such a high timeout?
Can we check for better testing strategies than this? E.g. waiting for deterministically for specific requests or events that we expect? Use latches to synchronize specific actions? etc.
2 participants
Clean up pre-existing backup schedules in samples and increase replica harness wait timeout.