yihua commented on code in PR #8441:
URL: https://github.com/apache/hudi/pull/8441#discussion_r1164508712
##
hudi-aws/pom.xml:
##
@@ -77,9 +77,8 @@
com.amazonaws
dynamodb-lock-client
-${dynamodb.lockclient.version}
+1.2.0
Review Comment:
nit: could you keep the version variable?
##
hudi-aws/src/main/java/org/apache/hudi/aws/credentials/HoodieAWSCredentialsProviderFactory.java:
##
@@ -30,16 +30,23 @@
* Factory class for Hoodie AWSCredentialsProvider.
*/
public class HoodieAWSCredentialsProviderFactory {
- public static AWSCredentialsProvider getAwsCredentialsProvider(Properties
props) {
+ public static AwsCredentialsProvider getAwsCredentialsProvider(Properties
props) {
return getAwsCredentialsProviderChain(props);
}
- private static AWSCredentialsProvider
getAwsCredentialsProviderChain(Properties props) {
-List providers = new ArrayList<>();
-providers.add(new HoodieConfigAWSCredentialsProvider(props));
-providers.add(new DefaultAWSCredentialsProviderChain());
-AWSCredentialsProviderChain providerChain = new
AWSCredentialsProviderChain(providers);
-providerChain.setReuseLastProvider(true);
+ private static AwsCredentialsProvider
getAwsCredentialsProviderChain(Properties props) {
+List providers = new ArrayList<>();
+HoodieConfigAWSCredentialsProvider hoodieConfigAWSCredentialsProvider =
new HoodieConfigAWSCredentialsProvider(props);
+if (hoodieConfigAWSCredentialsProvider.resolveCredentials() != null) {
+ providers.add(hoodieConfigAWSCredentialsProvider);
+}
+providers.add(DefaultCredentialsProvider.builder()
Review Comment:
Does this provide default chain as before (`new
DefaultAWSCredentialsProviderChain()`) or only a single credentials provider?
##
hudi-aws/src/main/java/org/apache/hudi/aws/sync/AWSGlueCatalogSyncClient.java:
##
@@ -246,22 +248,22 @@ public void updateTableSchema(String tableName,
MessageType newSchema) {
Table table = getTable(awsGlue, databaseName, tableName);
Map newSchemaMap = parquetSchemaToMapSchema(newSchema,
config.getBoolean(HIVE_SUPPORT_TIMESTAMP_TYPE), false);
List newColumns = getColumnsFromSchema(newSchemaMap);
- StorageDescriptor sd = table.getStorageDescriptor();
- sd.setColumns(newColumns);
-
- final Date now = new Date();
- TableInput updatedTableInput = new TableInput()
- .withName(tableName)
- .withTableType(table.getTableType())
- .withParameters(table.getParameters())
- .withPartitionKeys(table.getPartitionKeys())
- .withStorageDescriptor(sd)
- .withLastAccessTime(now)
- .withLastAnalyzedTime(now);
-
- UpdateTableRequest request = new UpdateTableRequest()
- .withDatabaseName(databaseName)
- .withTableInput(updatedTableInput);
+ StorageDescriptor sd = table.storageDescriptor();
+ StorageDescriptor partitionSD = sd.copy(copySd ->
copySd.columns(newColumns));
+ final Instant now = Instant.now();
+ TableInput updatedTableInput = TableInput.builder()
+ .tableType(table.tableType())
Review Comment:
table name is missed?
##
hudi-aws/src/main/java/org/apache/hudi/aws/credentials/HoodieAWSCredentialsProviderFactory.java:
##
@@ -30,16 +30,23 @@
* Factory class for Hoodie AWSCredentialsProvider.
*/
public class HoodieAWSCredentialsProviderFactory {
- public static AWSCredentialsProvider getAwsCredentialsProvider(Properties
props) {
+ public static AwsCredentialsProvider getAwsCredentialsProvider(Properties
props) {
return getAwsCredentialsProviderChain(props);
}
- private static AWSCredentialsProvider
getAwsCredentialsProviderChain(Properties props) {
-List providers = new ArrayList<>();
-providers.add(new HoodieConfigAWSCredentialsProvider(props));
-providers.add(new DefaultAWSCredentialsProviderChain());
-AWSCredentialsProviderChain providerChain = new
AWSCredentialsProviderChain(providers);
-providerChain.setReuseLastProvider(true);
+ private static AwsCredentialsProvider
getAwsCredentialsProviderChain(Properties props) {
+List providers = new ArrayList<>();
+HoodieConfigAWSCredentialsProvider hoodieConfigAWSCredentialsProvider =
new HoodieConfigAWSCredentialsProvider(props);
+if (hoodieConfigAWSCredentialsProvider.resolveCredentials() != null) {
+ providers.add(hoodieConfigAWSCredentialsProvider);
Review Comment:
Previously there is no such check so wondering if the if check is needed.
Could it just use default AWS credentials if `awsCredentials` is null?
##
hudi-aws/src/main/java/org/apache/hudi/aws/sync/AWSGlueCatalogSyncClient.java:
##
@@ -246,22 +248,22 @@ public void updateTableSchema(String tableName,
MessageType newSchema) {
Table table = getTable(awsGlue, databaseName, tableName);
Map newSchem