[AIRFLOW-2461] Add support for cluster scaling on dataproc operator #3357

piffall · 2018-05-14T08:12:46Z

Make sure you have checked all steps below.

JIRA

AIRFLOW-2461

Description

Add support for scale dataproc clusters:

Tests

Implementation of tests:

Commits

My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

Passes git diff upstream/master -u -- "*.py" | flake8 --diff

Fokko

Some small remarks. Could you also add this to the documentation? Maybe provide an example. I like the idea of the operator.

@kaxil Can you take a look?

Fokko · 2018-05-14T08:22:49Z

airflow/contrib/operators/dataproc_operator.py

Can't we poll the status instead of having a fixed sleep?

I think not. API has rate limits, and I see that all other dataproc operators are using 15s as sleep time. So I respect that previous decision.

Fokko · 2018-05-14T08:23:38Z

airflow/contrib/operators/dataproc_operator.py

d is not in the regex group, so this branch of the elif will never be invoked.

Thanks! Fixing this.

Fokko · 2018-05-14T08:23:50Z

airflow/contrib/operators/dataproc_operator.py

Maybe move this to the init?

You are wright! Fixing this! Thanks!

Fokko · 2018-05-14T08:24:18Z

tests/contrib/operators/test_dataproc_operator.py

Can we move this one to a new line as well?

Fixing this. Thanks!

kaxil · 2018-05-14T08:46:57Z

tests/contrib/operators/test_dataproc_operator.py

Nit. Can you change # Unit test for the DataprocClusterDeleteOperator to # Unit test for the DataprocClusterScaleOperatorTest

Fixing this. Thanks!

kaxil · 2018-05-14T08:49:11Z

airflow/contrib/operators/dataproc_operator.py

Can you change it to The name of the cluster to scale

Fixing this. Thanks!

kaxil · 2018-05-14T08:50:30Z

airflow/contrib/operators/dataproc_operator.py

Can you update this to The region for the dataproc cluster as now it does support regions like europe-west

Fixing this. Thanks!

kaxil · 2018-05-14T08:52:42Z

airflow/contrib/operators/dataproc_operator.py

Can we also have a try.. except block for 404 errors?

This can only happen if the operation is not found, that should never happen, so I think it should raise. Do you that that would be better to add the try..catch block and raise the exception after logging it?

I would want to log it tbh.

Error logged.

kaxil · 2018-05-14T08:53:49Z

airflow/contrib/operators/dataproc_operator.py

Is there a specific reason for selecting 15 seconds? or is it just the smallest time you think dataproc would take to scale up?

Yes, in my experience it couldn't be done in less time.

Makes sense.

kaxil · 2018-05-14T09:16:20Z

airflow/contrib/operators/dataproc_operator.py

Can you just add a link to https://cloud.google.com/dataproc/docs/concepts/configuring-clusters/scaling-clusters and say "For more information visit {LINK}".

Link added.

kaxil · 2018-05-14T09:22:39Z

Can you squash your commits?

Can you also fix the following flake8 error?

Running flake8 on the diff in the range 042c3f2a..8fa2eb3d (6 commit(s)):
--------------------------------------------------------------------------------
tests/contrib/operators/test_dataproc_operator.py:267:54: F841 local variable '_' is assigned to but never used
                with self.assertRaises(TypeError) as _:
                                                     ^
tests/contrib/operators/test_dataproc_operator.py:309:54: F841 local variable '_' is assigned to but never used
                with self.assertRaises(TypeError) as _:
                                                     ^
ERROR: InvocationError for command '/home/travis/build/apache/incubator-airflow/scripts/ci/flake8_diff.sh' (exited with code 1)

Thank you for adding this operator. This would be quite helpful.

piffall · 2018-05-14T09:58:42Z

Commits squashed and flake8 errors fixed. Waiting for test results.

codecov-io · 2018-05-14T10:09:16Z

Codecov Report

Merging #3357 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3357   +/-   ##
=======================================
  Coverage   76.41%   76.41%           
=======================================
  Files         203      203           
  Lines       14993    14993           
=======================================
  Hits        11457    11457           
  Misses       3536     3536

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9236349...e6e6bfa. Read the comment docs.

piffall · 2018-05-14T10:45:58Z

I think that is all fixed. Thank you guys!

kaxil · 2018-05-14T12:26:49Z

Can you also add documentation at https://github.com/apache/incubator-airflow/blob/master/docs/integration.rst

piffall · 2018-05-14T13:20:17Z

@kaxil Documentation added to code.rst and integration.rst files.

kaxil

LGTM

Closes #3357 from piffall/master (cherry picked from commit 9c915c1) Signed-off-by: Kaxil Naik <kaxilnaik@gmail.com>

Closes apache#3357 from piffall/master

Fokko requested changes May 14, 2018

View reviewed changes

kaxil reviewed May 14, 2018

View reviewed changes

piffall closed this May 14, 2018

piffall reopened this May 14, 2018

piffall closed this May 14, 2018

piffall reopened this May 14, 2018

[AIRFLOW-2461] Add support for cluster scaling on dataproc operator

e6e6bfa

kaxil approved these changes May 14, 2018

View reviewed changes

asfgit closed this in 9c915c1 May 14, 2018

asfgit pushed a commit that referenced this pull request May 14, 2018

[AIRFLOW-2461] Add support for cluster scaling on dataproc operator

deecbb9

Closes #3357 from piffall/master (cherry picked from commit 9c915c1) Signed-off-by: Kaxil Naik <kaxilnaik@gmail.com>

aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019

[AIRFLOW-2461] Add support for cluster scaling on dataproc operator

a288961

Closes apache#3357 from piffall/master

potiuk mentioned this pull request Dec 2, 2023

Bump minimum Airflow version in providers to Airflow 2.6.0 #36017

Merged

[AIRFLOW-2461] Add support for cluster scaling on dataproc operator #3357

[AIRFLOW-2461] Add support for cluster scaling on dataproc operator #3357

Uh oh!

Conversation

piffall commented May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JIRA

Description

Tests

Commits

Documentation

Code Quality

Uh oh!

Fokko 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!

kaxil May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kaxil commented May 14, 2018

Uh oh!

piffall commented May 14, 2018

Uh oh!

codecov-io commented May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

piffall commented May 14, 2018

Uh oh!

kaxil commented May 14, 2018

Uh oh!

piffall commented May 14, 2018

Uh oh!

kaxil left a comment

Choose a reason for hiding this comment

piffall commented May 14, 2018 •

edited

Loading

kaxil May 14, 2018 •

edited

Loading

codecov-io commented May 14, 2018 •

edited

Loading