[Suggestion] Shall we follow the applyMutation pattern here

.applyMutation(awsProperties:applyAwsRegionConfiguration)

and have the following method in AwsProperties.java

public <T extends AwsSyncClientBuilder> void applyAwsRegionConfiguration(T builder) {
   if (awsRegion != null) {
       builder.region(Region.of(awsRegion))
  }
}

In this way, we do not need to do any further initialization in the DefaultAwsClientFactories

What do you think about this change?

This is good suggestion @JonasJ-ap . I made the change. Please check.

Thank you for refactoring this.

Needs to note that this property expects the full classname of the provider and it would probably help to say that the class must implement "........" here as well.

Updated javadoc @RussellSpitzer

Shouldn't we check if this provider is loaded? Usually for things like this I just make a fake class which throws an exception when initialized. That we we can tell the constructor was called correctly.

@RussellSpitzer Added Class.forName to check class exists in the classpath. Please check. I could not find any existing API in iceberg project like Spark's Utils.classIsLoadable API

+1 for making a fake class for this. See examples like https://github.com/apache/iceberg/blob/master/core/src/test/java/org/apache/iceberg/TestLocationProvider.java#L185

And do we need Class.forName? I think it will naturally throw ClassNotFound in DynMethods

Jack has it right, I'm not worried this class does not exist. But I want the test to prove that it successfully loaded the class.

Using a fake class lets us know with out a doubt that the class was loaded and used as the credential provider because we can put traps in it.

Added check to make sure provider implements AwsCredentialsProvider interface.

@jackye1995 It looks like DynMethods does not throw ClassNotFoundException but DynClasses does. I replaced Class.forName with DynClasses.

The point is not to test whether the CNFE is thrown, but to make sure the test code is dynamically loading the class specified. We wouldn't have had a way of distinguishing between SystemPropertiesCredentialProvider being loaded and any other provider being loaded. The new tests added are better in that they check (in the error message) that the class we wanted was actually loaded.

Specifically this test just makes sure AwsClientFactories.from(properties) doesn't fail. It doesn't check that SystemProperitesCredentialsProvider is the credential provider

can we do it for all clients, instead of just S3FileIO? so variable below should also be clientCredentialsProvider instead of s3CredentialsProvider

Changed as suggested.

similar comment for variable name, should be clientRegion instead of awsRegion, and CLIENT_CLIENT_REGION instead of AWS_CLIENT_REGION.

Changed as suggested.

why do we need to trim?

Just added in case if a user makes a config mistake by adding space. Removed now.

you mentioned

Most of the AWS SDK Credential Providers are using create() method instead of constructor

any reference for that?

Because at least for the Trino case it's done by checking constructors. In Iceberg most uses initialize(...) instead of create(...).

I guess there is one case for create which is

public static S3V4RestSignerClient create(Map<String, String> properties) {
    return ImmutableS3V4RestSignerClient.builder().properties(properties).build();
  }

in that case I am fine with this. But I think we should still support only 1 method for create(Map) instead of an additional create() without argument.

In this case I would probably first look for a "map" create method and pass an empty map if we have no properties. If we don't have no-arg create method we can fall back to that if the first method doesn't exist. I think your filter code above guarantees that properties is non-null but we can check that.

@jackye1995 knows more about what credential providers there are, but I'm surprised that having a constructor is that rare. Anyway I definitely think we should always call a "create(map) if that exists"

try {
  getCreate(Map)
  invokeMapCreate(properties)
} catch (NoSuchMethod) {
  try {
     getCreate()
     invokeCreate()
   }
}

Thanks @RussellSpitzer . I am fine with your approach. Once @jackye1995 approves, I can add it.

hi @jackye1995 These are some of the AWS SDK provided credentials provider classes

software.amazon.awssdk.auth.credentials.AnonymousCredentialsProvider
software.amazon.awssdk.auth.credentials.EnvironmentVariableCredentialsProvider
software.amazon.awssdk.auth.credentials.SystemPropertyCredentialsProvider
software.amazon.awssdk.auth.credentials.WebIdentityTokenFileCredentialsProvider

AWS SDK V2 Apis not using constructors in most of the cases.

@RussellSpitzer I made change as you suggested by checking create(Map) first and fallback to create().

replaced with Strings.isNullOrEmpty().

I would probably not call this "Clazz", we use Class everywhere else

corrected.

Any reason why we are specifying the classloader here?

Removed ClassLoader. By default DynClasses api uses context class loader

This is the wrong exception type, we throw these only for table validation errors.

/** 
 * Exception raised when validation checks fail.
 * <p>
 * For example, this is thrown when attempting to create a table with a {@link PartitionSpec} that
 * is not compatible with the table {@link Schema}
 */

I think this is probably better as a Preconditions.checkArg

Replaced ValidationException.check with Preconditions. I also noticed that AwsProperties class still has ValidationException.check API call

https://github.com/apache/iceberg/blob/master/aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java#L777

Error message format in iceberg is
"Cannot X because Y. (Remediation)"

Also in this I would probably embed the class name as a variable in the format string. "AwsCredentialsProvider.class.getName"

Fixed all exception messages as recommended. thanks

This has a a version (String className, classes....)

So you don't actually need to get the class above

@RussellSpitzer DynMethods does not throw CNFE exception if a class does not exists. here is the code.

    public Builder impl(String className, String methodName, Class<?>... argClasses) {
      // don't do any work if an implementation has been found
      if (method != null) {
        return this;
      }

      try {
        Class<?> targetClass = Class.forName(className, true, loader);
        impl(targetClass, methodName, argClasses);
      } catch (ClassNotFoundException e) {
        // not the right implementation
      }
      return this;
    }

By calling DynClasses, I am able to validate CNFE and also adding Provider interface is implemented or not.