[GitHub] [hudi] yihua commented on a diff in pull request #8441: Upgrade aws java sdk to v2

[via GitHub]

yihua commented on code in PR #8441:
URL: https://github.com/apache/hudi/pull/8441#discussion_r1283850979


##
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:
   Got it.  Then we can keep it this way.



-- 
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: commits-unsubscr...@hudi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hudi] yihua commented on a diff in pull request #8441: Upgrade aws java sdk to v2

[via GitHub]

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