docs: update cluster sample #218

loferris · 2021-07-02T18:38:50Z

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:

[x ] Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
[x ] Code coverage does not decrease (if any source code was changed)
[] Appropriate docs were updated: New doc needs to be generated

Fixes #111 🦕

snippet-bot · 2021-07-02T18:38:54Z

Here is the summary of changes.

You are about to add 1 region tag.

samples/snippets/update_cluster.py:23, tag dataproc_update_cluster

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

See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md

bradmiro

This looks great, thanks for doing this! Added some feedback.

bradmiro · 2021-07-07T20:51:46Z

samples/snippets/update_cluster_test.py

+
+def test_update_cluster(capsys):
+    # Wrapper function for client library function
+    create_cluster.create_cluster(PROJECT_ID, REGION, CLUSTER_NAME)


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.

this is done, just waiting on unblocking get_cluster to commit

bradmiro · 2021-07-07T20:52:29Z

samples/snippets/update_cluster.py

+    client = dataproc.ClusterControllerClient(
+        client_options={"api_endpoint": f"{region}-dataproc.googleapis.com:443"}
+    )
+    # Get cluster you wish to update.


This may be a stylistic thing but I would recommend a line break before each new comment.

bradmiro · 2021-07-07T20:52:41Z

samples/snippets/update_cluster.py

+    cluster = client.get_cluster(
+        project_id=project_id, region=region, cluster_name=cluster_name
+    )
+    # Update number of clusters


Line break, see above.

bradmiro · 2021-07-07T20:52:48Z

samples/snippets/update_cluster.py

+    )
+    # Update number of clusters
+    mask = {"paths": {"config.worker_config.num_instances": str(new_num_instances)}}
+    # Update cluster config


Line break, see above.

bradmiro · 2021-07-07T20:52:57Z

samples/snippets/update_cluster.py

+    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


Line break, see above.

bradmiro · 2021-07-07T20:53:28Z

samples/snippets/update_cluster.py

+    updated_cluster = operation.result()
+    print(f"Cluster was updated successfully: {updated_cluster.cluster_name}")
+
+


Think you're missing the END region tag here.

bradmiro · 2021-07-07T20:53:39Z

samples/snippets/update_cluster_test.py

@@ -0,0 +1,56 @@
+# Copyright 2019 Google LLC


Change this to 2021.

bradmiro · 2021-07-07T20:58:20Z

samples/snippets/update_cluster_test.py

+    update_cluster.update_cluster(PROJECT_ID, REGION, CLUSTER_NAME, NEW_NUM_INSTANCES)
+
+    out, _ = capsys.readouterr()
+    assert CLUSTER_NAME in out


For the test, I would instead suggest a get_cluster call and confirm that the number of instances matches NEW_NUM_INSTANCES.

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.

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

@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.)

Actually does seem to be working in the current commit in CI!

final commit ready for review!

bradmiro · 2021-07-07T20:58:45Z

samples/snippets/update_cluster.py

@@ -0,0 +1,67 @@
+#!/usr/bin/env python
+# Licensed under the Apache License, Version 2.0 (the "License");


Double check the license is correct here. You can take it from here and replace the year with 2021.

samples/snippets/submit_job.py

busunkim96 · 2021-07-02T18:48:46Z

samples/snippets/update_cluster_test.py

@@ -0,0 +1,56 @@
+# Copyright 2019 Google LLC


Suggested change

# Copyright 2019 Google LLC

# Copyright 2021 Google LLC

samples/snippets/submit_job.py

samples/snippets/update_cluster.py

Co-authored-by: Bu Sun Kim <8822365+busunkim96@users.noreply.github.com>

loferris · 2021-07-23T23:27:23Z

@busunkim96 @bradmiro
So sorry for the delay - I didn't realize I wasn't getting my GH notifications! I've committed some of the formatting changes and have a commit ready to go with the rest of the changes as soon as I can get clarification on the get_cluster method from Brad!

bradmiro · 2021-07-26T15:45:54Z

@busunkim96 @bradmiro
So sorry for the delay - I didn't realize I wasn't getting my GH notifications! I've committed some of the formatting changes and have a commit ready to go with the rest of the changes as soon as I can get clarification on the get_cluster method from Brad!

Added a comment!

See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md

…dataproc into update_cluster_sample cleaning up branch

See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md

loferris · 2021-07-29T21:52:39Z

requested changes made!

bradmiro

Awesome work! I left one comment and it looks like there's an outstanding lint error, but otherwise LGTM. Thanks for doing this!

bradmiro · 2021-07-29T22:34:04Z

samples/snippets/update_cluster_test.py

+
+    out, _ = capsys.readouterr()
+    assert CLUSTER_NAME in out
+    assert new_num_cluster.config.worker_config.num_instances == 5


Minor nit: I would replace 5 with the NEW_NUM_INSTANCES variable

…dataproc into update_cluster_sample updating local branch

loferris · 2021-08-03T02:51:41Z

