-
Notifications
You must be signed in to change notification settings - Fork 109
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
feat: add Impala connection optional parameters (#743) #790
Conversation
/gcbrun |
/gcbrun |
): | ||
auth_mechanism = (auth_mechanism, "PLAIN")[auth_mechanism is None] | ||
database = (database, "default")[database is None] | ||
port = (port, 10000)[port is None] | ||
kerberos_service_name = (kerberos_service_name, "impala")[ | ||
kerberos_service_name is None | ||
] | ||
use_ssl = (use_ssl, False)[use_ssl is None] | ||
timeout = (timeout, 45)[timeout is None] | ||
pool_size = (pool_size, 8)[pool_size 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.
Do we need to add user, password, ca_cert, and the other variables here too? Or will it be picked up from the default parameters?
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.
I took as a reference how the code was already handling "host" parameters. Since the default value is None then it is not added in this section; for the other variables the default values are boolean or Int, and that is why it validates if the value is None to assign the default value. If it requires a change just let me know.
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.
LGTM!
Closes issue #743
Add optional parameters supported by Impala Connector, to give support in DVT:
Parameters added:
Attaching result for execution of the help command for the connection: