roryqi commented on code in PR #11294:
URL: https://github.com/apache/gravitino/pull/11294#discussion_r3332853682
##########
common/src/main/java/org/apache/gravitino/credential/CredentialPropertyUtils.java:
##########
@@ -82,24 +94,28 @@ public class CredentialPropertyUtils {
* @return a map of Iceberg properties derived from the credential
*/
public static Map<String, String> toIcebergProperties(Credential credential)
{
- if (credential instanceof S3TokenCredential
- || credential instanceof S3SecretKeyCredential
- || credential instanceof OSSTokenCredential
- || credential instanceof OSSSecretKeyCredential
- || credential instanceof AzureAccountKeyCredential
- || credential instanceof AwsIrsaCredential) {
- return transformProperties(credential.credentialInfo(),
icebergCredentialPropertyMap);
- }
-
- if (credential instanceof GCSTokenCredential) {
+ if (credential instanceof S3TokenCredential || credential instanceof
AwsIrsaCredential) {
+ Map<String, String> icebergProperties =
+ new HashMap<>(
+ transformProperties(credential.credentialInfo(),
icebergCredentialPropertyMap));
+ icebergProperties.put(
Review Comment:
Why not put this into the method `transformProperties`?
##########
common/src/main/java/org/apache/gravitino/credential/CredentialPropertyUtils.java:
##########
@@ -82,24 +94,28 @@ public class CredentialPropertyUtils {
* @return a map of Iceberg properties derived from the credential
*/
public static Map<String, String> toIcebergProperties(Credential credential)
{
- if (credential instanceof S3TokenCredential
- || credential instanceof S3SecretKeyCredential
- || credential instanceof OSSTokenCredential
- || credential instanceof OSSSecretKeyCredential
- || credential instanceof AzureAccountKeyCredential
- || credential instanceof AwsIrsaCredential) {
- return transformProperties(credential.credentialInfo(),
icebergCredentialPropertyMap);
- }
-
- if (credential instanceof GCSTokenCredential) {
+ if (credential instanceof S3TokenCredential || credential instanceof
AwsIrsaCredential) {
+ Map<String, String> icebergProperties =
+ new HashMap<>(
+ transformProperties(credential.credentialInfo(),
icebergCredentialPropertyMap));
+ icebergProperties.put(
+ ICEBERG_S3_TOKEN_EXPIRES_AT_MS,
String.valueOf(credential.expireTimeInMs()));
+ return icebergProperties;
+ } else if (credential instanceof OSSTokenCredential) {
+ Map<String, String> icebergProperties =
+ new HashMap<>(
+ transformProperties(credential.credentialInfo(),
icebergCredentialPropertyMap));
+ icebergProperties.put(
+ ICEBERG_OSS_SECURITY_TOKEN_EXPIRES_AT_MS,
String.valueOf(credential.expireTimeInMs()));
+ return icebergProperties;
+ } else if (credential instanceof GCSTokenCredential) {
Map<String, String> icebergGCSCredentialProperties =
- transformProperties(credential.credentialInfo(),
icebergCredentialPropertyMap);
+ new HashMap<>(
+ transformProperties(credential.credentialInfo(),
icebergCredentialPropertyMap));
icebergGCSCredentialProperties.put(
- GCS_OAUTH_2_TOKEN_EXPIRES_AT,
String.valueOf(credential.expireTimeInMs()));
+ ICEBERG_GCS_TOKEN_EXPIRES_AT,
String.valueOf(credential.expireTimeInMs()));
Review Comment:
Why do we change this?
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java:
##########
@@ -83,6 +102,63 @@ public String getValue() {
private IcebergRESTUtils() {}
+ /**
+ * Builds an Iceberg REST {@link
org.apache.iceberg.rest.credentials.Credential} for load-table,
+ * scan-plan, or credentials API responses.
+ *
+ * @param catalogName IRC catalog name used in the refresh path
+ * @param tableIdentifier table receiving the credential
+ * @param credential Gravitino credential to vend
+ * @param tableMetadata table metadata used to derive the storage prefix
+ * @return Iceberg REST credential with prefix and config
+ */
+ public static org.apache.iceberg.rest.credentials.Credential
toRestCredential(
+ String catalogName,
+ TableIdentifier tableIdentifier,
+ Credential credential,
+ TableMetadata tableMetadata) {
+ Map<String, String> config =
+ new HashMap<>(CredentialPropertyUtils.toIcebergProperties(credential));
+
+ String refreshProperty = null;
+ if (credential instanceof GCSTokenCredential) {
+ refreshProperty = GCS_OAUTH2_REFRESH_CREDENTIALS_ENDPOINT;
+ } else if (credential instanceof ADLSTokenCredential) {
+ refreshProperty = ADLS_REFRESH_CREDENTIALS_ENDPOINT;
+ } else if (credential instanceof S3TokenCredential
+ || credential instanceof AwsIrsaCredential
+ || credential instanceof OSSTokenCredential) {
+ refreshProperty = CLIENT_REFRESH_CREDENTIALS_ENDPOINT;
+ }
+
+ if (refreshProperty != null) {
+ config.put(
+ refreshProperty,
+ String.format(
+ "v1/%s/namespaces/%s/tables/%s/credentials",
+ RESTUtil.encodeString(catalogName),
+ RESTUtil.encodeNamespace(
+ tableIdentifier.namespace(),
NAMESPACE_SEPARATOR_URLENCODED_UTF_8),
+ RESTUtil.encodeString(tableIdentifier.name())));
+ }
+
+ String prefix = Strings.CS.appendIfMissing(tableMetadata.location(), "/");
Review Comment:
Could u avoid using Strings.CS.?
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java:
##########
@@ -83,6 +102,63 @@ public String getValue() {
private IcebergRESTUtils() {}
+ /**
+ * Builds an Iceberg REST {@link
org.apache.iceberg.rest.credentials.Credential} for load-table,
+ * scan-plan, or credentials API responses.
+ *
+ * @param catalogName IRC catalog name used in the refresh path
+ * @param tableIdentifier table receiving the credential
+ * @param credential Gravitino credential to vend
+ * @param tableMetadata table metadata used to derive the storage prefix
+ * @return Iceberg REST credential with prefix and config
+ */
+ public static org.apache.iceberg.rest.credentials.Credential
toRestCredential(
+ String catalogName,
+ TableIdentifier tableIdentifier,
+ Credential credential,
+ TableMetadata tableMetadata) {
+ Map<String, String> config =
+ new HashMap<>(CredentialPropertyUtils.toIcebergProperties(credential));
+
+ String refreshProperty = null;
Review Comment:
refreshEndpointProp.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java:
##########
@@ -83,6 +102,63 @@ public String getValue() {
private IcebergRESTUtils() {}
+ /**
+ * Builds an Iceberg REST {@link
org.apache.iceberg.rest.credentials.Credential} for load-table,
+ * scan-plan, or credentials API responses.
+ *
+ * @param catalogName IRC catalog name used in the refresh path
+ * @param tableIdentifier table receiving the credential
+ * @param credential Gravitino credential to vend
+ * @param tableMetadata table metadata used to derive the storage prefix
+ * @return Iceberg REST credential with prefix and config
+ */
+ public static org.apache.iceberg.rest.credentials.Credential
toRestCredential(
Review Comment:
Rest -> REST.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
