-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
VerifySstUniqueIds status is overrided for multi CFs #10247
Conversation
@jay-zhuang has imported this pull request. If you are a Meta 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.
If you're going to skip the unit test, can you at least make status
a variable in the inner scope and early return on non-OK? That is relying less on fragile invariants.
203d978
to
3598df4
Compare
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
@jay-zhuang has imported this pull request. If you are a Meta 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.
Thanks!
Summary: There's bug that basically we only report the last CF's VerifySstUniqueIds() result: facebook#9990 (comment) Test Plan: CI
3598df4
to
3762ea9
Compare
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: There's bug that basically we only report the last CF's
VerifySstUniqueIds() result:
#9990 (comment)
Test Plan: CI