[#5991] feat(gcs): unify the GCS server acount path configuration for fileset and GCSCredentialProvider #5992
Conversation
| @@ -22,7 +22,7 @@ | |||
| public class GCSProperties { | |||
|
|
|||
| // The path of service account JSON file of Google Cloud Storage. | |||
| public static final String GCS_SERVICE_ACCOUNT_JSON_PATH = "gcs-service-account-file"; | |||
| public static final String GRAVITINO_GCS_SERVICE_ACCOUNT_FILE = "gcs-service-account-file"; | |||
There was a problem hiding this comment.
Can you help explain why this change is necessary?
There was a problem hiding this comment.
Adding an GRAVITINO prefix declares this is a configuration for Gravitino , not for Iceberg, spark, or others, and keep consistent with other properties defined in S3Properties OSSProperties, etc
There was a problem hiding this comment.
@FANNG1 Yes. That was also my first perception of this naming style after having seen it prevail in many packages. But I'm still confused by such public static final strings.
You know, we are writing these code in Java, which is notorious/famous by its class hierarchies. In other words, we are not supposed to write two Foo classes in the same package. Each Foo has its own namespace. A Foo can be a Gravitino Foo and it can be a Iceberg Foo. We differentiate these two Foo classes by using their fully-qualified names. We are not supposed to add another level of complexity to the class hierarchy.
For most of the cases, a org.gravitino.Foo.CONFIG_FILE can be easily differentiated from a org.iceberg.Foo.CONFIG_FILE. Prepending GRAVATINO_ or ICEBERG_ to those variable/constant names is making them longer for no good. If these code is written in Go, for example, I'd strongly advise the other way around. A prefix can help us quickly locate the file where it is defined. Without a prefix, I have to grep the whole directory because GoLang compiler treats the whole directory as a single compilation unit. A variable can be defined in any source code. That design sucks.
I have seen simply too many cases where our contributors are repeating (copy-pasting) this pattern, and that is the reason I'm putting this forward for the team to reconsider, if appropriate.
There was a problem hiding this comment.
Gravitino as a metadata manage system, there are heavy works to transform different properties across different systems, take jdbc user for example, the name is jdbc-user for Gravitino, jdbc.user for Iceberg, adding a prefix like GRAVITINO_JDBC_USER, ICEBERG_JDBC_USER is clear to me, without the prefix we may use it by mistake more easily.
There was a problem hiding this comment.
another pattern is splitting to something like GravitinoProperties.JDBC_USER IcebergProperties.JDBC_USER, I agree this is more elegant and clear too, but requires an heavy refactor work for now.
There was a problem hiding this comment.
How about start this practice from today, in this PR, rather than deferring this to a huge refactor work later which may never happen?
There was a problem hiding this comment.
This should be GravitinoProperties.SERVICE_ACCOUNT_FILE, if using GCSProperties seems missing Gravitino information.
|
Ready for review now, @tengqm @jerryshao @yuqi1129 PTAL |
|
lgtm |
…on for fileset and GCSCredentialProvider (apache#5992) ### What changes were proposed in this pull request? fileset use `gcs-service-account-file` while gcsTokenCredentialProvider use `gcs-credential-file-path`, we'd better unify the name, use `gcs-service-account-file` for GCS credential to unify them. ### Why are the changes needed? Fix: apache#5991 ### Does this PR introduce _any_ user-facing change? yes ### How was this patch tested? existing tests
What changes were proposed in this pull request?
fileset use
gcs-service-account-filewhile gcsTokenCredentialProvider usegcs-credential-file-path, we'd better unify the name, usegcs-service-account-filefor GCS credential to unify them.Why are the changes needed?
Fix: #5991
Does this PR introduce any user-facing change?
yes
How was this patch tested?
existing tests