ready for review/approval!

loferris · 2021-08-03T16:23:32Z

@busunkim96 ready for final review/approval! :)

busunkim96

Sorry for the delay! I left one comment regarding api_endpoint, otherwise everything looks good. :)

busunkim96 · 2021-08-05T22:25:59Z

samples/snippets/list_clusters.py

+        client_transport = (
+            cluster_controller_grpc_transport.ClusterControllerGrpcTransport(
+                address="{}-dataproc.googleapis.com:443".format(region)
+            )
        )
        dataproc_cluster_client = dataproc_v1.ClusterControllerClient(client_transport)


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

Lo Ferris added 4 commits July 1, 2021 17:18

first steps in adding sample

11cf08a

consistent formatting with create_cluster.py

205a4d0

test first draft

7b62aa7

update_cluster sample complete

dac9663

loferris requested a review from a team as a code owner July 2, 2021 18:38

loferris requested review from busunkim96 and removed request for a team July 2, 2021 18:38

product-auto-label bot added api: dataproc Issues related to the googleapis/python-dataproc API. samples Issues that are directly related to samples. labels Jul 2, 2021

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 2, 2021

🦉 Updates from OwlBot

632fd09

See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md

bradmiro suggested changes Jul 7, 2021

View reviewed changes

busunkim96 reviewed Jul 8, 2021

View reviewed changes

Lo Ferris and others added 4 commits July 23, 2021 15:43

docs: add update cluster sample - fixing formatting

5be2ed9

Update samples/snippets/update_cluster.py

cd25bf8

Co-authored-by: Bu Sun Kim <8822365+busunkim96@users.noreply.github.com>

Update samples/snippets/update_cluster.py

9a80e3d

Co-authored-by: Bu Sun Kim <8822365+busunkim96@users.noreply.github.com>

updated test, still fine-tuning

b48cbb3

parthea changed the title ~~Update cluster sample~~ docs: update cluster sample Jul 26, 2021

Lo Ferris and others added 6 commits July 28, 2021 17:27

added get_cluster to test

853cf21

🦉 Updates from OwlBot

0bba122

See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md

another attempt at writing test

9ac4e88

Merge branch 'update_cluster_sample' of github.com:googleapis/python-…

2412e9c

…dataproc into update_cluster_sample cleaning up branch

new test pattern

47b7639

🦉 Updates from OwlBot

aa93d29

See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md

bradmiro approved these changes Jul 29, 2021

View reviewed changes

updated static for new_num_instances and fixed linting error

eaacde5

Lo Ferris and others added 2 commits August 2, 2021 14:03

Merge branch 'update_cluster_sample' of github.com:googleapis/python-…

8c51ca8

…dataproc into update_cluster_sample updating local branch

Merge branch 'master' into update_cluster_sample

dfb5f19

bradmiro approved these changes Aug 3, 2021

View reviewed changes

busunkim96 approved these changes Aug 5, 2021

View reviewed changes

Merge branch 'master' into update_cluster_sample

6e0c6db

loferris merged commit 80706f9 into master Aug 12, 2021

loferris deleted the update_cluster_sample branch August 12, 2021 21:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: update cluster sample #218

docs: update cluster sample #218

loferris commented Jul 2, 2021

snippet-bot bot commented Jul 2, 2021 •

edited

bradmiro left a comment

bradmiro Jul 7, 2021

loferris Jul 23, 2021

bradmiro Jul 7, 2021

bradmiro Jul 7, 2021

bradmiro Jul 7, 2021

bradmiro Jul 7, 2021

bradmiro Jul 7, 2021

bradmiro Jul 7, 2021

bradmiro Jul 7, 2021

loferris Jul 23, 2021

bradmiro Jul 26, 2021

loferris Jul 29, 2021

loferris Jul 29, 2021

loferris Jul 29, 2021

bradmiro Jul 7, 2021

busunkim96 Jul 2, 2021

loferris commented Jul 23, 2021

bradmiro commented Jul 26, 2021

loferris commented Jul 29, 2021

bradmiro left a comment

bradmiro Jul 29, 2021

loferris commented Aug 3, 2021

loferris commented Aug 3, 2021

busunkim96 left a comment

busunkim96 Aug 5, 2021

		updated_cluster = operation.result()
		print(f"Cluster was updated successfully: {updated_cluster.cluster_name}")

		@@ -0,0 +1,67 @@
		#!/usr/bin/env python
		# Licensed under the Apache License, Version 2.0 (the "License");

docs: update cluster sample #218

docs: update cluster sample #218

Conversation

loferris commented Jul 2, 2021

snippet-bot bot commented Jul 2, 2021 • edited

bradmiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loferris commented Jul 23, 2021

bradmiro commented Jul 26, 2021

loferris commented Jul 29, 2021

bradmiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loferris commented Aug 3, 2021

loferris commented Aug 3, 2021

busunkim96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snippet-bot bot commented Jul 2, 2021 •

edited