You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/01/19 10:23:31 UTC
[GitHub] [iceberg] rajarshisarkar opened a new pull request #3923: Core: Add no-arg constructor in ResolvingFileIO
rajarshisarkar opened a new pull request #3923:
URL: https://github.com/apache/iceberg/pull/3923
- Add no-arg constructor in `ResolvingFileIO` (maintaining consistency with `S3FileIO`, `HadoopFileIO`, `GCSFileIO` and `OSSFileIO`).
- Introduced static constant for `org.apache.iceberg.aws.s3.S3FileIO`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3923: Core: Add no-arg constructor in ResolvingFileIO
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3923:
URL: https://github.com/apache/iceberg/pull/3923#discussion_r789210365
##########
File path: core/src/main/java/org/apache/iceberg/io/ResolvingFileIO.java
##########
@@ -40,17 +40,26 @@
private static final Logger LOG = LoggerFactory.getLogger(ResolvingFileIO.class);
private static final String DEFAULT_SCHEME = "fs";
private static final String FALLBACK_IMPL = "org.apache.iceberg.hadoop.HadoopFileIO";
+ private static final String S3_FILE_IO_IMPL = "org.apache.iceberg.aws.s3.S3FileIO";
private static final Map<String, String> SCHEME_TO_FILE_IO = ImmutableMap.of(
DEFAULT_SCHEME, FALLBACK_IMPL,
- "s3", "org.apache.iceberg.aws.s3.S3FileIO",
- "s3a", "org.apache.iceberg.aws.s3.S3FileIO",
- "s3n", "org.apache.iceberg.aws.s3.S3FileIO"
+ "s3", S3_FILE_IO_IMPL,
+ "s3a", S3_FILE_IO_IMPL,
+ "s3n", S3_FILE_IO_IMPL
Review comment:
Nit: This change is good, but we _might_ want to put it in another PR to make it easier for people to cherry-pick only PRs that contain bugs (in my view, the lack of a no-arg constructor in `ResolvingFileIO` is a bug). And then possibly update the title of this PR to make it more clear that this is fixing a bug such as `Core: Add missing no-arg constructor in ResolvingFileIO`.
If other's feel differently, then by all means it can stay in this PR.
##########
File path: core/src/main/java/org/apache/iceberg/io/ResolvingFileIO.java
##########
@@ -40,17 +40,26 @@
private static final Logger LOG = LoggerFactory.getLogger(ResolvingFileIO.class);
private static final String DEFAULT_SCHEME = "fs";
private static final String FALLBACK_IMPL = "org.apache.iceberg.hadoop.HadoopFileIO";
+ private static final String S3_FILE_IO_IMPL = "org.apache.iceberg.aws.s3.S3FileIO";
private static final Map<String, String> SCHEME_TO_FILE_IO = ImmutableMap.of(
DEFAULT_SCHEME, FALLBACK_IMPL,
- "s3", "org.apache.iceberg.aws.s3.S3FileIO",
- "s3a", "org.apache.iceberg.aws.s3.S3FileIO",
- "s3n", "org.apache.iceberg.aws.s3.S3FileIO"
+ "s3", S3_FILE_IO_IMPL,
+ "s3a", S3_FILE_IO_IMPL,
+ "s3n", S3_FILE_IO_IMPL
);
private final Map<String, FileIO> ioInstances = Maps.newHashMap();
private Map<String, String> properties;
private SerializableSupplier<Configuration> hadoopConf;
+ /**
+ * No-arg constructor to load the FileIO dynamically.
+ * <p>
+ * All fields are initialized by calling {@link ResolvingFileIO#initialize(Map)} later.
+ */
+ public ResolvingFileIO() {
+ }
Review comment:
This is consistent with what is in other catalogs (such as `S3FileIO`). This is fine and generally speaking required depending on where it's used. 👍
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3923: Core: Add no-arg constructor in ResolvingFileIO
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3923:
URL: https://github.com/apache/iceberg/pull/3923#issuecomment-1019610280
While I don't think that this is necessary, it does improve the codebase slightly because we won't have to remember to create a no-arg constructor if we ever create a second constructor for this class. I'll merge this. Thanks, @rajarshisarkar!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #3923: Core: Add no-arg constructor in ResolvingFileIO
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #3923:
URL: https://github.com/apache/iceberg/pull/3923
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org