Allow Hive GCS to pass JSON key as a config #17892
Conversation
The validation is added in a non-standard manner (not annotation driven), as it would make it impossible to write `TestHiveGcsConfig.testExplicitPropertyMappings`.
findepi
force-pushed
the
findepi/gcs-credentials
branch
from
426b922 to
04e9fee
Compare
| @@ -58,6 +58,7 @@ public GcsStorageFactory(HdfsEnvironment hdfsEnvironment, HiveGcsConfig hiveGcsC | |||
| throws IOException | |||
| { | |||
| this.hdfsEnvironment = requireNonNull(hdfsEnvironment, "hdfsEnvironment is null"); | |||
| hiveGcsConfig.validate(); | |||
There was a problem hiding this comment.
Should support for validate method in Config class be added to airlift?
There was a problem hiding this comment.
airlift supports standard bean validation, so i don't think so
normally it works awesome
the problem for this particular config is with the testExplicitPropertyMappings that wants to set all properties to some value, and they are mutually exclusive
There was a problem hiding this comment.
i think the proper solution would be to split this into two config classes, and make one bound conditionally.
overkill i think.
| // This cannot be normal validation, as it would make it impossible to write TestHiveGcsConfig.testExplicitPropertyMappings | ||
|
|
||
| if (useGcsAccessToken) { | ||
| checkState(jsonKeyFilePath == null, "Cannot specify 'hive.gcs.json-key-file-path' when 'hive.gcs.use-access-token' is set"); |
There was a problem hiding this comment.
what about the other way around.
if jsonKeyFilePath is null is it expected that useGcsAccessToken is true?
There was a problem hiding this comment.
they are mutually exclusive, and i don't think it matters which way around i write it?
| { | ||
| try { | ||
| // Just create a temporary json key file. | ||
| Path tempFile = Files.createTempFile("gcs-key-", ".json", PosixFilePermissions.asFileAttribute(EnumSet.of(OWNER_READ, OWNER_WRITE))); |
There was a problem hiding this comment.
I am not a big fan of exposing credentials in temporary file - but I assume this is the only way you can integrate with Hive Configuration
There was a problem hiding this comment.
yes, this will be gone after we de-hadoop file systems
obviously this should be used only when deploying in a trusted isolated environment, like k8s
github-actions
bot
added
delta-lake
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/GcsStorageFactory.java
Show resolved
Hide resolved
lib/trino-hdfs/src/main/java/io/trino/hdfs/gcs/GoogleGcsConfigurationInitializer.java
Show resolved
Hide resolved
| return jsonKey; | ||
| } | ||
|
|
||
| @Config("hive.gcs.json-key") |
There was a problem hiding this comment.
@ConfigSecuritySensitive seems missing.
| return useGcsAccessToken; | ||
| } | ||
|
|
||
| @Config("hive.gcs.use-access-token") |
There was a problem hiding this comment.
Can we update hive-gcs-tutorial.rst? Follow-up is fine though.
There was a problem hiding this comment.
let's follow-up. Docs would need to have good advice on where this can be used, which is tricky.
Co-authored-by: Eric Hwang <[email protected]> Co-authored-by: Slawomir Pajak <[email protected]> Co-authored-by: Marius Grama <[email protected]>
findepi
force-pushed
the
findepi/gcs-credentials
branch
from
04e9fee to
a7d3f8e
Compare
|
Does this need release notes? @findepi |
findepi
added
the
no-release-notes
No description provided.