Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix a failing test case intermittenly in TestMergeOnRead due to incorrect prev commit time #448

Merged
merged 1 commit into from
Sep 8, 2018

Conversation

n3nash
Copy link
Contributor

@n3nash n3nash commented Sep 7, 2018

@vinothchandar This should fix the failing test case in dependency removal.

@n3nash
Copy link
Contributor Author

n3nash commented Sep 7, 2018

@vinothchandar pinging to re-surface

@n3nash
Copy link
Contributor Author

n3nash commented Sep 7, 2018

Will add a test case soon to concretely add this case : #450

@@ -235,7 +235,6 @@ public WriteStatus close() {
if (writer != null) {
writer.close();
}
writeStatus.getStat().setPrevCommit(commitTime);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@n3nash : Can you provide some context here around this and why this needs to be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bvaradar Yes, so essentially 2 reasons :

  1. For inserts we always have PREV_COMMIT set to NULL as you can see in HoodieCreateHandle, I want to keep the same semantics
  2. Say a log file was created with commit = 101. Now, when an update comes along for this log file at commit = 102, the write status will be set PREV_COMMIT = 102. Now, any rollback will try to write to a log file with commit = 102 which is incorrect. This got introduced in the inserts to log files diff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. makes sense

Copy link
Contributor

@bvaradar bvaradar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vinothchandar : looks good to me.

@@ -235,7 +235,6 @@ public WriteStatus close() {
if (writer != null) {
writer.close();
}
writeStatus.getStat().setPrevCommit(commitTime);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. makes sense

@vinothchandar vinothchandar merged commit 0fe92de into apache:master Sep 8, 2018
vinishjail97 pushed a commit to vinishjail97/hudi that referenced this pull request Dec 15, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants