Cluster - Client library API migration changes #177

Jeyaprakash-NK · 2024-08-22T11:38:48Z

Replaced the below mentioned client side GCP API calls with server side calls:

listClustersAPIService
getClusterDetailsService
statusApiService
restartClusterApiService
deleteClusterApi
startClusterApi
stopClusterApi

…upyter-plugin-fork into sprint19-api-migration-changes

ojarjur · 2024-09-04T16:09:15Z

src/utils/utils.ts

    return STATUS_PROVISIONING;
  } else {
-    return data.status.state;
+    return ClusterStatusState[data.status.state.toString()];


Change the dictionary to use integer keys and then drop this toString() call.

ojarjur · 2024-09-04T16:12:00Z

src/utils/const.ts

+  '0': 'UNKNOWN',
+  '1': 'CREATING',
+  '2': 'RUNNING',
+  '3': 'ERROR',
+  '4': 'DELETING',
+  '5': 'UPDATING',
+  '6': 'STOPPING',
+  '7': 'STOPPED',
+  '8': 'STARTING',
+  '9': 'ERROR_DUE_TO_UPDATE',
+  '10': 'REPAIRING'


This is reproducing a lot of constant strings that are already defined below here (everything except UNKNOWN, UPDATING, ERROR_DUE_TO_UPDATE, and REPAIRING).

Instead, move this dictionary to the end of the file, add new constants for the 4 entries that don't already have one, and then reference the existing constants rather than reproducing them.

ojarjur · 2024-09-04T16:39:50Z

dataproc_jupyter_plugin/controllers/cluster.py

@@ -0,0 +1,95 @@
+# Copyright 2023 Google LLC


We already have a file in the controllers directory for Dataproc called dataproc.

Move all of the methods from this file into that one and then delete this entire file.

ojarjur · 2024-09-04T16:41:30Z

dataproc_jupyter_plugin/services/cluster.py

@@ -0,0 +1,171 @@
+# Copyright 2023 Google LLC


We already have a file in the services directory for Dataproc called dataproc.

Move all of the methods from this file into that one and then delete this entire file.

ojarjur · 2024-09-04T16:42:43Z

dataproc_jupyter_plugin/controllers/cluster.py

+    @tornado.web.authenticated
+    async def get(self):
+        try:
+            cluster_selected = self.get_argument("clusterSelected")


Change all instances of clusterSelected to just cluster.

ojarjur · 2024-09-04T16:46:47Z

dataproc_jupyter_plugin/controllers/cluster.py

+from dataproc_jupyter_plugin.services import cluster
+
+
+class ClusterListPageController(APIHandler):


Completely remove this method. It duplicates the ClusterListController

ojarjur · 2024-09-04T16:47:24Z

dataproc_jupyter_plugin/handlers.py

@@ -193,6 +194,11 @@ def full_path(name):
        "dagRunTask": airflow.DagRunTaskController,
        "dagRunTaskLogs": airflow.DagRunTaskLogsController,
        "clusterList": dataproc.ClusterListController,
+        "clusterListPage": cluster.ClusterListPageController,


Drop this line entirely; there's no justification for having two different endpoints that make the exact same API call.

ojarjur · 2024-09-04T16:48:38Z

src/cluster/listCluster.tsx

@@ -214,23 +215,23 @@ function ListCluster({
      <div
        role="button"
        aria-disabled={
-          data.status.state !== ClusterStatus.STATUS_STOPPED &&
+          ClusterStatusState[data.status.state.toString()] !== ClusterStatus.STATUS_STOPPED &&


Drop all of the toString() calls and instead use integer keys for the dictionary.

ojarjur · 2024-09-05T17:22:36Z

dataproc_jupyter_plugin/services/dataproc.py

+            # Create a client
+            client = dataproc.ClusterControllerAsyncClient(
+                client_options={
+                    "api_endpoint": f"us-central1-dataproc.googleapis.com:443"


This is clearly wrong on multiple levels...

First off al, we can't hardcode the API endpoint to a single region. In fact, I don't see why we would specify a region at all... although a region can be specified as part of this, the default used by the client library if it is not specified does not have a region in it.

Further, we have to use the API endpoint override for Dataproc if it was configured by the user.

E.G. we could detect if the user configured this using await urls.gcp_service_url(DATAPROC_SERVICE_NAME, default='unset'), and then only if the value is not unset, then we configure the client_options with an api_endpoint taken from the hostname of the configured URL.

Finally, this logic needs to move into the __init__ method so that it is only written once rather than reproduced in every single method of the class.

ojarjur · 2024-09-05T18:03:09Z

dataproc_jupyter_plugin/services/dataproc.py

@@ -18,9 +18,15 @@
    DATAPROC_SERVICE_NAME,
 )

+from google.cloud import dataproc_v1 as dataproc
+import proto
+import json


This import order is wrong.

Standard library imports (e.g. json) have to go first, then external imports (google.cloud, google.oauth2.credentials, google.protobuf.empty_pb2), and finally local packages (dataproc_jupyter_plugin and it's sub packages).

Within each section, the imports should be in alphabetical order unless the imports have side effects that must be performed in a specific order.

ojarjur · 2024-09-05T18:07:23Z

dataproc_jupyter_plugin/controllers/dataproc.py

+            get_cluster = await client.get_cluster_detail(cluster)
+            self.finish(json.dumps(get_cluster))
+        except Exception as e:
+            self.log.exception(f"Error fetching get cluster")


fetching get is redundant. Just say Error fetching a cluster, but also add the error itself to the log message.

e.g. f"Error fetching a cluster: {str(e)}"

ojarjur · 2024-09-05T18:07:47Z

dataproc_jupyter_plugin/controllers/dataproc.py

+            stop_cluster = await client.stop_cluster(cluster)
+            self.finish(json.dumps(stop_cluster))
+        except Exception as e:
+            self.log.exception(f"Error fetching stop cluster")


There's no fetch happening here. The message should be f"Error stopping a cluster: {str(e)}"

ojarjur · 2024-09-05T18:08:54Z

dataproc_jupyter_plugin/controllers/dataproc.py

        except Exception as e:
-            self.log.exception(f"Error fetching runtime template list: {str(e)}")
+            self.log.exception(f"Error fetching cluster list")


Include the error in the log message

ojarjur · 2024-09-05T18:09:49Z

dataproc_jupyter_plugin/controllers/dataproc.py

+            start_cluster = await client.start_cluster(cluster)
+            self.finish(json.dumps(start_cluster))
+        except Exception as e:
+            self.log.exception(f"Error fetching start cluster")


Again, there is no fetch and the error must be included in the log message

ojarjur · 2024-09-05T18:10:05Z

dataproc_jupyter_plugin/controllers/dataproc.py

+            delete_cluster = await client.delete_cluster(cluster)
+            self.finish(json.dumps(delete_cluster))
+        except Exception as e:
+            self.log.exception(f"Error deleting cluster")


Include the error in the log message

ojarjur · 2024-09-05T18:21:55Z

dataproc_jupyter_plugin/services/dataproc.py


+            # Handle the response
+            async for response in page_result:
+                clusters_list.append(json.loads(proto.Message.to_json(response)))


You're traversing the message, serializing it as a string representing a JSON object, and then parsing that string to get back a Python dictionary.

All of the while, there is a corresponding method that just directly generates a dictionary without having to write to a string first.

Further, you are taking these resulting dictionaries, which use integers for enum values, and manually converting those integers to the corresponding enum value names.

However, I see that there is a keyword parameter on these methods that will use the enum value names to begin with if it is set to False.

Please change all of the calls to proto.Message.to_json(...) in this file to corresponding calls to proto.Message.to_dict(..., use_integers_for_enums=False), and then delete the integer-to-enum value name mapping from the constants file.

ojarjur · 2024-09-05T18:23:02Z

dataproc_jupyter_plugin/services/dataproc.py

        except Exception as e:
-            self.log.exception("Error fetching cluster list")
+            self.log.exception(f"Error fetching cluster list")


Everywhere in this file where we log an exception, include the actual exception in the log message.

…upyter-plugin-fork into sprint19-api-migration-changes

ojarjur · 2024-09-12T00:31:53Z

dataproc_jupyter_plugin/services/dataproc.py

@@ -33,17 +37,18 @@ def __init__(self, credentials, log, client_session):
        self.project_id = credentials["project_id"]
        self.region_id = credentials["region_id"]
        self.client_session = client_session
+        self.dataproc_url = dataproc_url
+        self.api_endpoint = f"{self.region_id}-{dataproc_url.split('/')[2]}:443"


OK, so it seems clear that the region name is required for the Dataproc API when not using an API endpoint override.

Our support for API endpoint overrides is primarily to support users of private service connect, so I went ahead and created a private service connect endpoint to access the Dataproc API, and tested it out to see how the expected DNS name in that case compares to the DNS name when not using private service connect.

It turns out that when using the default DNS names (e.g. dataproc-<ENDPOINT>.p.googleapis.com), you also have to add a prefix on the domain name for the region, or else you get this same error.

As such, my concerns appear to have been unwarranted, and we do in fact want to add on the {region}- prefix onto the domain name for the API endpoint override.

ojarjur · 2024-09-12T00:35:29Z

pyproject.toml

@@ -30,6 +30,7 @@ dependencies = [
    "pydantic~=1.10.0",


Why are we pinning the minor versions in these packages?

I.E. why "~=.." instead of ">=.."?

ojarjur · 2024-09-12T00:36:19Z

pyproject.toml

@@ -30,6 +30,7 @@ dependencies = [
    "pydantic~=1.10.0",
    "bigframes~=0.22.0",
    "aiohttp~=3.9.5",
+    "google-cloud-dataproc~=5.10.2",


We need to support the latest version, which is "5.11.0".

ojarjur

Please fix the test failures that this change has introduced.

ojarjur · 2024-09-17T22:19:41Z

dataproc_jupyter_plugin/tests/test_dataproc.py

-    response = await jp_fetch(
-        "dataproc-plugin",
-        "clusterList",
-        params={"pageSize": mock_page_size, "pageToken": mock_page_token},
-    )


This call needs to still be in the updated test. This is the entire point of the test.

ojarjur · 2024-09-17T22:21:22Z

dataproc_jupyter_plugin/tests/test_dataproc.py

+    ],
+)
+
+def test_list_clusters(request_type, transport: str = "grpc"):


This modified method does not actually test listing clusters via the server.

There needs to be an invocation of the jp_fetch method used to hit the /dataproc-plugin/clusterList endpoint, and the response from that call needs to be inspected to verify that it actually called into the underlying API client.

ojarjur · 2024-09-23T16:42:29Z

dataproc_jupyter_plugin/tests/test_dataproc.py

    response = await jp_fetch(
        "dataproc-plugin",
        "clusterList",
        params={"pageSize": mock_page_size, "pageToken": mock_page_token},
    )
    assert response.code == 200
-    payload = json.loads(response.body)


We still need to parse and validate the response body, which means we should also be mocking the list clusters call within the Dataproc client library.

…nt21-gsutil-async-changes gsutil - review comments changes

Jeyaprakash-NK and others added 17 commits April 15, 2024 15:32

list and get cluster temporary changes

14a7916

Merge branch 'main' of https://github.com/Shubha-accenture/dataproc-j…

75e1576

…upyter-plugin-fork into sprint19-api-migration-changes

Merge branch 'main' of https://github.com/Shubha-accenture/dataproc-j…

c8f9643

…upyter-plugin-fork into sprint19-api-migration-changes

cluster service BE code change

654b621

list cluster client library temp changes

9659c02

list and get cluster api status changes

3708794

stop cluster BE and FE temp changes

25e25c1

controller rename changes

cdec0d7

latest pull from main and conflicts resolved

2a37f2c

start cluster and BE temp changes

d539d90

Service file rename changes

1d60038

Merge branch 'main' of https://github.com/Shubha-accenture/dataproc-j…

611fbac

…upyter-plugin-fork into sprint19-api-migration-changes

delete cluster and auth access token fix

cd84677

await changes in start, stop, delete

2b7b95b

delete cluster empty handled

9b71492

added new dependency "google-cloud-dataproc"

312902a

code cleanup

68c21db

Jeyaprakash-NK requested review from ywskycn, jinnthehuman, harsha-accenture and Shubha-accenture August 22, 2024 11:38

jinnthehuman approved these changes Aug 22, 2024

View reviewed changes

Shubha-accenture added the kokoro:force-run label Aug 26, 2024

kokoro-team removed the kokoro:force-run label Aug 26, 2024

Jeyaprakash-NK requested a review from ojarjur September 4, 2024 04:36

ojarjur requested changes Sep 4, 2024

View reviewed changes

Code review comments fix

4d70971

ojarjur requested changes Sep 5, 2024

View reviewed changes

Jeyaprakash-NK added 2 commits September 6, 2024 11:25

pull from main and conflicts resolved

65d004f

package conflicts resolved in pyproject

2eb3d98

Merge branch 'main' of https://github.com/Shubha-accenture/dataproc-j…

50f2d2d

…upyter-plugin-fork into sprint19-api-migration-changes

Jeyaprakash-NK requested a review from ojarjur September 11, 2024 07:53

ojarjur reviewed Sep 12, 2024

View reviewed changes

changed all package versions >= instead ~=

a734f1e

Jeyaprakash-NK requested a review from ojarjur September 12, 2024 06:15

ojarjur approved these changes Sep 12, 2024

View reviewed changes

ojarjur requested changes Sep 12, 2024

View reviewed changes

code format fix python

ff65fda

Shubha-accenture added the kokoro:force-run label Sep 13, 2024

kokoro-team removed the kokoro:force-run label Sep 13, 2024

Jeyaprakash-NK added the kokoro:force-run label Sep 13, 2024

kokoro-team removed the kokoro:force-run label Sep 13, 2024

Jeyaprakash-NK added the kokoro:force-run label Sep 13, 2024

kokoro-team removed the kokoro:force-run label Sep 13, 2024

Jeyaprakash-NK added 6 commits September 13, 2024 13:42

pyproject version change

7580728

pyproject change '>=' from '~='

36dfd9e

client library test for list cluster

a3edf3d

aioHttp pyproject version change

d8e3567

list cluster revert changes test

f87ce93

test changes for list cluster

f656955

Jeyaprakash-NK added the kokoro:force-run label Sep 16, 2024

kokoro-team removed the kokoro:force-run label Sep 16, 2024

Jeyaprakash-NK added the kokoro:force-run label Sep 16, 2024

kokoro-team removed the kokoro:force-run label Sep 16, 2024

Jeyaprakash-NK requested a review from ojarjur September 16, 2024 13:59

ojarjur requested changes Sep 17, 2024

View reviewed changes

list cluster test file changes

218ecde

Jeyaprakash-NK requested a review from ojarjur September 23, 2024 10:42

ojarjur reviewed Sep 23, 2024

View reviewed changes

jinnthehuman pushed a commit to jinnthehuman/dataproc-jupyter-plugin that referenced this pull request Dec 11, 2024

Merge pull request GoogleCloudDataproc#177 from Shubha-accenture/spri…

1c9f91d

…nt21-gsutil-async-changes gsutil - review comments changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cluster - Client library API migration changes #177

Cluster - Client library API migration changes #177

Jeyaprakash-NK commented Aug 22, 2024 •

edited by Shubha-accenture

Loading

ojarjur Sep 4, 2024

ojarjur Sep 4, 2024

ojarjur Sep 4, 2024

ojarjur Sep 4, 2024

ojarjur Sep 4, 2024

ojarjur Sep 4, 2024

ojarjur Sep 4, 2024

ojarjur Sep 4, 2024

ojarjur Sep 5, 2024

ojarjur Sep 5, 2024

ojarjur Sep 5, 2024

ojarjur Sep 5, 2024

ojarjur Sep 5, 2024

ojarjur Sep 5, 2024

ojarjur Sep 5, 2024

ojarjur Sep 5, 2024

ojarjur Sep 5, 2024

ojarjur Sep 12, 2024

ojarjur Sep 12, 2024

ojarjur Sep 12, 2024

ojarjur left a comment

ojarjur Sep 17, 2024

ojarjur Sep 17, 2024

ojarjur Sep 23, 2024

		from dataproc_jupyter_plugin.services import cluster


		class ClusterListPageController(APIHandler):

Cluster - Client library API migration changes #177

Are you sure you want to change the base?

Cluster - Client library API migration changes #177

Conversation

Jeyaprakash-NK commented Aug 22, 2024 • edited by Shubha-accenture Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojarjur 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

Jeyaprakash-NK commented Aug 22, 2024 •

edited by Shubha-accenture

Loading