voonhous commented on code in PR #17785:
URL: https://github.com/apache/hudi/pull/17785#discussion_r3346814188
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/common/table/log/HoodieLogFormatWriter.java:
##########
@@ -207,23 +206,29 @@ private int getLogBlockLength(int contentLength, int
headerLength, int footerLen
private void rolloverIfNeeded() throws IOException {
// Roll over if the size is past the threshold
- if (getCurrentSize() > sizeThreshold) {
- log.info("CurrentSize {} has reached threshold {}. Rolling over to the
next version", getCurrentSize(), sizeThreshold);
+ if (getCurrentSize() > getSizeThreshold()) {
+ log.info("CurrentSize {} has reached threshold {}. Rolling over to the
next version", getCurrentSize(), getSizeThreshold());
rollOver();
}
}
private void rollOver() throws IOException {
closeStream();
- this.logFile = logFile.rollOver(rolloverLogWriteToken);
+ this.logFile = getLogFile().rollOver(getLogWriteToken());
this.closed = false;
}
private void createNewFile() throws IOException {
- fileCreationHook.preFileCreation(this.logFile);
- this.output = new FSDataOutputStream(
- storage.create(this.logFile.getPath(), false, bufferSize, replication,
WriterBuilder.DEFAULT_SIZE_THRESHOLD),
- new FileSystem.Statistics(storage.getScheme())
+ getFileCreationCallback().preFileCreation(this.getLogFile());
+ this.outputStream = new FSDataOutputStream(
+ getStorage().create(
+ this.getLogFile().getPath(),
+ false,
+ getBufferSize(),
+ getReplication(),
Review Comment:
Good catch — confirmed this was a behavioral change. Restored the fixed
`DEFAULT_SIZE_THRESHOLD` (512 MB) as the block-size argument to `create()`,
since the HDFS block size is a separate concept from the log rollover
threshold. Thanks!
##########
hudi-common/src/main/java/org/apache/hudi/common/table/log/HoodieLogFormat.java:
##########
@@ -60,31 +60,140 @@ public interface HoodieLogFormat {
String DEFAULT_WRITE_TOKEN = "0-0-0";
- String DEFAULT_LOG_FORMAT_WRITER =
"org.apache.hudi.common.table.log.HoodieLogFormatWriter";
-
/**
- * Writer interface to allow appending block to this file format.
+ * Abstract base class for appending blocks to the Hoodie log format.
+ * Subclasses provide specific implementations for writing to different
storage layers.
*/
- interface Writer extends Closeable {
+ @Getter
+ @Slf4j
+ abstract class Writer implements Closeable {
+
+ // Default max log file size 512 MB
+ public static final long DEFAULT_SIZE_THRESHOLD = 512 * 1024 * 1024L;
+
+ // Buffer size
+ protected Integer bufferSize;
+ // FileSystem
+ protected HoodieStorage storage;
+ // Size threshold for the log file. Useful when used with a rolling log
appender
+ protected Long sizeThreshold;
+ // Log File extension. Could be .avro.delta or .avro.commits etc
+ protected String fileExtension;
+ // File Id
+ protected String logFileId;
+ // File Commit Time stamp
+ protected String instantTime;
+ // version number for this log file. If not specified, then the current
version will be
+ // computed by inspecting the file system
+ protected Integer logVersion;
+ // file len of this log file
Review Comment:
Done — updated the comment to "file size" and removed the redundant `= 0L`
inline initializer; the default is set in the constructor.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/rollback/RollbackHelperV1.java:
##########
@@ -311,9 +312,9 @@ List<Pair<String, HoodieRollbackStat>>
maybeDeleteAndCollectStats(HoodieEngineCo
// Let's emit markers for rollback as well. markers are emitted
under rollback instant time.
WriteMarkers writeMarkers =
WriteMarkersFactory.get(config.getMarkersType(), table, instantTime);
- HoodieLogFormat.WriterBuilder writerBuilder =
HoodieLogFormat.newWriterBuilder()
-
.onParentPath(FSUtils.constructAbsolutePath(metaClient.getBasePath(),
partitionPath))
- .withFileId(fileId)
+ HoodieLogFormatWriter.HoodieLogFormatWriterBuilder writerBuilder =
HoodieLogFormatWriter.builder()
Review Comment:
Thanks — I follow the concern about the Lombok-generated name, but I'd
prefer to keep it as-is. The builder belongs to `HoodieLogFormatWriter`, which
lives in `hudi-hadoop-common` and references hadoop types
(`FSDataOutputStream`). The old `HoodieLogFormat.WriterBuilder` only lived in
`hudi-common` by loading the writer reflectively via `ReflectionUtils` — the
brittleness this PR removes (#11207). Re-adding a named builder type in
`HoodieLogFormat` would mean either bringing back that reflection or
re-coupling `hudi-common` to hadoop. The generated type is referenced in
exactly one internal call site (`RollbackHelperV1`), where we hold the builder
to conditionally set the pre-computed log version before `build()`, so the leak
is contained to a single internal caller.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
