docs: update cluster sample by loferris · Pull Request #218 · googleapis/python-dataproc

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

+
+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

+    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

+    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

+    )
+    # 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

+    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

+    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

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


Change this to 2021.

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

+    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

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

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

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


Suggested change

# Copyright 2019 Google LLC

# Copyright 2021 Google LLC

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

+
+    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

+                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

		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");

Uh oh!

Conversation

loferris commented Jul 2, 2021

Uh oh!

snippet-bot Bot commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradmiro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

loferris commented Jul 23, 2021

Uh oh!

bradmiro commented Jul 26, 2021

Uh oh!

loferris commented Jul 29, 2021

Uh oh!

bradmiro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loferris commented Aug 3, 2021

Uh oh!

loferris commented Aug 3, 2021

Uh oh!

busunkim96 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

snippet-bot Bot commented Jul 2, 2021 •

edited

Loading