You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "wombatu-kun (via GitHub)" <gi...@apache.org> on 2024/03/23 15:58:09 UTC
[PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
wombatu-kun opened a new pull request, #10917:
URL: https://github.com/apache/hudi/pull/10917
### Change Logs
After refactoring [HUDI-7530](https://issues.apache.org/jira/browse/HUDI-7530) there still remains many places to be cleaned in handleUpdate. I propose to move handleUpdateInternal(), handleUpdate() and getUpdateHandle() to HoodieTable, so we could clean a lot of code duplicates by deleting them from BaseXXXCommitActionExecutors and HodieXXXTables.
### Impact
none
### Risk level (write none, low medium or high below)
none
### Documentation Update
none
- _The config description must be updated if new configs are added or the default value of the configs are changed_
- _Any new feature or user-facing change requires updating the Hudi website. Please create a Jira ticket, attach the
ticket number here and follow the [instruction](https://hudi.apache.org/contribute/developer-setup#website) to make
changes to the website._
### Contributor's checklist
- [ ] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute)
- [ ] Change Logs and Impact were stated clearly
- [ ] Adequate tests were added if applicable
- [ ] CI passed
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #10917:
URL: https://github.com/apache/hudi/pull/10917#discussion_r1536992371
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java:
##########
@@ -1096,11 +1104,37 @@ private Set<String> getDropPartitionColNames() {
return new HashSet<>(Arrays.asList(partitionFields.get()));
}
- public void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ public Iterator<List<WriteStatus>> handleUpdate(String instantTime, String partitionPath, String fileId,
+ Map<String, HoodieRecord<T>> keyToNewRecords, HoodieBaseFile oldDataFile) throws IOException {
+ HoodieMergeHandle upsertHandle = getUpdateHandle(instantTime, partitionPath, fileId, keyToNewRecords, oldDataFile);
+ return updateWithUpsertHandle(upsertHandle, instantTime, fileId);
+ }
+
+ public Iterator<List<WriteStatus>> updateWithUpsertHandle(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ runMerge(upsertHandle, instantTime, fileId);
+ return upsertHandle.getWriteStatusesAsIterator();
+ }
+
+ protected void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
if (upsertHandle.getOldFilePath() == null) {
throw new HoodieUpsertException("Error in finding the old file path at commit " + instantTime + " for fileId: " + fileId);
} else {
HoodieMergeHelper.newInstance().runMerge(this, upsertHandle);
}
}
+
+ protected HoodieMergeHandle getUpdateHandle(String instantTime, String partitionPath, String fileId,
Review Comment:
Yes, this handleUpdate method belongs to HoodieCompactionHandler inteface, but HoodieTable is the only place where this code can be reused for all engine types without duplications.
I don't think it's possible to merge runMerge, updateWithUpsertHandle and handleUpdate into one methods. I tried and had these problems:
- updateWithUpsertHandle is used not only in compaction process, but in CommitActionExecutors too;
- runMerge code is the same for Java and Flink engines, but it is different for Spark, so it must be separated from handleUpdate and overriden;
- also it's impossible to move such realization of handleUpdate method to HoodieCompactionHandler as default implementation.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #10917:
URL: https://github.com/apache/hudi/pull/10917#discussion_r1536959896
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java:
##########
@@ -1096,11 +1104,37 @@ private Set<String> getDropPartitionColNames() {
return new HashSet<>(Arrays.asList(partitionFields.get()));
}
- public void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ public Iterator<List<WriteStatus>> handleUpdate(String instantTime, String partitionPath, String fileId,
+ Map<String, HoodieRecord<T>> keyToNewRecords, HoodieBaseFile oldDataFile) throws IOException {
+ HoodieMergeHandle upsertHandle = getUpdateHandle(instantTime, partitionPath, fileId, keyToNewRecords, oldDataFile);
+ return updateWithUpsertHandle(upsertHandle, instantTime, fileId);
+ }
+
+ public Iterator<List<WriteStatus>> updateWithUpsertHandle(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ runMerge(upsertHandle, instantTime, fileId);
+ return upsertHandle.getWriteStatusesAsIterator();
+ }
+
+ protected void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
if (upsertHandle.getOldFilePath() == null) {
throw new HoodieUpsertException("Error in finding the old file path at commit " + instantTime + " for fileId: " + fileId);
} else {
HoodieMergeHelper.newInstance().runMerge(this, upsertHandle);
}
}
+
+ protected HoodieMergeHandle getUpdateHandle(String instantTime, String partitionPath, String fileId,
Review Comment:
Not sure we should move these handle methods into `HoodieTable` because it is a top level notion, the current API already includes `upsert` operation method on this class. These overall "handle" related methods are kind of internal and I kind of think they do not belongs to the `HoodieTable` interface.
And is there any possibility the three methods `runMerge`, `updateWithUpsertHandle` and `handleUpdate` be merged into one method?
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #10917:
URL: https://github.com/apache/hudi/pull/10917#issuecomment-2016538989
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f1161ad007a7bb3b4b748d85e200ce29a87f34b2",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "f1161ad007a7bb3b4b748d85e200ce29a87f34b2",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* f1161ad007a7bb3b4b748d85e200ce29a87f34b2 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #10917:
URL: https://github.com/apache/hudi/pull/10917#discussion_r1536716915
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java:
##########
@@ -1096,11 +1104,37 @@ private Set<String> getDropPartitionColNames() {
return new HashSet<>(Arrays.asList(partitionFields.get()));
}
- public void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ public Iterator<List<WriteStatus>> handleUpdate(String instantTime, String partitionPath, String fileId,
+ Map<String, HoodieRecord<T>> keyToNewRecords, HoodieBaseFile oldDataFile) throws IOException {
+ HoodieMergeHandle upsertHandle = getUpdateHandle(instantTime, partitionPath, fileId, keyToNewRecords, oldDataFile);
+ return handleUpdateInternal(upsertHandle, instantTime, fileId);
+ }
+
+ public Iterator<List<WriteStatus>> handleUpdateInternal(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ runMerge(upsertHandle, instantTime, fileId);
Review Comment:
Agree with you, bad naming. Renamed to updateWithUpsertHandle()
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun closed pull request #10917: [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables
URL: https://github.com/apache/hudi/pull/10917
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #10917:
URL: https://github.com/apache/hudi/pull/10917#issuecomment-2016540546
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f1161ad007a7bb3b4b748d85e200ce29a87f34b2",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=23006",
"triggerID" : "f1161ad007a7bb3b4b748d85e200ce29a87f34b2",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* f1161ad007a7bb3b4b748d85e200ce29a87f34b2 Azure: [PENDING](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=23006)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #10917:
URL: https://github.com/apache/hudi/pull/10917#issuecomment-2016656151
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f1161ad007a7bb3b4b748d85e200ce29a87f34b2",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=23006",
"triggerID" : "f1161ad007a7bb3b4b748d85e200ce29a87f34b2",
"triggerType" : "PUSH"
}, {
"hash" : "84dd6612f0f52236936b70cfcb734eaf33fbe9e7",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "84dd6612f0f52236936b70cfcb734eaf33fbe9e7",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* f1161ad007a7bb3b4b748d85e200ce29a87f34b2 Azure: [SUCCESS](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=23006)
* 84dd6612f0f52236936b70cfcb734eaf33fbe9e7 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #10917:
URL: https://github.com/apache/hudi/pull/10917#issuecomment-2016670285
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f1161ad007a7bb3b4b748d85e200ce29a87f34b2",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=23006",
"triggerID" : "f1161ad007a7bb3b4b748d85e200ce29a87f34b2",
"triggerType" : "PUSH"
}, {
"hash" : "84dd6612f0f52236936b70cfcb734eaf33fbe9e7",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=23009",
"triggerID" : "84dd6612f0f52236936b70cfcb734eaf33fbe9e7",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 84dd6612f0f52236936b70cfcb734eaf33fbe9e7 Azure: [SUCCESS](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=23009)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #10917:
URL: https://github.com/apache/hudi/pull/10917#discussion_r1537019796
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java:
##########
@@ -1096,11 +1104,37 @@ private Set<String> getDropPartitionColNames() {
return new HashSet<>(Arrays.asList(partitionFields.get()));
}
- public void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ public Iterator<List<WriteStatus>> handleUpdate(String instantTime, String partitionPath, String fileId,
+ Map<String, HoodieRecord<T>> keyToNewRecords, HoodieBaseFile oldDataFile) throws IOException {
+ HoodieMergeHandle upsertHandle = getUpdateHandle(instantTime, partitionPath, fileId, keyToNewRecords, oldDataFile);
+ return updateWithUpsertHandle(upsertHandle, instantTime, fileId);
+ }
+
+ public Iterator<List<WriteStatus>> updateWithUpsertHandle(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ runMerge(upsertHandle, instantTime, fileId);
+ return upsertHandle.getWriteStatusesAsIterator();
+ }
+
+ protected void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
if (upsertHandle.getOldFilePath() == null) {
throw new HoodieUpsertException("Error in finding the old file path at commit " + instantTime + " for fileId: " + fileId);
} else {
HoodieMergeHelper.newInstance().runMerge(this, upsertHandle);
}
}
+
+ protected HoodieMergeHandle getUpdateHandle(String instantTime, String partitionPath, String fileId,
Review Comment:
Let's keep the core API clean first if we have no good ideas.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #10917:
URL: https://github.com/apache/hudi/pull/10917#issuecomment-2016554779
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f1161ad007a7bb3b4b748d85e200ce29a87f34b2",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=23006",
"triggerID" : "f1161ad007a7bb3b4b748d85e200ce29a87f34b2",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* f1161ad007a7bb3b4b748d85e200ce29a87f34b2 Azure: [SUCCESS](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=23006)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #10917:
URL: https://github.com/apache/hudi/pull/10917#discussion_r1536992371
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java:
##########
@@ -1096,11 +1104,37 @@ private Set<String> getDropPartitionColNames() {
return new HashSet<>(Arrays.asList(partitionFields.get()));
}
- public void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ public Iterator<List<WriteStatus>> handleUpdate(String instantTime, String partitionPath, String fileId,
+ Map<String, HoodieRecord<T>> keyToNewRecords, HoodieBaseFile oldDataFile) throws IOException {
+ HoodieMergeHandle upsertHandle = getUpdateHandle(instantTime, partitionPath, fileId, keyToNewRecords, oldDataFile);
+ return updateWithUpsertHandle(upsertHandle, instantTime, fileId);
+ }
+
+ public Iterator<List<WriteStatus>> updateWithUpsertHandle(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ runMerge(upsertHandle, instantTime, fileId);
+ return upsertHandle.getWriteStatusesAsIterator();
+ }
+
+ protected void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
if (upsertHandle.getOldFilePath() == null) {
throw new HoodieUpsertException("Error in finding the old file path at commit " + instantTime + " for fileId: " + fileId);
} else {
HoodieMergeHelper.newInstance().runMerge(this, upsertHandle);
}
}
+
+ protected HoodieMergeHandle getUpdateHandle(String instantTime, String partitionPath, String fileId,
Review Comment:
Yes, this `handleUpdate` method belongs to `HoodieCompactionHandler` interface, but `HoodieTable` is the only place where this code can be reused for all engine types without duplications.
I don't think it's possible to merge `runMerge`, `updateWithUpsertHandle` and `handleUpdate` into one methods. I tried and had these problems:
- `updateWithUpsertHandle` is used not only in compaction process, but in XXXCommitActionExecutors too;
- `runMerge` code is the same for Java and Flink engines, but it is different for Spark, so it must be separated from `handleUpdate` and overriden;
- also it's impossible to move such realization of `handleUpdate` method to `HoodieCompactionHandler` as default implementation.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #10917:
URL: https://github.com/apache/hudi/pull/10917#discussion_r1536992371
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java:
##########
@@ -1096,11 +1104,37 @@ private Set<String> getDropPartitionColNames() {
return new HashSet<>(Arrays.asList(partitionFields.get()));
}
- public void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ public Iterator<List<WriteStatus>> handleUpdate(String instantTime, String partitionPath, String fileId,
+ Map<String, HoodieRecord<T>> keyToNewRecords, HoodieBaseFile oldDataFile) throws IOException {
+ HoodieMergeHandle upsertHandle = getUpdateHandle(instantTime, partitionPath, fileId, keyToNewRecords, oldDataFile);
+ return updateWithUpsertHandle(upsertHandle, instantTime, fileId);
+ }
+
+ public Iterator<List<WriteStatus>> updateWithUpsertHandle(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ runMerge(upsertHandle, instantTime, fileId);
+ return upsertHandle.getWriteStatusesAsIterator();
+ }
+
+ protected void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
if (upsertHandle.getOldFilePath() == null) {
throw new HoodieUpsertException("Error in finding the old file path at commit " + instantTime + " for fileId: " + fileId);
} else {
HoodieMergeHelper.newInstance().runMerge(this, upsertHandle);
}
}
+
+ protected HoodieMergeHandle getUpdateHandle(String instantTime, String partitionPath, String fileId,
Review Comment:
Yes, this `handleUpdate` method belongs to `HoodieCompactionHandler` interface, but `HoodieTable` is the only place where this code can be reused for all engine types without duplications.
I don't think it's possible to merge `runMerge`, `updateWithUpsertHandle` and `handleUpdate` into one method. I tried and had these problems:
- `updateWithUpsertHandle` is used not only in compaction process, but in XXXCommitActionExecutors too;
- `runMerge` code is the same for Java and Flink engines, but it is different for Spark, so it must be separated from `handleUpdate` and overriden;
- also it's impossible to move such realization of `handleUpdate` method to `HoodieCompactionHandler` as default implementation.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #10917:
URL: https://github.com/apache/hudi/pull/10917#discussion_r1537030730
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java:
##########
@@ -1096,11 +1104,37 @@ private Set<String> getDropPartitionColNames() {
return new HashSet<>(Arrays.asList(partitionFields.get()));
}
- public void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ public Iterator<List<WriteStatus>> handleUpdate(String instantTime, String partitionPath, String fileId,
+ Map<String, HoodieRecord<T>> keyToNewRecords, HoodieBaseFile oldDataFile) throws IOException {
+ HoodieMergeHandle upsertHandle = getUpdateHandle(instantTime, partitionPath, fileId, keyToNewRecords, oldDataFile);
+ return updateWithUpsertHandle(upsertHandle, instantTime, fileId);
+ }
+
+ public Iterator<List<WriteStatus>> updateWithUpsertHandle(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ runMerge(upsertHandle, instantTime, fileId);
+ return upsertHandle.getWriteStatusesAsIterator();
+ }
+
+ protected void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
if (upsertHandle.getOldFilePath() == null) {
throw new HoodieUpsertException("Error in finding the old file path at commit " + instantTime + " for fileId: " + fileId);
} else {
HoodieMergeHelper.newInstance().runMerge(this, upsertHandle);
}
}
+
+ protected HoodieMergeHandle getUpdateHandle(String instantTime, String partitionPath, String fileId,
Review Comment:
If the code is copied 3-6 times in different places it doesn't look clean.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #10917:
URL: https://github.com/apache/hudi/pull/10917#discussion_r1536712262
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java:
##########
@@ -1096,11 +1104,37 @@ private Set<String> getDropPartitionColNames() {
return new HashSet<>(Arrays.asList(partitionFields.get()));
}
- public void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ public Iterator<List<WriteStatus>> handleUpdate(String instantTime, String partitionPath, String fileId,
+ Map<String, HoodieRecord<T>> keyToNewRecords, HoodieBaseFile oldDataFile) throws IOException {
+ HoodieMergeHandle upsertHandle = getUpdateHandle(instantTime, partitionPath, fileId, keyToNewRecords, oldDataFile);
+ return handleUpdateInternal(upsertHandle, instantTime, fileId);
+ }
+
+ public Iterator<List<WriteStatus>> handleUpdateInternal(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ runMerge(upsertHandle, instantTime, fileId);
Review Comment:
If it is "Internal", how could it be public scope?
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #10917:
URL: https://github.com/apache/hudi/pull/10917#discussion_r1536992371
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java:
##########
@@ -1096,11 +1104,37 @@ private Set<String> getDropPartitionColNames() {
return new HashSet<>(Arrays.asList(partitionFields.get()));
}
- public void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ public Iterator<List<WriteStatus>> handleUpdate(String instantTime, String partitionPath, String fileId,
+ Map<String, HoodieRecord<T>> keyToNewRecords, HoodieBaseFile oldDataFile) throws IOException {
+ HoodieMergeHandle upsertHandle = getUpdateHandle(instantTime, partitionPath, fileId, keyToNewRecords, oldDataFile);
+ return updateWithUpsertHandle(upsertHandle, instantTime, fileId);
+ }
+
+ public Iterator<List<WriteStatus>> updateWithUpsertHandle(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ runMerge(upsertHandle, instantTime, fileId);
+ return upsertHandle.getWriteStatusesAsIterator();
+ }
+
+ protected void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
if (upsertHandle.getOldFilePath() == null) {
throw new HoodieUpsertException("Error in finding the old file path at commit " + instantTime + " for fileId: " + fileId);
} else {
HoodieMergeHelper.newInstance().runMerge(this, upsertHandle);
}
}
+
+ protected HoodieMergeHandle getUpdateHandle(String instantTime, String partitionPath, String fileId,
Review Comment:
Yes, this handleUpdate method belongs to HoodieCompactionHandler interface, but HoodieTable is the only place where this code can be reused for all engine types without duplications.
I don't think it's possible to merge `runMerge`, `updateWithUpsertHandle` and `handleUpdate` into one methods. I tried and had these problems:
- `updateWithUpsertHandle` is used not only in compaction process, but in XXXCommitActionExecutors too;
- `runMerge` code is the same for Java and Flink engines, but it is different for Spark, so it must be separated from `handleUpdate` and overriden;
- also it's impossible to move such realization of `handleUpdate` method to `HoodieCompactionHandler` as default implementation.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #10917:
URL: https://github.com/apache/hudi/pull/10917#discussion_r1537030730
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java:
##########
@@ -1096,11 +1104,37 @@ private Set<String> getDropPartitionColNames() {
return new HashSet<>(Arrays.asList(partitionFields.get()));
}
- public void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ public Iterator<List<WriteStatus>> handleUpdate(String instantTime, String partitionPath, String fileId,
+ Map<String, HoodieRecord<T>> keyToNewRecords, HoodieBaseFile oldDataFile) throws IOException {
+ HoodieMergeHandle upsertHandle = getUpdateHandle(instantTime, partitionPath, fileId, keyToNewRecords, oldDataFile);
+ return updateWithUpsertHandle(upsertHandle, instantTime, fileId);
+ }
+
+ public Iterator<List<WriteStatus>> updateWithUpsertHandle(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
+ runMerge(upsertHandle, instantTime, fileId);
+ return upsertHandle.getWriteStatusesAsIterator();
+ }
+
+ protected void runMerge(HoodieMergeHandle<?, ?, ?, ?> upsertHandle, String instantTime, String fileId) throws IOException {
if (upsertHandle.getOldFilePath() == null) {
throw new HoodieUpsertException("Error in finding the old file path at commit " + instantTime + " for fileId: " + fileId);
} else {
HoodieMergeHelper.newInstance().runMerge(this, upsertHandle);
}
}
+
+ protected HoodieMergeHandle getUpdateHandle(String instantTime, String partitionPath, String fileId,
Review Comment:
If the code is copied 3-6 times in different places it doesn't looks clean.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-7534] Refactoring of handleUpdate in CommitActionExecutors and HoodieTables [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #10917:
URL: https://github.com/apache/hudi/pull/10917#issuecomment-2016657794
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f1161ad007a7bb3b4b748d85e200ce29a87f34b2",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=23006",
"triggerID" : "f1161ad007a7bb3b4b748d85e200ce29a87f34b2",
"triggerType" : "PUSH"
}, {
"hash" : "84dd6612f0f52236936b70cfcb734eaf33fbe9e7",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=23009",
"triggerID" : "84dd6612f0f52236936b70cfcb734eaf33fbe9e7",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* f1161ad007a7bb3b4b748d85e200ce29a87f34b2 Azure: [SUCCESS](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=23006)
* 84dd6612f0f52236936b70cfcb734eaf33fbe9e7 Azure: [PENDING](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=23009)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org