Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cluster - Client library API migration changes #177

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
14a7916
list and get cluster temporary changes
Jeyaprakash-NK Apr 15, 2024
75e1576
Merge branch 'main' of https://github.com/Shubha-accenture/dataproc-j…
Jeyaprakash-NK Apr 15, 2024
c8f9643
Merge branch 'main' of https://github.com/Shubha-accenture/dataproc-j…
Jeyaprakash-NK Apr 16, 2024
654b621
cluster service BE code change
Jeyaprakash-NK Apr 16, 2024
9659c02
list cluster client library temp changes
Jeyaprakash-NK Apr 17, 2024
3708794
list and get cluster api status changes
Jeyaprakash-NK Aug 8, 2024
25e25c1
stop cluster BE and FE temp changes
Jeyaprakash-NK Aug 12, 2024
cdec0d7
controller rename changes
Jeyaprakash-NK Aug 12, 2024
2a37f2c
latest pull from main and conflicts resolved
Jeyaprakash-NK Aug 12, 2024
d539d90
start cluster and BE temp changes
Jeyaprakash-NK Aug 12, 2024
1d60038
Service file rename changes
Jeyaprakash-NK Aug 12, 2024
611fbac
Merge branch 'main' of https://github.com/Shubha-accenture/dataproc-j…
Jeyaprakash-NK Aug 14, 2024
cd84677
delete cluster and auth access token fix
Jeyaprakash-NK Aug 19, 2024
2b7b95b
await changes in start, stop, delete
Jeyaprakash-NK Aug 19, 2024
9b71492
delete cluster empty handled
Jeyaprakash-NK Aug 19, 2024
312902a
added new dependency "google-cloud-dataproc"
Jeyaprakash-NK Aug 20, 2024
68c21db
code cleanup
Jeyaprakash-NK Aug 22, 2024
4d70971
Code review comments fix
Jeyaprakash-NK Sep 5, 2024
65d004f
pull from main and conflicts resolved
Jeyaprakash-NK Sep 6, 2024
2eb3d98
package conflicts resolved in pyproject
Jeyaprakash-NK Sep 6, 2024
cb818d0
Code review feedback changes BE and FE
Jeyaprakash-NK Sep 6, 2024
8a6072a
line space removed
Jeyaprakash-NK Sep 6, 2024
c5ae698
api endpoint changes in client library
Jeyaprakash-NK Sep 6, 2024
6daa124
prettier changes
Jeyaprakash-NK Sep 6, 2024
7f64795
runtime list code review fix
Jeyaprakash-NK Sep 9, 2024
50f2d2d
Merge branch 'main' of https://github.com/Shubha-accenture/dataproc-j…
Jeyaprakash-NK Sep 11, 2024
a734f1e
changed all package versions >= instead ~=
Jeyaprakash-NK Sep 12, 2024
ff65fda
code format fix python
Jeyaprakash-NK Sep 12, 2024
7580728
pyproject version change
Jeyaprakash-NK Sep 13, 2024
36dfd9e
pyproject change '>=' from '~='
Jeyaprakash-NK Sep 13, 2024
a3edf3d
client library test for list cluster
Jeyaprakash-NK Sep 16, 2024
d8e3567
aioHttp pyproject version change
Jeyaprakash-NK Sep 16, 2024
f87ce93
list cluster revert changes test
Jeyaprakash-NK Sep 16, 2024
f656955
test changes for list cluster
Jeyaprakash-NK Sep 16, 2024
218ecde
list cluster test file changes
Jeyaprakash-NK Sep 23, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 61 additions & 12 deletions dataproc_jupyter_plugin/controllers/dataproc.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
from dataproc_jupyter_plugin.services import dataproc


class ClusterListController(APIHandler):
class RuntimeController(APIHandler):
@tornado.web.authenticated
async def get(self):
try:
Expand All @@ -32,25 +32,74 @@ async def get(self):
client = dataproc.Client(
await credentials.get_cached(), self.log, client_session
)
cluster_list = await client.list_clusters(page_size, page_token)
self.finish(json.dumps(cluster_list))
runtime_list = await client.list_runtime(page_size, page_token)
self.finish(json.dumps(runtime_list))
except Exception as e:
self.log.exception("Error fetching cluster list")
self.log.exception(f"Error fetching runtime template list: {str(e)}")
self.finish({"error": str(e)})


