[SERVER-82295] Classic and SBE plan cache API definition and naming clarity Created: 18/Oct/23 Updated: 27/Oct/23 |
|
| Status: | In Code Review |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Task | Priority: | Major - P3 |
| Reporter: | Kevin Cherkauer | Assignee: | Kevin Cherkauer |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Participants: |
| Description |
|
Improve the ease of working in the plan cache code, especially by creating a single, central API definition for each of the Classic and SBE plan caches; renaming classes, methods, functions, and data types to have consistent, unique, and non-generic names; and consolidating code into fewer places. |
| Comments |
| Comment by Kevin Cherkauer [ 27/Oct/23 ] |
|
PR 16378 is ready for review and is intended to be delivered as production code. The purpose of this PR is to make plan cache code easier to work with through several code hygiene practices. Most of the diff is from moving existing code around and renaming a number of types and methods, not fundamentally changing existing code or writing new code. Friction points this PR addresses:
There was a barrier to fully consolidating the APIs because some pieces (classic_plan_cache.cpp, sbe_plan_cache.cpp, and their shared parent plan_cache.h) are in a separate, small build target that other components depend on, while the APIs for adding things to the plan cache exist in and depend on code from in a different, very large build target that other components probably don't want to depend on, and attempting to depend on it would probably create dependency cycles anyway. Therefore I was not able to move all the relevant APIs from the small library to the large one or vice versa, but the large library depends on the small one, so I created APIs in the large library that, in some cases, wrap APIs in the small library and made the APIs in the small library private. (In several cases making the small library APIs private required writing public methodNameTest() wrappers for them that are clearly commented as being for unit test use only, while the new API classes are made friend classes so they can call the private APIs. This is to discourage more APIs from being added in the small library that would again break the goal of having a single central official API definition for each plan cache. Production code should not call any "xyzTest()" methods.) The crux of this PR is the creation of central API definitions for each plan cache in one *.h file each:
These consolidate APIs for
There was not sufficient time during Skunkworks to try to consolidate auto-parameterization APIs, which is the last main piece of the SBE plan cache. |