-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Refactor external sst file ingestion job #12305
Conversation
f0f1438
to
edf1d41
Compare
edf1d41
to
9b62dfd
Compare
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.
LGTM
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@jowlyzhang merged this pull request in 4eaa771. |
f.internal_file_path.c_str(), s.ToString().c_str()); | ||
} | ||
} | ||
DeleteInternalFiles(); |
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.
It seems we call Cleanup() after Prepare() return non-ok status:
Line 5850 in 6e88126
ingestion_jobs[i].Cleanup(status); |
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.
Yes, do you mean that we should remove this DeleteInternalFiles
call from Prepare
, so we don't get the "clean up for file failed" for this scenario?
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, I did not think about the warnings but just that we don't need to call DeleteInternalFiles()
here.
Updates some documentations and invariant assertions after #12257 and #12284. Also refactored some duplicate code and improved some error message and preconditions for errors.
Test plan:
Existing unit tests