class RuntimeController(APIHandler):
class ClusterListController(APIHandler):
@tornado.web.authenticated
async def get(self):
try:
page_token = self.get_argument("pageToken")
page_size = self.get_argument("pageSize")
async with aiohttp.ClientSession() as client_session:
client = dataproc.Client(
await credentials.get_cached(), self.log, client_session
)
runtime_list = await client.list_runtime(page_size, page_token)
self.finish(json.dumps(runtime_list))
client = dataproc.Client(await credentials.get_cached(), self.log)
cluster_list = await client.list_clusters(page_size, page_token)
self.finish(json.dumps(cluster_list))
except Exception as e:
self.log.exception(f"Error fetching runtime template list: {str(e)}")
self.log.exception(f"Error fetching cluster list")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include the error in the log message

self.finish({"error": str(e)})


class ClusterDetailController(APIHandler):
@tornado.web.authenticated
async def get(self):
try:
cluster = self.get_argument("cluster")
client = dataproc.Client(await credentials.get_cached(), self.log)
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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.finish({"error": str(e)})


class StopClusterController(APIHandler):
@tornado.web.authenticated
async def post(self):
try:
cluster = self.get_argument("cluster")
client = dataproc.Client(await credentials.get_cached(), self.log)
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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.finish({"error": str(e)})


class StartClusterController(APIHandler):
@tornado.web.authenticated
async def post(self):
try:
cluster = self.get_argument("cluster")
client = dataproc.Client(await credentials.get_cached(), self.log)
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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.finish({"error": str(e)})


class DeleteClusterController(APIHandler):
@tornado.web.authenticated
async def delete(self):
try:
cluster = self.get_argument("cluster")
client = dataproc.Client(await credentials.get_cached(), self.log)
delete_cluster = await client.delete_cluster(cluster)
self.finish(json.dumps(delete_cluster))
except Exception as e:
self.log.exception(f"Error deleting cluster")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include the error in the log message

self.finish({"error": str(e)})
4 changes: 4 additions & 0 deletions dataproc_jupyter_plugin/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,10 @@ def full_path(name):
"dagRunTask": airflow.DagRunTaskController,
"dagRunTaskLogs": airflow.DagRunTaskLogsController,
"clusterList": dataproc.ClusterListController,
"clusterDetail": dataproc.ClusterDetailController,
"stopCluster": dataproc.StopClusterController,
"startCluster": dataproc.StartClusterController,
"deleteCluster": dataproc.DeleteClusterController,
"runtimeList": dataproc.RuntimeController,
"createJobScheduler": executor.ExecutorController,
"dagList": airflow.DagListController,
Expand Down
163 changes: 144 additions & 19 deletions dataproc_jupyter_plugin/services/dataproc.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,15 @@
DATAPROC_SERVICE_NAME,
)

from google.cloud import dataproc_v1 as dataproc
import proto
import json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

import google.oauth2.credentials as oauth2
from google.protobuf.empty_pb2 import Empty


class Client:
def __init__(self, credentials, log, client_session):
def __init__(self, credentials, log, client_session=None):
self.log = log
if not (
("access_token" in credentials)
Expand All @@ -40,10 +46,10 @@ def create_headers(self):
"Authorization": f"Bearer {self._access_token}",
}

async def list_clusters(self, page_size, page_token):
async def list_runtime(self, page_size, page_token):
try:
dataproc_url = await urls.gcp_service_url(DATAPROC_SERVICE_NAME)
api_endpoint = f"{dataproc_url}/v1/projects/{self.project_id}/regions/{self.region_id}/clusters?pageSize={page_size}&pageToken={page_token}"
api_endpoint = f"{dataproc_url}/v1/projects/{self.project_id}/locations/{self.region_id}/sessionTemplates?pageSize={page_size}&pageToken={page_token}"
async with self.client_session.get(
api_endpoint, headers=self.create_headers()
) as response:
Expand All @@ -52,27 +58,146 @@ async def list_clusters(self, page_size, page_token):
return resp
else:
return {
"error": f"Failed to fetch clusters: {response.status} {await response.text()}"
"error": f"Failed to fetch runtimes: {response.status} {await response.text()}"
}
except Exception as e:
self.log.exception(f"Error fetching runtime list: {str(e)}")
return {"error": str(e)}

