fix: Remove wait after SQLInstance periodic test steps #3189
Conversation
|
/hold need to test this manually |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This is should longer be necessary after waiting for LRO completion: GoogleCloudPlatform@d9baf0b
jasonvigil
force-pushed
the
remove-sql-timeouts
branch
from
19ce34d to
f35da81
Compare
|
@yuwenma could you pls re-review this? Turns out we no longer need |
| @@ -503,7 +503,7 @@ func testDriftCorrection(ctx context.Context, t *testing.T, testContext testrunn | |||
|
|
|||
| // Underlying APIs may not have strongly-consistent reads due to caching. Sleep before attempting a re-reconcile, to | |||
| // give the underlying system some time to propagate the deletion info. | |||
| time.Sleep(resourceContext.RecreateDelay) | |||
| time.Sleep(time.Second * 10) | |||
There was a problem hiding this comment.
Is this a revert to a previous const var? I like that we make this a configurable field. It is more manageable and can help future resources which needs additional delays.
There was a problem hiding this comment.
There are no known circumstances where we need to add additional delays. Since there are no more uses, I think we should remove the configurability, because it is now dead code.
There was a problem hiding this comment.
"dead code should be cleaned up" --> Right, that makes the KCC code base much better.
I consider this not a dead code but a feature that has been approved working well. For dynamic test, we do not expect the contributors can be able to fix the source code when their resource need some special treatment, the only option for them is to explore the configurables in the resource-context file. Enriching the configurable pools is always appreciated, especially if the feature does not introduce performance regression to the current test. Therefore, can we keep it?
There was a problem hiding this comment.
Aside from this code no longer being used, I also feel like this code is not really a desirable feature. It allowed me to miss an actual bug, which was that my direct controller was not waiting correctly for delete LRO's to complete. From this one example of a previous use, this configuration seems like more like a "foot-gun", rather than a desirable feature. If any contributors hit issues in the future where they feel like something like this is actually necessary, I think not having this configuration available would cause us to examine the situation more closely, rather than allow a "quick workaround" of extending the wait time and potentially miss a similar bug.
There was a problem hiding this comment.
Ok. If it is not approved to be a desired feature, agree that we should drop it .
|
/lgtm We want to keep the custom delay feature in dynamic test. Non blocker to this PR. You can always put it back in the next code change. |
|
/hold cancel |
google-oss-prow
bot
merged commit 2e24c2e
into
GoogleCloudPlatform:master
This is should longer be necessary after waiting for LRO completion: d9baf0b