Skip to content
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.

Sign up for GitHub

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

[#5991] feat(gcs): unify the GCS server acount path configuration for fileset and GCSCredentialProvider #5992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3370344
fix gccs
FANNG1 Dec 25, 2024
6f4603e
fix gccs
FANNG1 Dec 25, 2024
a86e9d1
fix gccs
FANNG1 Dec 25, 2024
5b6a7ca
fix gccs
FANNG1 Dec 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion bundles/gcp-bundle/src/main/java/org/apache/gravitino/gcs/fs/GCSFileSystemProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ public class GCSFileSystemProvider implements FileSystemProvider {

@VisibleForTesting
public static final Map<String, String> GRAVITINO_KEY_TO_GCS_HADOOP_KEY =
ImmutableMap.of(GCSProperties.GCS_SERVICE_ACCOUNT_JSON_PATH, GCS_SERVICE_ACCOUNT_JSON_FILE);
ImmutableMap.of(
GCSProperties.GRAVITINO_GCS_SERVICE_ACCOUNT_FILE, GCS_SERVICE_ACCOUNT_JSON_FILE);

@Override
public FileSystem getFileSystem(Path path, Map<String, String> config) throws IOException {
Expand Down
2 changes: 1 addition & 1 deletion catalogs/catalog-common/src/main/java/org/apache/gravitino/storage/GCSProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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 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.


private GCSProperties() {}
}
6 changes: 3 additions & 3 deletions ...rc/test/java/org/apache/gravitino/catalog/hadoop/integration/test/HadoopGCSCatalogIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
package org.apache.gravitino.catalog.hadoop.integration.test;

import static org.apache.gravitino.catalog.hadoop.HadoopCatalogPropertiesMetadata.FILESYSTEM_PROVIDERS;
import static org.apache.gravitino.storage.GCSProperties.GCS_SERVICE_ACCOUNT_JSON_PATH;
import static org.apache.gravitino.storage.GCSProperties.GRAVITINO_GCS_SERVICE_ACCOUNT_FILE;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
Expand Down Expand Up @@ -99,7 +99,7 @@ protected String defaultBaseLocation() {

protected void createCatalog() {
Map<String, String> map = Maps.newHashMap();
map.put(GCS_SERVICE_ACCOUNT_JSON_PATH, SERVICE_ACCOUNT_FILE);
map.put(GRAVITINO_GCS_SERVICE_ACCOUNT_FILE, SERVICE_ACCOUNT_FILE);
map.put(FILESYSTEM_PROVIDERS, "gcs");
metalake.createCatalog(catalogName, Catalog.Type.FILESET, provider, "comment", map);

Expand All @@ -117,7 +117,7 @@ public void testCreateSchemaAndFilesetWithSpecialLocation() {
String ossLocation = String.format("gs://%s", BUCKET_NAME);
Map<String, String> catalogProps = Maps.newHashMap();
catalogProps.put("location", ossLocation);
catalogProps.put(GCS_SERVICE_ACCOUNT_JSON_PATH, SERVICE_ACCOUNT_FILE);
catalogProps.put(GRAVITINO_GCS_SERVICE_ACCOUNT_FILE, SERVICE_ACCOUNT_FILE);
catalogProps.put(FILESYSTEM_PROVIDERS, "gcs");

Catalog localCatalog =
Expand Down
2 changes: 1 addition & 1 deletion .../apache/gravitino/filesystem/hadoop/integration/test/GravitinoVirtualFileSystemGCSIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public void startUp() throws Exception {
conf.set("fs.gravitino.client.metalake", metalakeName);

// Pass this configuration to the real file system
conf.set(GCSProperties.GCS_SERVICE_ACCOUNT_JSON_PATH, SERVICE_ACCOUNT_FILE);
conf.set(GCSProperties.GRAVITINO_GCS_SERVICE_ACCOUNT_FILE, SERVICE_ACCOUNT_FILE);
}

@AfterAll
Expand Down
7 changes: 2 additions & 5 deletions core/src/main/java/org/apache/gravitino/credential/config/GCSCredentialConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,18 @@

package org.apache.gravitino.credential.config;

import com.google.common.annotations.VisibleForTesting;
import java.util.Map;
import javax.annotation.Nullable;
import org.apache.gravitino.Config;
import org.apache.gravitino.config.ConfigBuilder;
import org.apache.gravitino.config.ConfigConstants;
import org.apache.gravitino.config.ConfigEntry;
import org.apache.gravitino.storage.GCSProperties;

public class GCSCredentialConfig extends Config {

@VisibleForTesting
public static final String GRAVITINO_GCS_CREDENTIAL_FILE_PATH = "gcs-credential-file-path";

public static final ConfigEntry<String> GCS_CREDENTIAL_FILE_PATH =
new ConfigBuilder(GRAVITINO_GCS_CREDENTIAL_FILE_PATH)
new ConfigBuilder(GCSProperties.GRAVITINO_GCS_SERVICE_ACCOUNT_FILE)
.doc("The path of GCS credential file")
.version(ConfigConstants.VERSION_0_7_0)
.stringConf()
Expand Down
2 changes: 1 addition & 1 deletion dev/docker/iceberg-rest-server/rewrite_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"GRAVITINO_URI" : "uri",
"GRAVITINO_WAREHOUSE" : "warehouse",
"GRAVITINO_CREDENTIAL_PROVIDER_TYPE" : "credential-provider-type",
"GRAVITINO_GCS_CREDENTIAL_FILE_PATH" : "gcs-credential-file-path",
"GRAVITINO_GCS_SERVICE_ACCOUNT_FILE" : "gcs-service-account-file",
"GRAVITINO_S3_ACCESS_KEY" : "s3-access-key-id",
"GRAVITINO_S3_SECRET_KEY" : "s3-secret-access-key",
"GRAVITINO_S3_REGION" : "s3-region",
Expand Down
16 changes: 8 additions & 8 deletions docs/how-to-use-gvfs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,20 +71,20 @@ Apart from the above properties, to access fileset like S3, GCS, OSS and custom

#### S3 fileset

| Configuration item | Description | Default value | Required | Since version |
|--------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|--------------------------|------------------|
| `s3-endpoint` | The endpoint of the AWS S3. | (none) | Yes if it's a S3 fileset.| 0.7.0-incubating |
| `s3-access-key-id` | The access key of the AWS S3. | (none) | Yes if it's a S3 fileset.| 0.7.0-incubating |
| `s3-secret-access-key` | The secret key of the AWS S3. | (none) | Yes if it's a S3 fileset.| 0.7.0-incubating |
| Configuration item | Description | Default value | Required | Since version |
|------------------------|-------------------------------|---------------|---------------------------|------------------|
| `s3-endpoint` | The endpoint of the AWS S3. | (none) | Yes if it's a S3 fileset. | 0.7.0-incubating |
| `s3-access-key-id` | The access key of the AWS S3. | (none) | Yes if it's a S3 fileset. | 0.7.0-incubating |
| `s3-secret-access-key` | The secret key of the AWS S3. | (none) | Yes if it's a S3 fileset. | 0.7.0-incubating |

At the same time, you need to place the corresponding bundle jar [`gravitino-aws-bundle-${version}.jar`](https://repo1.maven.org/maven2/org/apache/gravitino/aws-bundle/) in the Hadoop environment(typically located in `${HADOOP_HOME}/share/hadoop/common/lib/`).


#### GCS fileset

| Configuration item | Description | Default value | Required | Since version |
|--------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|---------------------------|------------------|
| `gcs-service-account-file` | The path of GCS service account JSON file. | (none) | Yes if it's a GCS fileset.| 0.7.0-incubating |
| Configuration item | Description | Default value | Required | Since version |
|----------------------------|--------------------------------------------|---------------|----------------------------|------------------|
| `gcs-service-account-file` | The path of GCS service account JSON file. | (none) | Yes if it's a GCS fileset. | 0.7.0-incubating |

In the meantime, you need to place the corresponding bundle jar [`gravitino-gcp-bundle-${version}.jar`](https://repo1.maven.org/maven2/org/apache/gravitino/gcp-bundle/) in the Hadoop environment(typically located in `${HADOOP_HOME}/share/hadoop/common/lib/`).

Expand Down
39 changes: 20 additions & 19 deletions docs/iceberg-rest-service.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,8 @@ Supports using static GCS credential file or generating GCS token to access GCS
|---------------------------------------------------|----------------------------------------------------------------------------------------------------|---------------|----------|------------------|
| `gravitino.iceberg-rest.io-impl` | The io implementation for `FileIO` in Iceberg, use `org.apache.iceberg.gcp.gcs.GCSFileIO` for GCS. | (none) | No | 0.6.0-incubating |
| `gravitino.iceberg-rest.credential-provider-type` | Supports `gcs-token`, generates a temporary token according to the query data path. | (none) | No | 0.7.0-incubating |
| `gravitino.iceberg-rest.gcs-credential-file-path` | The location of GCS credential file, only used when `credential-provider-type` is `gcs-token`. | (none) | No | 0.7.0-incubating |
| `gravitino.iceberg-rest.gcs-credential-file-path` | Deprecated, please use `gravitino.iceberg-rest.gcs-service-account-file` instead. | (none) | No | 0.7.0-incubating |
| `gravitino.iceberg-rest.gcs-service-account-file` | The location of GCS credential file, only used when `credential-provider-type` is `gcs-token`. | (none) | No | 0.8.0-incubating |

For other Iceberg GCS properties not managed by Gravitino like `gcs.project-id`, you could config it directly by `gravitino.iceberg-rest.gcs.project-id`.

Expand Down Expand Up @@ -441,24 +442,24 @@ docker run -d -p 9001:9001 apache/gravitino-iceberg-rest:0.7.0-incubating

Gravitino Iceberg REST server in docker image could access local storage by default, you could set the following environment variables if the storage is cloud/remote storage like S3, please refer to [storage section](#storage) for more details.

| Environment variables | Configuration items | Since version |
|-----------------------------------------|-----------------------------------------------------|-------------------|
| `GRAVITINO_IO_IMPL` | `gravitino.iceberg-rest.io-impl` | 0.7.0-incubating |
| `GRAVITINO_URI` | `gravitino.iceberg-rest.uri` | 0.7.0-incubating |
| `GRAVITINO_WAREHOUSE` | `gravitino.iceberg-rest.warehouse` | 0.7.0-incubating |
| `GRAVITINO_CREDENTIAL_PROVIDER_TYPE` | `gravitino.iceberg-rest.credential-provider-type` | 0.7.0-incubating |
| `GRAVITINO_GCS_CREDENTIAL_FILE_PATH` | `gravitino.iceberg-rest.gcs-credential-file-path` | 0.7.0-incubating |
| `GRAVITINO_S3_ACCESS_KEY` | `gravitino.iceberg-rest.s3-access-key-id` | 0.7.0-incubating |
| `GRAVITINO_S3_SECRET_KEY` | `gravitino.iceberg-rest.s3-secret-access-key` | 0.7.0-incubating |
| `GRAVITINO_S3_REGION` | `gravitino.iceberg-rest.s3-region` | 0.7.0-incubating |
| `GRAVITINO_S3_ROLE_ARN` | `gravitino.iceberg-rest.s3-role-arn` | 0.7.0-incubating |
| `GRAVITINO_S3_EXTERNAL_ID` | `gravitino.iceberg-rest.s3-external-id` | 0.7.0-incubating |
| `GRAVITINO_S3_TOKEN_SERVICE_ENDPOINT` | `gravitino.iceberg-rest.s3-token-service-endpoint` | 0.8.0-incubating |
| `GRAVITINO_AZURE_STORAGE_ACCOUNT_NAME` | `gravitino.iceberg-rest.azure-storage-account-name` | 0.8.0-incubating |
| `GRAVITINO_AZURE_STORAGE_ACCOUNT_KEY` | `gravitino.iceberg-rest.azure-storage-account-key` | 0.8.0-incubating |
| `GRAVITINO_AZURE_TENANT_ID` | `gravitino.iceberg-rest.azure-tenant-id` | 0.8.0-incubating |
| `GRAVITINO_AZURE_CLIENT_ID` | `gravitino.iceberg-rest.azure-client-id` | 0.8.0-incubating |
| `GRAVITINO_AZURE_CLIENT_SECRET` | `gravitino.iceberg-rest.azure-client-secret` | 0.8.0-incubating |
| Environment variables | Configuration items | Since version |
|----------------------------------------|-----------------------------------------------------|------------------|
| `GRAVITINO_IO_IMPL` | `gravitino.iceberg-rest.io-impl` | 0.7.0-incubating |
| `GRAVITINO_URI` | `gravitino.iceberg-rest.uri` | 0.7.0-incubating |
| `GRAVITINO_WAREHOUSE` | `gravitino.iceberg-rest.warehouse` | 0.7.0-incubating |
| `GRAVITINO_CREDENTIAL_PROVIDER_TYPE` | `gravitino.iceberg-rest.credential-provider-type` | 0.7.0-incubating |
| `GRAVITINO_GCS_SERVICE_ACCOUNT_FILE` | `gravitino.iceberg-rest.gcs-service-account-file` | 0.8.0-incubating |
| `GRAVITINO_S3_ACCESS_KEY` | `gravitino.iceberg-rest.s3-access-key-id` | 0.7.0-incubating |
| `GRAVITINO_S3_SECRET_KEY` | `gravitino.iceberg-rest.s3-secret-access-key` | 0.7.0-incubating |
| `GRAVITINO_S3_REGION` | `gravitino.iceberg-rest.s3-region` | 0.7.0-incubating |
| `GRAVITINO_S3_ROLE_ARN` | `gravitino.iceberg-rest.s3-role-arn` | 0.7.0-incubating |
| `GRAVITINO_S3_EXTERNAL_ID` | `gravitino.iceberg-rest.s3-external-id` | 0.7.0-incubating |
| `GRAVITINO_S3_TOKEN_SERVICE_ENDPOINT` | `gravitino.iceberg-rest.s3-token-service-endpoint` | 0.8.0-incubating |
| `GRAVITINO_AZURE_STORAGE_ACCOUNT_NAME` | `gravitino.iceberg-rest.azure-storage-account-name` | 0.8.0-incubating |
| `GRAVITINO_AZURE_STORAGE_ACCOUNT_KEY` | `gravitino.iceberg-rest.azure-storage-account-key` | 0.8.0-incubating |
| `GRAVITINO_AZURE_TENANT_ID` | `gravitino.iceberg-rest.azure-tenant-id` | 0.8.0-incubating |
| `GRAVITINO_AZURE_CLIENT_ID` | `gravitino.iceberg-rest.azure-client-id` | 0.8.0-incubating |
| `GRAVITINO_AZURE_CLIENT_SECRET` | `gravitino.iceberg-rest.azure-client-secret` | 0.8.0-incubating |

Or build it manually to add custom configuration or logics:

Expand Down
5 changes: 2 additions & 3 deletions ...-server/src/test/java/org/apache/gravitino/iceberg/integration/test/IcebergRESTGCSIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@
import java.util.Map;
import org.apache.gravitino.catalog.lakehouse.iceberg.IcebergConstants;
import org.apache.gravitino.credential.CredentialConstants;
import org.apache.gravitino.credential.config.GCSCredentialConfig;
import org.apache.gravitino.iceberg.common.IcebergConfig;
import org.apache.gravitino.integration.test.util.BaseIT;
import org.apache.gravitino.integration.test.util.DownloaderUtils;
import org.apache.gravitino.integration.test.util.ITUtils;
import org.apache.gravitino.storage.GCSProperties;
import org.junit.jupiter.api.condition.EnabledIfEnvironmentVariable;

// You should export GRAVITINO_GCS_BUCKET and GOOGLE_APPLICATION_CREDENTIALS to run the test
Expand Down Expand Up @@ -75,8 +75,7 @@ private Map<String, String> getGCSConfig() {
IcebergConfig.ICEBERG_CONFIG_PREFIX + CredentialConstants.CREDENTIAL_PROVIDER_TYPE,
CredentialConstants.GCS_TOKEN_CREDENTIAL_PROVIDER_TYPE);
configMap.put(
IcebergConfig.ICEBERG_CONFIG_PREFIX
+ GCSCredentialConfig.GRAVITINO_GCS_CREDENTIAL_FILE_PATH,
IcebergConfig.ICEBERG_CONFIG_PREFIX + GCSProperties.GRAVITINO_GCS_SERVICE_ACCOUNT_FILE,
gcsCredentialPath);
configMap.put(
IcebergConfig.ICEBERG_CONFIG_PREFIX + IcebergConstants.IO_IMPL,
Expand Down
Loading