Add support for driver pool, instance flexibility policy, and min_num_instances for Dataproc #34172

ahzaz · 2023-09-07T12:30:50Z

Add following to ClusterGenerator for Dataproc

min_num_workers: The minimum number of primary worker instances to create
driver_pool_size: The size of the cluster driver pool.
driver_pool_id: Custom identifier for the DRIVER Node Group being created.
secondary_worker_instance_flexibility_policy: Instance flexibility Policy allowing a mixture of VM shapes and provisioning models.
Upgrade google-cloud-dataproc to 5.5.0

boring-cyborg · 2023-09-07T12:30:55Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://s.apache.org/airflow-slack

SamWheating

A couple suggestions

airflow/providers/google/cloud/operators/dataproc.py

airflow/providers/google/provider.yaml

ahzaz · 2023-09-21T07:14:49Z

@potiuk @SamWheating Can you please review/approve the PR if there are no more concerns ?

SamWheating

A couple comments / suggestions

SamWheating · 2023-09-21T15:22:43Z

airflow/providers/google/cloud/operators/dataproc.py

@@ -84,6 +117,7 @@ class ClusterGenerator:
        to create the cluster. (templated)
    :param num_workers: The # of workers to spin up. If set to zero will
        spin up cluster in a single node mode
+    :param min_num_workers: The minimum number of primary worker instances to create.


Its not immediately clear how this value interacts with num_workers, might be worth elaborating a bit more?

From the API docs:

Example: Cluster creation request with num_instances = 5 and min_num_instances = 3:

If 4 VMs are created and 1 instance fails, the failed VM is deleted. The cluster is resized to 4 instances and placed in a RUNNING state.
If 2 instances are created and 3 instances fail, the cluster in placed in an ERROR state. The failed VMs are not deleted.

Could we add some of that context here? I don't have any strong opinions on the exact wording.

SamWheating · 2023-09-21T15:23:53Z

airflow/providers/google/cloud/operators/dataproc.py

@@ -344,6 +402,10 @@ def _build_cluster_data(self):
            "autoscaling_config": {},
            "endpoint_config": {},
        }
+
+        if self.min_num_workers:


Should we raise an Exception at initialization time if min_num_workers > num_workers?

It's usually best to surface issues as early as possible.

SamWheating · 2023-09-21T15:32:38Z

airflow/providers/google/cloud/operators/dataproc.py

