-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Remove config property hive.gcs.scheme #11235
Remove config property hive.gcs.scheme #11235
Conversation
✅ Deploy Preview for meta-velox canceled.
|
a026723
to
826edd3
Compare
@tigrux any feedback on this? Thanks! |
826edd3
to
7c8b9c5
Compare
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.
Thank you!
It totally makes sense.
I tested this change locally. It works! |
@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
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.
Nit, lets make sure the doc is matching the (new) behavoir and leaves no questions.
velox/docs/configs.rst
Outdated
@@ -633,10 +633,6 @@ Each query can override the config by setting corresponding query session proper | |||
- string | |||
- | |||
- The GCS storage endpoint server. |
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.
Maybe should mention that this is a URI that includes the scheme rather than just the server? This PR changes the behavior of this variable. Before it did require a scheme-less hostname (+port).
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.
Good point. I added the endpoint default as https://storage.googleapis.com
since it is the most common.
I also updated the doc.
3a64483
to
922a3ea
Compare
@kgpai can you please reimport? |
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.
Looks good.
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
The GCS adapter expects two configs
hive.gcs.scheme
value for the scheme andhive.gcs.endpoint
value without the scheme.However, in practice, the endpoint value contains the scheme. We can remove
hive.gcs.scheme
to be consistent.See Trino for example https://trino.io/docs/current/admin/fault-tolerant-execution.html#google-cloud-storage