chore(bigtable): improve system test instance usage - #18149
chore(bigtable): improve system test instance usage#18149daniel-sanche wants to merge 6 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 optimizes and improves the robustness of system tests by configuring the emulator environment, skipping stale instance cleanup when using the emulator, limiting the number of stale instance deletions to conserve API write quotas, and reusing existing instances in several test cases. The review feedback points out that catching generic exceptions during instance deletion could lead to noisy logs in CI/CD if an instance is already deleted, and suggests explicitly catching and ignoring NotFound exceptions.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| cluster_id=alt_cluster_id, | ||
| location_id=location_id, | ||
| serve_nodes=1, | ||
| # Testing `Backup.restore()`: |
There was a problem hiding this comment.
did this change the behavior? it was testing restoring to a different instance before. Now it's just backing up to a different table? on the same instance?
There was a problem hiding this comment.
Yes, this is a slightly different test case now.
Looking at the code, the client isn't really aware of whether the passed in is is new or old when building a request. So we're not really exercising any different client-side logic here, and it didn't feel worth consuming an extra Instance create/delete for this.
But if you think this is important to cover, we can leave it as-is
| client.delete_instance(name=instance.name) | ||
| except NotFound: | ||
| pass | ||
| try: |
There was a problem hiding this comment.
Are instances cleaned up properly in the tests? Over time, would this cause instances to slowly increase on the test project? and eventually we could run into SSD node or HDD node quota and will need to clean up the test project manually
There was a problem hiding this comment.
Instances are expected to clean themselves up in a post-test step when tests are finished. But there previously were (and maybe still are, and maybe will be in the future) resource leaks in certain cases, causing instances to build up slowly in the test project over time over time.
This function is trying to identify > day old test instances, and clear them, as an extra defense against this build-up. But ideally, it shouldn't be needed
daniel-sanche
added
the
do not merge
|
adding do not merge until next bulk release, to prevent re-triggering job |
daniel-sanche
removed
the
do not merge
2 participants
We have been hitting quota limits on bigtable system tests recently. This PR attempts to make a few improovements:
GOOGLE_CLOUD_PROJECTenvvar in system_emulated mode, to prevent talking to the live GCP environment