alexeykudinkin commented on code in PR #6740:
URL: https://github.com/apache/hudi/pull/6740#discussion_r980606386
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieMergeHandle.java:
##########
@@ -210,18 +203,6 @@ private void init(String fileId, String partitionPath,
HoodieBaseFile baseFileTo
// Create the writer for writing the new version file
fileWriter = createNewFileWriter(instantTime, newFilePath, hoodieTable,
config,
writeSchemaWithMetaFields, taskContextSupplier);
-
- // init the cdc logger
- this.cdcEnabled =
config.getBooleanOrDefault(HoodieTableConfig.CDC_ENABLED);
Review Comment:
@danny0405 appreciate your viewpoint regarding separation of concerns in
regard to CDC logging, but i don't think this change make sense to me:
- Instead of having CDC logging centralized in one place we now have it
spread across every class that is extending `HoodieMergeHandle` class
(`HoodieMergeHandleWithChangelog`, `HoodieSortedMergeHandleWithChangelog`). As
i called out on the original PR this is not scalable approach -- we can't
introduce sub-classes for every feature we introduce and we need to balance
this consideration against other factors such as including, for ex, how many
features we have to support (for N features if we do subclassing we will have
to do up ton 2^N subclasses). Not long ago [i had to inline sub-classes for
WriteHandle](https://github.com/apache/hudi/pull/5470/files#diff-ba11482e3356cce0c6aeed5ba4106e3341b83d347e1c1876caf7c0f13fab52c3)
impls for exactly that reason -- w/o it with this new feature we would have to
essentially double the number of classes.
- Previously, CDC logic was implemented w/in a single class, this impl now
introduces 5 new classes to achieve the same thing
--
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]
