DWX-19926: Add Impala properties #216
Conversation
|
@prateek1192 this PR looks like something really in progress, could you please change it to draft until it will be ready for review? |
I let some TODOs remain so that I can fix in Impala APIs and come back here but lemme remove those as well . |
prateek1192
force-pushed
the
dwx-19926
branch
from
cb79f47 to
9a32c29
Compare
| @@ -27,7 +27,48 @@ resource "cdp_dw_vw_impala" "impala-terraform" { | |||
| database_catalog_id = var.database_catalog_id | |||
| name = var.name | |||
| image_version = var.image_version | |||
| tshirt_size = var.tshirt_size | |||
There was a problem hiding this comment.
I think you forgot to add the example file to the change
There was a problem hiding this comment.
Thanks for pointing that out, I added it
| scratch_space_limit = var.scratch_space_limit | ||
| } | ||
|
|
||
| impala_ha_settings = { |
There was a problem hiding this comment.
this is an impala resource, simply ha_settings should be clear, no need to duplicate the type
There was a problem hiding this comment.
I feel like this is more consistent with swagger and also since this is a impala specific options and if at some point Hive adds it no matter how unlikely it feels right now, we would have a conflict. Lets just keep it.
|
|
||
| enable_unified_analytics = var.enable_unified_analytics | ||
|
|
||
| query_isolation_options = { |
There was a problem hiding this comment.
are you sure impala supports this? query isolation is a feature in hive
There was a problem hiding this comment.
you are right, this exists! can you implement in a way that it can be set only if the enable_unified_analytics is set? one other way to do it to add these into a "unified_analytics" group and set the query isolation from there, like
unified_analytics = {
enabled = bool
query_isolation_max_queries = var.max_queries
query_isolation_max_nodes_per_query = var.max_nodes_per_query
}
all options are optional I think
| } | ||
|
|
||
| impala_options = { | ||
| scratch_space_limit = var.scratch_space_limit |
There was a problem hiding this comment.
can the scratch space limit be set in azure?
There was a problem hiding this comment.
I checked with Abhishek Rawat and he said that Azure does have it
docs/resources/dw_vw_impala.md
Outdated
| hive_authentication_mode = var.hive_authentication_mode | ||
| platform_jwt_auth = var.platform_jwt_auth | ||
| impala_query_log = var.impala_query_log | ||
| ebs_llap_spill_gb = var.ebs_llap_spill_gb |
docs/resources/dw_vw_impala.md
Outdated
| } | ||
|
|
||
| resource_pool = var.resource_pool | ||
| hive_authentication_mode = var.hive_authentication_mode |
There was a problem hiding this comment.
What does this property represent? what auth mode can be set?
There was a problem hiding this comment.
this looks deprecated, because both kerberos and ldap options are enabled by default. However there should an an enable_sso option for impala and hive too
docs/resources/dw_vw_impala.md
Outdated
| max_nodes_per_query = var.max_nodes_per_query | ||
| } | ||
|
|
||
| resource_pool = var.resource_pool |
There was a problem hiding this comment.
this is the quota management feature, only private cloud supports this, it can be dropped for now
| min_clusters = var.min_clusters | ||
| } | ||
|
|
||
| impala_options = { |
There was a problem hiding this comment.
all of these are impala options :) if the scratch space can only be set in aws can you rename this to aws_options? I think the availability_zone can only be set in case of AWS
There was a problem hiding this comment.
I mean you are right :)
But I am trying to stay close to the swagger docs as much as possible, later on when someone looks at this and tries to figure out and sees that we have opiniated abstractions on top of swagger naming, that would be a real mess.
| "impala_scale_up_delay_seconds": types.Int32Type, | ||
| "max_clusters": types.Int32Type, | ||
| "min_clusters": types.Int32Type, | ||
| /*"impala_executor_group_sets": types.ListType{ |
There was a problem hiding this comment.
yeah you should remove these from here. But keep them on a local branch, I mean if you will add them later it is good to keep it around somewhere.
There was a problem hiding this comment.
Thanks, I added a backup branch with the name of this branch but backup appended in the end so that I can come back and fix :)
Thanks for the review.
This commit adds the Impala resource and adds all the remaining properties except for a config and a subset of Autoscaling group. Those two need changes in Impala APIs as such would make those changes and would come back and try to enable the code here.