Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

samples: add default leader options samples #428

Merged
merged 20 commits into from Aug 9, 2021
Merged

samples: add default leader options samples #428

merged 20 commits into from Aug 9, 2021

Conversation

zoercai
Copy link
Contributor

@zoercai zoercai commented Jul 26, 2021 •
edited

Samples for #399. Implementation code will be gone once #399 is merged.

@zoercai zoercai requested review from a team as code owners July 26, 2021 12:40
@zoercai zoercai requested review from crwilcox and removed request for a team July 26, 2021 12:40
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/python-spanner API. label Jul 26, 2021
@snippet-bot
Copy link

snippet-bot bot commented Jul 26, 2021 •
edited

Here is the summary of changes.

You are about to add 7 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label Jul 26, 2021
@zoercai zoercai requested a review from larkee July 26, 2021 12:40
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 26, 2021
@zoercai zoercai requested a review from hengfengli July 26, 2021 12:41
larkee
larkee requested changes Jul 27, 2021
View reviewed changes
Copy link
Contributor

@larkee larkee left a comment

Choose a reason for hiding this comment

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

Currently, you are creating an instance within the spanner_create_database_with_default_leader and spanner_update_database_with_default_leader samples.

This is an issue as if there is a failure later on in the sample, the instance does not get cleaned up correctly. Additionally the second sample fails as it tried to create the same instance.

The instance creation should be removed from the samples and you should instead be using sample_instance, or a pytest fixture that uses sample_instance. This will ensure that the instance clean up is handled correctly.

larkee
larkee approved these changes Jul 28, 2021
View reviewed changes
Copy link
Contributor

@larkee larkee left a comment

Choose a reason for hiding this comment

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

LGTM once the implementation is merged and released 👍

@larkee larkee changed the title samples: add default leader options samples 3ce1ed7 samples: add default leader options samples Jul 28, 2021
@larkee larkee added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 28, 2021
@zoercai zoercai added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 28, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 28, 2021
@zoercai
Merge branch 'master' of github.com:googleapis/python-spanner into cm…
3c88ef0 
…mr-1-samples

# Conflicts:
#	tests/system/test_system.py
@zoercai zoercai added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 29, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 29, 2021
@zoercai zoercai added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 29, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 29, 2021
larkee
larkee reviewed Jul 30, 2021
View reviewed changes
Copy link
Contributor

@larkee larkee left a comment

Choose a reason for hiding this comment

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

Having just written the samples for PHP I noticed some inconsistencies. Just a few changes for the list_* samples

samples/samples/snippets.py Outdated Show resolved Hide resolved
samples/samples/snippets.py Outdated Show resolved Hide resolved
@larkee larkee removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 3, 2021
@zoercai zoercai added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 5, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 5, 2021
@hengfengli
Copy link

@zoercai Why do we get an error Instance already exist?

value = None
from_value = <_InactiveRpcError of RPC that terminated with:
	status = StatusCode.ALREADY_EXISTS
	details = "Instance already exist...message":"Instance already exists: projects/precise-truck-742/instances/google-cloud-1628122209464","grpc_status":6}"
>

>   ???
E   google.api_core.exceptions.AlreadyExists: 409 Instance already exists: projects/precise-truck-742/instances/google-cloud-1628122209464

@larkee larkee added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 9, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 9, 2021
@larkee larkee merged commit 86293f9 into googleapis:master Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larkee larkee larkee approved these changes

@crwilcox crwilcox Awaiting requested review from crwilcox

@hengfengli hengfengli Awaiting requested review from hengfengli

Assignees
No one assigned
Labels
api: spanner Issues related to the googleapis/python-spanner API. cla: yes This human has signed the Contributor License Agreement. samples Issues that are directly related to samples.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@zoercai @hengfengli @larkee @yoshi-kokoro