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

sessionctx: move set variable to sysvar struct #21424

Closed
wants to merge 7 commits into from

Conversation

morgo
Copy link
Contributor

@morgo morgo commented Dec 1, 2020

What problem does this PR solve?

Problem Summary:

This is a refactor only.

Previously, setting a sysvar used a large switch statement to run the required code to process the change. In this PR, the switch statement is refactored to be a function call on the sysvar struct. The advantage of doing this as a function call, is that plugins will be able to add/process setvar changes in the same way as regular server options.

Additional notes:

  • Validation already calls a function on the sysvar struct. This change moves the SetSession function to the struct.
  • There are some special-cases for character set, which for now continue to be handled as part of the switch statement.

What is changed and how it works?

What's Changed:

Code refactored

Related changes

  • None

Check List

Tests

  • Unit test

Existing tests pass

Side effects

  • Large code changes (makes merging/cherry picking hard.)

Release note

  • No release note

@morgo
Copy link
Contributor Author

morgo commented Dec 1, 2020

/run-all-tests

@morgo
Copy link
Contributor Author

morgo commented Dec 1, 2020

/run-common-test

@morgo
Copy link
Contributor Author

morgo commented Dec 1, 2020

/run-all-tests

@morgo
Copy link
Contributor Author

morgo commented Dec 1, 2020

/run-common-test

@morgo
Copy link
Contributor Author

morgo commented Dec 2, 2020

/run-check-dev-2

@morgo morgo mentioned this pull request Dec 2, 2020
3 tasks
@morgo
Copy link
Contributor Author

morgo commented Dec 2, 2020

/run-all-tests

@morgo
Copy link
Contributor Author

morgo commented Dec 2, 2020

/run-check_dev_2

@morgo morgo requested a review from a team as a code owner December 3, 2020 01:55
@morgo morgo requested review from qw4990 and removed request for a team December 3, 2020 01:55
@github-actions github-actions bot added the sig/execution SIG execution label Dec 3, 2020
@morgo
Copy link
Contributor Author

morgo commented Dec 3, 2020

There was a test failure that is fixed in #21442 - I've added the code from this PR here, but it will dissapear from the diff assuming it merges to master soon.

@morgo
Copy link
Contributor Author

morgo commented Dec 3, 2020

/run-all-tests

@morgo
Copy link
Contributor Author

morgo commented Feb 4, 2021

Closing for now.

@morgo morgo closed this Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant