-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Document missing AWS Glue configs in Hive connector #3689
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/prestosql/cla. |
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 suggested a couple of minor edits
I'll address the review comments over the weekend - a little busy with work stuff for now. I hope that's okay. |
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/prestosql/cla. |
@mosabua can you do a re-review. I have applied the suggested changes. |
@@ -305,13 +305,24 @@ Property Name Description | |||
running in EC2, or when the catalog is in a different region. | |||
Example: ``us-east-1`` | |||
|
|||
``hive.metastore.glue.endpoint-url`` Glue API endpoint URL (optional). Might be useful in |
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.
Remove "Might be" and simplify .. so .. "Useful when all requests to Glue should be proxied.
``hive.metastore.glue.default-warehouse-dir`` Default warehouse directory for tables created without an | ||
explicit ``LOCATION`` property. | ||
|
||
``hive.metastore.glue.catalogid`` Glue Catalog id (optional). |
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.
either ID or identifier.
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.
Two minor changes .. otherwise good to go.
@mosabua Applied recent suggestions. Thanks for the review. |
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.
Docs look good.
``hive.metastore.glue.endpoint-url`` Glue API endpoint URL (optional). Useful when all requests | ||
to Glue should be proxied. | ||
Example: ``https://glue.us-east-1.amazonaws.com`` |
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 am not sure "be proxied" is the right term, as this implies they would end up hitting AWS APIs somehow, whereas this could be used to provide your own standalone glue implementation.
I think "Glue API endpoint URL" should be enough for now.
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.
Changed. That makes sense.
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.
One more, sorry overlooked that previously
Document the new configurations introduced as part of #1465 and a few other configs introduced over time. Co-authored-by: Piotr Findeisen <[email protected]>
Merged, thanks! |
Cherry pick of trinodb/trino#3689 and https://github.com/vincentpoon/prestosql/commit/c1ac9ac257bc5a07f32a359c7aac6735fd6ef69f Co-authored-by: Philippe Gagnon <[email protected]> Co-authored-by: Ashhar Hasan <[email protected]>
Cherry pick of trinodb/trino#3689 and https://github.com/vincentpoon/prestosql/commit/c1ac9ac257bc5a07f32a359c7aac6735fd6ef69f Co-authored-by: Philippe Gagnon <[email protected]> Co-authored-by: Ashhar Hasan <[email protected]>
Cherry-pick of trinodb/trino#1363, trinodb/trino#741 and trinodb/trino#3689 Add config hive.metastore.glue.aws-credentials-provider for glue credential provider. where value is fully qualified class name of custom aws credential provider implementation. Co-authored-by: Li Yu <[email protected]> Co-authored-by: Anoop Johnson <[email protected]> Co-authored-by: Ashhar Hasan <[email protected]>
Cherry-pick of trinodb/trino#3689 Add configuration property ``hive.metastore.glue.aws-credentials-provider`` to supply a custom credential provider. Co-authored-by: Ashhar Hasan <[email protected]> Co-authored-by: Piotr Findeisen <[email protected]>
Document the new configurations introduced as part of
#1465 and a few other configs
introduced over time.