-
Notifications
You must be signed in to change notification settings - Fork 379
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
[#5991] feat(gcs): unify the GCS server acount path configuration for fileset and GCSCredentialProvider #5992
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help explain why this change is necessary?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 string
s.
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.
What changes were proposed in this pull request?
fileset use
gcs-service-account-file
while gcsTokenCredentialProvider usegcs-credential-file-path
, we'd better unify the name, seemsgcs-service-account-file
is betterWhy are the changes needed?
Fix: #5991
Does this PR introduce any user-facing change?
yes
How was this patch tested?
existing tests