Skip to content
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

Deprecate EnforceMultiVA and MultiVAFullResults feature flags #7520

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

aarongable
Copy link
Contributor

@aarongable aarongable commented May 31, 2024

These flags have been true and false, respectively, for years. We do not expect to change them at any time in the future, and their continued existence makes certain parts of the VA code significantly more complex.

Remove all references to them, preserving behavior in the "enforce, but not full results" configuration.

IN-10358 tracks the corresponding config changes

@aarongable aarongable marked this pull request as ready for review May 31, 2024 23:39
@aarongable aarongable requested a review from a team as a code owner May 31, 2024 23:39
@aarongable aarongable requested a review from jsha May 31, 2024 23:39
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

Looks great. I pointed out another possible simplification, which you can take or leave; I'm fine merging without it.

Comment on lines +569 to +570
// If we somehow haven't returned early, we need to break the loop once all
// of the VAs have returned a result.
Copy link
Contributor

Choose a reason for hiding this comment

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

A further available simplification: the return probs.ServerInternal("Too few remote PerformValidation RPC results") line below can be inlined here, in place of the break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I considered that, but then you still have to have a "this should never happen" return at the bottom of the function anyway, because Go can't tell that it's impossible to actually fall off the loop. So I felt like having one break and one return was cleaner in the end.

@aarongable aarongable merged commit c3c278a into main Jun 4, 2024
24 checks passed
@aarongable aarongable deleted the rm-va-full-results branch June 4, 2024 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants