Conversation
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
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.
This looks great, thanks for doing this! Added some feedback.
|
||
def test_update_cluster(capsys): | ||
# Wrapper function for client library function | ||
create_cluster.create_cluster(PROJECT_ID, REGION, CLUSTER_NAME) |
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.
I would recommend implementing the logic for creating the cluster as a part of the setup, similar to here.
At least for Python, I typically tried not to make samples co-dependent on each other.
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.
this is done, just waiting on unblocking get_cluster
to commit
client = dataproc.ClusterControllerClient( | ||
client_options={"api_endpoint": f"{region}-dataproc.googleapis.com:443"} | ||
) | ||
# Get cluster you wish to update. |
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.
This may be a stylistic thing but I would recommend a line break before each new comment.
cluster = client.get_cluster( | ||
project_id=project_id, region=region, cluster_name=cluster_name | ||
) | ||
# Update number of clusters |
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.
Line break, see above.
) | ||
# Update number of clusters | ||
mask = {"paths": {"config.worker_config.num_instances": str(new_num_instances)}} | ||
# Update cluster config |
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.
Line break, see above.
mask = {"paths": {"config.worker_config.num_instances": str(new_num_instances)}} | ||
# Update cluster config | ||
cluster.config.worker_config.num_instances = new_num_instances | ||
# Update cluster |
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.
Line break, see above.
updated_cluster = operation.result() | ||
print(f"Cluster was updated successfully: {updated_cluster.cluster_name}") | ||
|
||
|
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.
Think you're missing the END region tag here.
@@ -0,0 +1,56 @@ | |||
# Copyright 2019 Google LLC |
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.
Change this to 2021.
update_cluster.update_cluster(PROJECT_ID, REGION, CLUSTER_NAME, NEW_NUM_INSTANCES) | ||
|
||
out, _ = capsys.readouterr() | ||
assert CLUSTER_NAME in out |
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 the test, I would instead suggest a get_cluster call and confirm that the number of instances matches NEW_NUM_INSTANCES
.
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.
I've been calling dataproc.ClusterControllerClient.get_cluster(PROJECT_ID).config
to try and get the worker_config, but it doesn't let me get any deeper into the cluster object than config
. I'm not sure how to get in there currently.
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.
Hmmm, the following ended up working for me when pointed to a cluster that I have with two workers. Did you also provide the region / cluster_name to get_cluster
?
from google.cloud import dataproc
PROJECT_ID = "my-project"
REGION = "my-region"
CLUSTER_NAME = "my-cluster"
client = dataproc.ClusterControllerClient(
client_options={
"api_endpoint": "{}-dataproc.googleapis.com:443".format(REGION)
}
)
cluster = client.get_cluster(project_id=PROJECT_ID, region=REGION, cluster_name=CLUSTER_NAME)
print(cluster.config.worker_config.num_instances) # should print an integer
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.
@bradmiro I haven't been able to get it working in the context of a test. My recent commit has three options I've tried: creating a fixture, creating a static cluster, and creating the cluster in the test. I still can't get in deeper than config
. (This may be a Pythonic learning curve thing.)
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.
Actually does seem to be working in the current commit in CI!
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.
final commit ready for review!
@@ -0,0 +1,67 @@ | |||
#!/usr/bin/env python | |||
# Licensed under the Apache License, Version 2.0 (the "License"); |
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.
Double check the license is correct here. You can take it from here and replace the year with 2021.
@@ -0,0 +1,56 @@ | |||
# Copyright 2019 Google LLC |
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.
# Copyright 2019 Google LLC | |
# Copyright 2021 Google LLC |
Co-authored-by: Bu Sun Kim <8822365+busunkim96@users.noreply.github.com>
Co-authored-by: Bu Sun Kim <8822365+busunkim96@users.noreply.github.com>
@busunkim96 @bradmiro |
Added a comment! |
…dataproc into update_cluster_sample cleaning up branch
requested changes made! |
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.
Awesome work! I left one comment and it looks like there's an outstanding lint error, but otherwise LGTM. Thanks for doing this!
|
||
out, _ = capsys.readouterr() | ||
assert CLUSTER_NAME in out | ||
assert new_num_cluster.config.worker_config.num_instances == 5 |
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.
Minor nit: I would replace 5
with the NEW_NUM_INSTANCES
variable
…dataproc into update_cluster_sample updating local branch
ready for review/approval! |
@busunkim96 ready for final review/approval! :) |
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.
Sorry for the delay! I left one comment regarding api_endpoint
, otherwise everything looks good. :)
client_transport = ( | ||
cluster_controller_grpc_transport.ClusterControllerGrpcTransport( | ||
address="{}-dataproc.googleapis.com:443".format(region) | ||
) | ||
) | ||
dataproc_cluster_client = dataproc_v1.ClusterControllerClient(client_transport) |
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.
No need to block this PR on this, but there's an easier way to set regional endpoints now through google.api_core.client_options
. See this speech sample: https://cloud.google.com/speech-to-text/docs/endpoints
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #111 🦕