+        driver_pool = {
+            "node_group": {
+                "roles": ["DRIVER"],
+                "node_group_config": {"num_instances": self.driver_pool_size},


Thoughts on allowing instance type here as well? Something like:

Suggested change

"node_group_config": {"num_instances": self.driver_pool_size},

"node_group_config": {

"num_instances": self.driver_pool_size

"machine_type_uri": self.driver_pool_machine_type

},

Maybe this is too much config though? I'm fine with leaving this for another day.

Let keep it these two for now. Later on, we can driver pool config (like InstanceFlexibilityPolicy) to cover rest of the config options.

ahzaz · 2023-09-27T05:23:12Z

@SamWheating Thank you for review. Can you please trigger the workflow?

SamWheating · 2023-09-27T16:12:30Z

I actually can't trigger the workflow - we'll have to wait for a committer to come along and do the final check + merge.

ahzaz · 2023-10-06T05:17:12Z

@potiuk Can you please help get this merged?

Taragolis

Looks good.

…_instances for Dataproc

ahzaz · 2023-11-15T09:17:00Z

@Taragolis Can you please help merge this.

boring-cyborg · 2023-11-16T11:21:55Z

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

The change in apache#34172 had been merged without rebasing and caused static check failure in main. This PR fixes it.

The change in #34172 had been merged without rebasing and caused static check failure in main. This PR fixes it.

boring-cyborg bot added area:providers provider:google Google (including GCP) related issues labels Sep 7, 2023

ahzaz force-pushed the dataproc-updates branch 2 times, most recently from 42e33bd to 0e87d7c Compare September 7, 2023 12:38

SamWheating reviewed Sep 7, 2023

View reviewed changes

airflow/providers/google/cloud/operators/dataproc.py Show resolved Hide resolved

airflow/providers/google/cloud/operators/dataproc.py Outdated Show resolved Hide resolved

airflow/providers/google/cloud/operators/dataproc.py Outdated Show resolved Hide resolved

eladkal reviewed Sep 8, 2023

View reviewed changes

airflow/providers/google/provider.yaml Show resolved Hide resolved

ahzaz force-pushed the dataproc-updates branch 5 times, most recently from 7da8f15 to 7c4dc41 Compare September 11, 2023 15:37

potiuk force-pushed the dataproc-updates branch from 7c4dc41 to 5950352 Compare September 11, 2023 21:15

ahzaz requested a review from potiuk September 13, 2023 05:49

ahzaz force-pushed the dataproc-updates branch from 5950352 to b6366f6 Compare September 13, 2023 05:49

ahzaz requested a review from SamWheating September 14, 2023 08:56

ahzaz force-pushed the dataproc-updates branch 2 times, most recently from fee1497 to f2302aa Compare September 21, 2023 06:05

SamWheating reviewed Sep 21, 2023

View reviewed changes

ahzaz force-pushed the dataproc-updates branch 2 times, most recently from 06cf8f9 to 9c36945 Compare September 26, 2023 09:05

SamWheating approved these changes Sep 26, 2023

View reviewed changes

ahzaz force-pushed the dataproc-updates branch from 9c36945 to fcdd197 Compare September 27, 2023 05:16

ahzaz force-pushed the dataproc-updates branch from fcdd197 to 703eff4 Compare October 6, 2023 05:17

ahzaz force-pushed the dataproc-updates branch from 703eff4 to 667843f Compare November 8, 2023 09:42

Taragolis approved these changes Nov 8, 2023

View reviewed changes

Add support for driver pool, instance flexibility policy, and min_num…

47308cf

…_instances for Dataproc

ahzaz force-pushed the dataproc-updates branch from 667843f to 47308cf Compare November 9, 2023 11:31

potiuk approved these changes Nov 16, 2023

View reviewed changes

potiuk merged commit 08d15d0 into apache:main Nov 16, 2023

potiuk added a commit to potiuk/airflow that referenced this pull request Nov 16, 2023

Fix generated depdendencies and breeze readme

d09ccb4

The change in apache#34172 had been merged without rebasing and caused static check failure in main. This PR fixes it.

potiuk mentioned this pull request Nov 16, 2023

Fix generated depdendencies and breeze readme #35680

Merged

potiuk added a commit that referenced this pull request Nov 16, 2023

Fix generated depdendencies and breeze readme (#35680)

9877f36

The change in #34172 had been merged without rebasing and caused static check failure in main. This PR fixes it.

ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Nov 20, 2023

ephraimbuddy added this to the Airflow 2.8.0 milestone Nov 20, 2023

eladkal mentioned this pull request Nov 24, 2023

Status of testing Providers that were prepared on November 24, 2023 #35845

Closed

69 tasks

Add support for driver pool, instance flexibility policy, and min_num_instances for Dataproc #34172

Add support for driver pool, instance flexibility policy, and min_num_instances for Dataproc #34172

Uh oh!

Conversation

ahzaz commented Sep 7, 2023

Uh oh!

boring-cyborg bot commented Sep 7, 2023

Uh oh!

SamWheating left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahzaz commented Sep 21, 2023

Uh oh!

SamWheating left a comment

Choose a reason for hiding this comment

Uh oh!

SamWheating Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

ahzaz Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

SamWheating Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

ahzaz Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

SamWheating Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

ahzaz Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

ahzaz commented Sep 27, 2023

Uh oh!

SamWheating commented Sep 27, 2023

Uh oh!

ahzaz commented Oct 6, 2023

Uh oh!

Taragolis left a comment

Choose a reason for hiding this comment

Uh oh!

ahzaz commented Nov 15, 2023

Uh oh!

boring-cyborg bot commented Nov 16, 2023

Uh oh!

Uh oh!