-
Notifications
You must be signed in to change notification settings - Fork 37
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
User tools fallback to default zone/region #1054
Conversation
Signed-off-by: Niranjan Artal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nartal1 !
A couple of minor comments.
@@ -55,15 +55,17 @@ | |||
}, | |||
{ | |||
"envVariableKey": "CLOUDSDK_DATAPROC_REGION", | |||
"confProperty": "region" | |||
"confProperty": "region", | |||
"defaultValue": "us-central1" | |||
}, | |||
{ | |||
"envVariableKey": "CLOUDSDK_COMPUTE_REGION", | |||
"confProperty": "region" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a default for CLOUDSDK_COMPUTE_REGION
too?
@@ -55,15 +55,17 @@ | |||
}, | |||
{ | |||
"envVariableKey": "CLOUDSDK_DATAPROC_REGION", | |||
"confProperty": "region" | |||
"confProperty": "region", | |||
"defaultValue": "us-central1" | |||
}, | |||
{ | |||
"envVariableKey": "CLOUDSDK_COMPUTE_REGION", | |||
"confProperty": "region" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a default for CLOUDSDK_COMPUTE_REGION too?
for prop_entry in properties_map_arr: | ||
prop_entry_key = prop_entry.get('propKey') | ||
if self.ctxt.get(prop_entry_key) is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a comment to explain what we are trying to do?
I know that the original code did not have much explanation, but it will be nice to add that since we are already modifying the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment. PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nartal1.
Since this updates region in our env_vars
and not the actual CLI configuration, CLI cmds such as
aws emr describe-cluster --cluster-id {cluster_id}
might crash because it will try to get region from the CLI config. We might have to add the region explicitly in these CLI cmds.
Repro CMD for EMR:
spark_rapids qualification --cluster my-cluster --platform emr --eventlogs <eventlog>
Signed-off-by: Niranjan Artal <[email protected]>
Yes @parthosa ! You are correct. Region becomes mandatory when
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nartal1. That makes sense. We cannot identify a cluster without the region.
Signed-off-by: Niranjan Artal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nartal1!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nartal1 for the fix.
Great team work @cindyyuanjiang and @parthosa
This fixes #1018.
This PR fallsback to default region/zone(where applicable) for CLI command when Region/Zone is not set by the user.
Earlier it would throw an error which doesn't show the exact reason though. With this PR, it continues with the default region with a warning that Region was not set and using the default values from environment variable.
In addition to it, updated the way the remaining environment variables are set in sp_types.py. Earlier condition would miss some of the environment variables.
Tested it on platform:
databricks-azure has already default defined.
Dataproc failure
Dataproc completion with this PR
EMR failure
EMR completion with this PR