Skip to content

Conversation

@tseaver
Copy link
Contributor

@tseaver tseaver commented Jul 12, 2021

Closes #353.

@kolea2 Note that this PR injects "repeatability" noise into the sample, which may be undesirable from the perspective of documentation.

@tseaver tseaver requested a review from kolea2 July 12, 2021 19:32
@tseaver tseaver requested a review from a team as a code owner July 12, 2021 19:32
@product-auto-label product-auto-label bot added api: bigtable Issues related to the googleapis/python-bigtable API. samples Issues that are directly related to samples. labels Jul 12, 2021
print("\nCluster not created, as {} already exists.".format(cluster_id))
else:
operation = cluster.create()
# Avoid failing for "instance is currently being changed" by
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this belong here or wrapping the add_cluster call in test_instanceadmin.py? https://github.com/googleapis/python-bigtable/blob/master/samples/instanceadmin/test_instanceadmin.py#L118-L122. Might be a better fit there, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapping inside the testrunner would certainly leave the sample itself more pristine: the issue here is that the back-end is raising a 503 UNAVAILABLE for the cluster.create() call, which is (presumably) occurring "too soon" after the instance.create call. Re-running the whole sample doesn't really address the root cause of the flakiness. It might happen to delay running the sample until the back-end load dropped (or whatever is causing the current error to be raised on the back-end).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instance.create occurs outside of this sample. This seems like a synchronization problem of how we run our tests, and therefore are a bit confusing to put inside the sample itself. To me, it makes more sense to handle issues with our testing in the test class instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kolea2 b0267cd re-does the PR with the backoff wrapper in the test driver, rather than the sample. PTAL

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, thank you! LGTM

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 13, 2021
@tseaver tseaver force-pushed the 353-use-backoff-for-flaky-sample branch from 9370f85 to b0267cd Compare July 13, 2021 19:15
@tseaver tseaver requested a review from kolea2 July 13, 2021 19:16
@kolea2 kolea2 changed the title doc(samples): add backoff to cluster creation sample test(samples): add backoff to cluster creation sample Jul 13, 2021
@kolea2
Copy link
Collaborator

kolea2 commented Jul 13, 2021

@googlebot

@tseaver tseaver merged commit 38902c4 into master Jul 13, 2021
@tseaver tseaver deleted the 353-use-backoff-for-flaky-sample branch July 13, 2021 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigtable Issues related to the googleapis/python-bigtable API. cla: yes This human has signed the Contributor License Agreement. samples Issues that are directly related to samples.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

samples.instanceadmin.test_instanceadmin: test_add_and_delete_cluster failed

3 participants