rdblue commented on a change in pull request #1618:
URL: https://github.com/apache/iceberg/pull/1618#discussion_r505969491
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -21,57 +21,43 @@
import java.util.Map;
import org.apache.hadoop.fs.Path;
-import org.apache.iceberg.common.DynConstructors;
import org.apache.iceberg.io.LocationProvider;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.transforms.Transform;
import org.apache.iceberg.transforms.Transforms;
import org.apache.iceberg.types.Types;
+import org.apache.iceberg.util.ClassLoaderUtil;
import org.apache.iceberg.util.PropertyUtil;
import static org.apache.iceberg.TableProperties.OBJECT_STORE_PATH;
public class LocationProviders {
+ private static final Class<?>[] CONSTRUCTOR_ARG_TYPES = { String.class,
Map.class };
+
private LocationProviders() {
}
public static LocationProvider locationsFor(String location, Map<String,
String> properties) {
- if (properties.containsKey(TableProperties.WRITE_LOCATION_PROVIDER_IMPL)) {
- String impl =
properties.get(TableProperties.WRITE_LOCATION_PROVIDER_IMPL);
- DynConstructors.Ctor<LocationProvider> ctor;
- try {
- ctor = DynConstructors.builder(LocationProvider.class)
- .impl(impl, String.class, Map.class)
- .impl(impl).buildChecked(); // fall back to no-arg constructor
- } catch (NoSuchMethodException e) {
- throw new IllegalArgumentException(String.format(
- "Unable to find a constructor for implementation %s of %s. " +
- "Make sure the implementation is in classpath, and that it
either " +
- "has a public no-arg constructor or a two-arg constructor " +
- "taking in the string base table location and its property
string map.",
- impl, LocationProvider.class), e);
- }
- try {
- return ctor.newInstance(location, properties);
- } catch (ClassCastException e) {
- throw new IllegalArgumentException(
- String.format("Provided implementation for dynamic instantiation
should implement %s.",
- LocationProvider.class), e);
- }
- } else if (PropertyUtil.propertyAsBoolean(properties,
+ // for backwards compatibility of write.object-storage.enabled property
+ Class<?> defaultProviderClass = PropertyUtil.propertyAsBoolean(properties,
TableProperties.OBJECT_STORE_ENABLED,
- TableProperties.OBJECT_STORE_ENABLED_DEFAULT)) {
- return new ObjectStoreLocationProvider(location, properties);
- } else {
- return new DefaultLocationProvider(location, properties);
- }
+ TableProperties.OBJECT_STORE_ENABLED_DEFAULT) ?
+ ObjectStoreLocationProvider.class : DefaultLocationProvider.class;
+
+ return ClassLoaderUtil.fromProperty(
+ properties,
+ TableProperties.WRITE_LOCATION_PROVIDER_IMPL,
+ defaultProviderClass.getName(),
+ LocationProvider.class,
+ CONSTRUCTOR_ARG_TYPES,
+ new Object[]{location, properties});
}
- static class DefaultLocationProvider implements LocationProvider {
+ public static class DefaultLocationProvider implements LocationProvider {
Review comment:
There's no need to make this public, and I'd like to avoid exposing new
classes through the public API. If we need to use reflection, we can always use
`hiddenImpl` to set the accessible flag. But I think it would be better to
instantiate these classes directly like before so that we have compile checks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
