-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HUDI-7587] Make hudi-hadoop-common module dependent on hudi-common module #11131
[HUDI-7587] Make hudi-hadoop-common module dependent on hudi-common module #11131
Conversation
70e6f70
to
190884f
Compare
…Utils#getFSDataInputStream part
hudi-common/src/main/java/org/apache/hudi/common/table/log/block/HoodieHFileDataBlock.java
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/common/util/OrcUtils.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/io/storage/HoodieAvroFileWriterFactory.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/io/storage/HoodieAvroFileWriterFactory.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/io/storage/HoodieAvroFileWriterFactory.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/io/storage/HoodieNativeAvroHFileReader.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/storage/HoodieStorageUtils.java
Outdated
Show resolved
Hide resolved
hudi-common/src/test/java/org/apache/hudi/common/testutils/HoodieTestDataGenerator.java
Show resolved
Hide resolved
hudi-common/src/test/java/org/apache/hudi/common/testutils/HoodieTestUtils.java
Outdated
Show resolved
Hide resolved
hudi-common/src/test/java/org/apache/hudi/common/testutils/HoodieTestUtils.java
Outdated
Show resolved
Hide resolved
hudi-common/src/test/java/org/apache/hudi/common/testutils/reader/HoodieFileSliceTestUtils.java
Outdated
Show resolved
Hide resolved
hudi-hadoop-common/src/main/java/org/apache/hudi/common/config/DFSPropertiesConfiguration.java
Show resolved
Hide resolved
hudi-hadoop-common/src/main/java/org/apache/hudi/hadoop/fs/HadoopFSUtils.java
Outdated
Show resolved
Hide resolved
@@ -109,25 +105,6 @@ public static FileSystem getFs(String pathStr, Configuration conf, boolean local | |||
return getFs(pathStr, conf); | |||
} | |||
|
|||
public static HoodieStorage getStorageWithWrapperFS(StoragePath path, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this moved to somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, since we need to use reflection, I moved it into a new constructor for HoodieHadoopStorage:
public HoodieHadoopStorage(StoragePath path,
StorageConfiguration<?> conf,
boolean enableRetry,
long maxRetryIntervalMs,
int maxRetryNumbers,
long initialRetryIntervalMs,
String retryExceptions,
ConsistencyGuard consistencyGuard) {
FileSystem fileSystem = getFs(path, conf.unwrapCopyAs(Configuration.class));
if (enableRetry) {
fileSystem = new HoodieRetryWrapperFileSystem(fileSystem,
maxRetryIntervalMs, maxRetryNumbers, initialRetryIntervalMs, retryExceptions);
}
checkArgument(!(fileSystem instanceof HoodieWrapperFileSystem),
"File System not expected to be that of HoodieWrapperFileSystem");
this.fs = new HoodieWrapperFileSystem(fileSystem, consistencyGuard);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yihua to respond later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we have to add a new constructor due to the usage of reflection. We'll simplify this later.
hudi-hadoop-common/src/main/java/org/apache/hudi/hadoop/fs/HoodieHadoopUtils.java
Outdated
Show resolved
Hide resolved
hudi-hadoop-common/src/main/java/org/apache/hudi/storage/hadoop/HadoopStorageConfiguration.java
Show resolved
Hide resolved
hudi-hadoop-common/src/main/java/org/apache/hudi/storage/hadoop/HoodieHadoopStorage.java
Outdated
Show resolved
Hide resolved
hudi-hadoop-common/src/main/java/org/apache/hudi/storage/hadoop/HoodieHadoopStorage.java
Show resolved
Hide resolved
hudi-hadoop-common/src/test/java/org/apache/hudi/common/functional/TestHoodieLogFormat.java
Outdated
Show resolved
Hide resolved
hudi-hadoop-common/src/test/java/org/apache/hudi/common/table/TestTableSchemaResolver.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/storage/HoodieStorageUtils.java
Outdated
Show resolved
Hide resolved
hudi-hadoop-common/src/test/java/org/apache/hudi/common/table/TestHoodieTableConfig.java
Outdated
Show resolved
Hide resolved
...p-common/src/test/java/org/apache/hudi/common/table/timeline/TestWaitBasedTimeGenerator.java
Outdated
Show resolved
Hide resolved
hudi-hadoop-common/src/test/java/org/apache/hudi/common/testutils/FileSystemTestUtils.java
Show resolved
Hide resolved
<artifactId>hudi-hadoop-common</artifactId> | ||
<version>${project.version}</version> | ||
<classifier>tests</classifier> | ||
<type>test-jar</type> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is test-jar
type required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDK, I just copied from what the other poms did
hudi-io/src/main/java/org/apache/hudi/storage/HoodieStorage.java
Outdated
Show resolved
Hide resolved
...he/spark/sql/execution/datasources/parquet/HoodieFileGroupReaderBasedParquetFileFormat.scala
Outdated
Show resolved
Hide resolved
...he/spark/sql/execution/datasources/parquet/HoodieFileGroupReaderBasedParquetFileFormat.scala
Outdated
Show resolved
Hide resolved
...park/src/test/scala/org/apache/hudi/common/table/read/TestHoodieFileGroupReaderOnSpark.scala
Outdated
Show resolved
Hide resolved
...tasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestMORDataSourceStorage.scala
Outdated
Show resolved
Hide resolved
hudi-common/src/test/java/org/apache/hudi/common/testutils/HoodieTestUtils.java
Outdated
Show resolved
Hide resolved
hudi-hadoop-common/src/main/java/org/apache/hudi/storage/hadoop/HoodieHadoopStorage.java
Show resolved
Hide resolved
...p-common/src/test/java/org/apache/hudi/common/table/timeline/TestWaitBasedTimeGenerator.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/common/config/PropertiesConfig.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/common/util/BaseFileUtils.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/io/storage/HoodieAvroFileWriterFactory.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/storage/HoodieStorageUtils.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM! Could you rename the PR as "Make hudi-hadoop-common
module dependent on hudi-common
" to be clear?
Once you address all the nits, you can merge the PR.
hudi-common/src/main/java/org/apache/hudi/storage/HoodieStorageUtils.java
Outdated
Show resolved
Hide resolved
…odule (#11131) Co-authored-by: Jonathan Vexler <=>
…odule (#11131) Co-authored-by: Jonathan Vexler <=>
Change Logs
make hudi-hadoop-common depend on hudi-common instead of the reverse. This is the next step for removing hadoop from hudi-common. There is hardcoded reflection currently that will be mostly removed or will be configurable for the other filesystems.
Took some methods out of FileSystemTestUtils and moved to HoodieTestTable because they are only needed in hadoop tests, and are mostly only used by that single test class
Impact
make hudi-hadoop-common depend on hudi-common instead of the reverse
Risk level (write none, low medium or high below)
medium
Documentation Update
N/A
Contributor's checklist