-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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.
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
Hello example update #1496
Hello example update #1496
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
@jabubake: This needs a review from your team. I'll also do first pass to make sure that this is right. |
I signed it! |
CLAs look good, thanks! |
bigtable/hello/main.py
Outdated
parser = argparse.ArgumentParser( | ||
description=__doc__, | ||
formatter_class=argparse.ArgumentDefaultsHelpFormatter) | ||
parser.add_argument('project_id', help='Your Cloud Platform project ID.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Cloud Platform/Google Cloud Platform/
bigtable/hello/main.py
Outdated
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
import os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please insert an empty line after license
bigtable/hello/main.py
Outdated
- Create a Cloud Bigtable cluster. | ||
https://cloud.google.com/bigtable/docs/creating-cluster | ||
- Set your Google Application Default Credentials. | ||
https://developers.google.com/identity/protocols/application-default-credentials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the updated auth link: https://cloud.google.com/docs/authentication/getting-started
bigtable/hello/main.py
Outdated
|
||
def main(project_id, instance_id, table_id): | ||
# [START connecting_to_bigtable] | ||
# The client must be created with admin=True because it will create a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update comment to # Set admin=True to create an client that can perform admin tasks like creating a table.
bigtable/hello/main.py
Outdated
# [START creating_a_table] | ||
print('Creating the {} table.'.format(table_id)) | ||
table = instance.table(table_id) | ||
table.create() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle error if any on table creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain, only exception handling needs to add or we should add checks like whether a table already exists etc.?
bigtable/hello/main.py
Outdated
|
||
# [START scanning rows with filter] | ||
print('Scanning for all greetings:') | ||
rows = table.yield_rows(filter_=CellsRowLimitFilter(1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment / logging statement to what this filter means would be useful.
bigtable/hello/main.py
Outdated
formatter_class=argparse.ArgumentDefaultsHelpFormatter) | ||
parser.add_argument('project_id', help='Your Cloud Platform project ID.') | ||
parser.add_argument( | ||
'instance_id', help='ID of the Cloud Bigtable instance to connect to.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your Cloud Bigtable instance ID
bigtable/hello/main.py
Outdated
'instance_id', help='ID of the Cloud Bigtable instance to connect to.') | ||
parser.add_argument( | ||
'--table', | ||
help='Table to create and destroy.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/destroy/delete/g
bigtable/hello/main.py
Outdated
# [END deleting_a_table] | ||
|
||
|
||
if __name__ == '__main1__': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/main1/main/
bigtable/hello/main.py
Outdated
RowSampleFilter, ColumnRangeFilter, SinkFilter, RowKeyRegexFilter,\ | ||
TimestampRange, ApplyLabelFilter, ColumnQualifierRegexFilter,\ | ||
CellsRowLimitFilter, CellsColumnLimitFilter, _BoolFilter, ValueRangeFilter | ||
from datetime import datetime, timezone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ran into issue w/ timezone included on MacOS X. Samples are expected to be compatible with Python 2.7 and 3.4+. I see similar issues on Github that this may not work with 2.7 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, issues with Python 2.7.
What I observed was, we need to use datetime.datetime object and the object needs to pass is compatible with Python 3. For now, we removed timestamp thing from code as per comment from Solomon. But this needs to address this issue in Bigtable code if we want to run with Python 2.7.
bigtable/hello/main.py
Outdated
'Hello Python!', | ||
] | ||
|
||
local_timezone = datetime.now(timezone.utc).astimezone().tzinfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this. This does not seem to be used anywhere.
bigtable/hello/main.py
Outdated
row.set_cell( | ||
column_family_id, | ||
column_id, | ||
value.encode('utf-8'), timestamp=datetime.now(tz=pytz.timezone('UTC'))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, please remove the timestamp. We'll address this later.
- Fixed print statement - Added filter while using read_row function
Added exception handling block while creating table Added method to check whether table already exists Added table already exists check before creating table
# [END deleting_a_table] | ||
|
||
|
||
def check_table_already_exists(table_id, instance): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a table.exists()
method? If so, please use that instead. If not, then we need to file a feature request in https://github.com/GoogleCloudPlatform/google-cloud-python for that method.
Thank you for your submission. Unfortunately, it has been outdated by other code updates since submission, so we are closing it. I hope you continue to contribute to this or other projects. |
No description provided.