async def list_clusters(self, page_size, page_token):
try:
# Create a client
client = dataproc.ClusterControllerAsyncClient(
client_options={
"api_endpoint": f"us-central1-dataproc.googleapis.com:443"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
credentials=oauth2.Credentials(self._access_token),
)

# Initialize request argument(s)
request = dataproc.ListClustersRequest(
project_id=self.project_id,
page_size=int(page_size),
page_token=page_token,
region=self.region_id,
)

# Make the request
page_result = await client.list_clusters(request=request)
clusters_list = []

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return clusters_list
except Exception as e:
self.log.exception("Error fetching cluster list")
self.log.exception(f"Error fetching cluster list")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return {"error": str(e)}

async def list_runtime(self, page_size, page_token):
async def get_cluster_detail(self, cluster):
try:
dataproc_url = await urls.gcp_service_url(DATAPROC_SERVICE_NAME)
api_endpoint = f"{dataproc_url}/v1/projects/{self.project_id}/locations/{self.region_id}/sessionTemplates?pageSize={page_size}&pageToken={page_token}"
async with self.client_session.get(
api_endpoint, headers=self.create_headers()
) as response:
if response.status == 200:
resp = await response.json()
return resp
else:
return {
"error": f"Failed to fetch runtimes: {response.status} {await response.text()}"
}
# Create a client
client = dataproc.ClusterControllerAsyncClient(
client_options={
"api_endpoint": f"us-central1-dataproc.googleapis.com:443"
},
credentials=oauth2.Credentials(self._access_token),
)

# Initialize request argument(s)
request = dataproc.GetClusterRequest(
project_id=self.project_id,
region=self.region_id,
cluster_name=cluster,
)

# Make the request
response = await client.get_cluster(request=request)

# Handle the response
return json.loads(proto.Message.to_json(response))
except Exception as e:
self.log.exception(f"Error fetching runtime list: {str(e)}")
self.log.exception(f"Error fetching cluster detail")
return {"error": str(e)}

async def stop_cluster(self, cluster):
try:
# Create a client
client = dataproc.ClusterControllerAsyncClient(
client_options={
"api_endpoint": f"us-central1-dataproc.googleapis.com:443"
},
credentials=oauth2.Credentials(self._access_token),
)

# Initialize request argument(s)
request = dataproc.StopClusterRequest(
project_id=self.project_id,
region=self.region_id,
cluster_name=cluster,
)

operation = await client.stop_cluster(request=request)

response = await operation.result()
# Handle the response
return json.loads(proto.Message.to_json(response))
except Exception as e:
self.log.exception(f"Error fetching stop cluster")
return {"error": str(e)}

async def start_cluster(self, cluster):
try:
# Create a client
client = dataproc.ClusterControllerAsyncClient(
client_options={
"api_endpoint": f"us-central1-dataproc.googleapis.com:443"
},
credentials=oauth2.Credentials(self._access_token),
)

# Initialize request argument(s)
request = dataproc.StartClusterRequest(
project_id=self.project_id,
region=self.region_id,
cluster_name=cluster,
)

operation = await client.start_cluster(request=request)

response = await operation.result()
# Handle the response
return json.loads(proto.Message.to_json(response))
except Exception as e:
self.log.exception(f"Error fetching start cluster")
return {"error": str(e)}

async def delete_cluster(self, cluster):
try:
# Create a client
client = dataproc.ClusterControllerAsyncClient(
client_options={
"api_endpoint": f"us-central1-dataproc.googleapis.com:443"
},
credentials=oauth2.Credentials(self._access_token),
)

# Initialize request argument(s)
request = dataproc.DeleteClusterRequest(
project_id=self.project_id,
region=self.region_id,
cluster_name=cluster,
)

operation = await client.delete_cluster(request=request)

response = await operation.result()
# Handle the response
if isinstance(response, Empty):
return "Deleted successfully"
else:
return json.loads(proto.Message.to_json(response))
except Exception as e:
self.log.exception(f"Error deleting cluster")
return {"error": str(e)}
3 changes: 2 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ dependencies = [
"pendulum>=3.0.0",
"pydantic~=1.10.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we pinning the minor versions in these packages?

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

"bigframes~=0.22.0",
"aiohttp~=3.9.5"
"aiohttp~=3.9.5",
"google-cloud-dataproc~=5.10.2"
]
dynamic = ["version", "description", "authors", "urls", "keywords"]

Expand Down
Loading
Loading