-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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 ProcessManifestWrites a little bit #7751
Refactor ProcessManifestWrites a little bit #7751
Conversation
This PR is a follow-up of #7740 (comment). |
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.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
0980eca
to
3c3f960
Compare
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
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.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
cc @Cheng-Chang since the PR is about WAL info computation. |
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, thanks for the patch @riversand963 !
Summary: This PR removes a nested loop inside ProcessManifestWrites. The new implementation has the same behavior as the old code with simpler logic and lower complexity. Test Plan: make check Run make crash_test on devserver and succeeds 3 times.
3c3f960
to
206821d
Compare
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
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.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
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.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@riversand963 merged this pull request in 11c4be2. |
Summary: This PR removes a nested loop inside ProcessManifestWrites. The new implementation has the same behavior as the old code with simpler logic and lower complexity. Pull Request resolved: facebook#7751 Test Plan: make check Run make crash_test on devserver and succeeds 3 times. Reviewed By: ltamasi Differential Revision: D25363526 Pulled By: riversand963 fbshipit-source-id: 27e681949dacd7501a752e5e517b9e85b54ccb2e
Summary:
This PR removes a nested loop inside ProcessManifestWrites. The new
implementation has the same behavior as the old code with simpler logic
and lower complexity.
Test Plan:
make check
Run make crash_test on devserver and succeeds 3 times.