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

Cache Parameter Metadata #1845

Merged
merged 23 commits into from
Jun 22, 2022
Merged

Cache Parameter Metadata #1845

merged 23 commits into from
Jun 22, 2022

Conversation

Jeffery-Wasty
Copy link
Contributor

@Jeffery-Wasty Jeffery-Wasty commented Jun 13, 2022

Cache sp_describe_parameter_encryption calls to improved performance by reducing trips to the server. For now, supports AE without secure enclaves, with enclave support (v3) to be added in a later update.

@Jeffery-Wasty Jeffery-Wasty marked this pull request as ready for review June 15, 2022 16:54
@Jeffery-Wasty Jeffery-Wasty added this to the 11.1.2 milestone Jun 15, 2022
@Jeffery-Wasty Jeffery-Wasty self-assigned this Jun 15, 2022
@Jeffery-Wasty Jeffery-Wasty added the Under Review Used for pull requests under review label Jun 15, 2022
* @return true, if the metadata for the query can be retrieved
*
*/
static boolean getQueryMetadata(Parameter[] params, ArrayList<String> parameterNames,
Copy link
Contributor

@lilgreenbird lilgreenbird Jun 21, 2022

Choose a reason for hiding this comment

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

we don't need all these static methods this takes up memory. The caller should create an instance of the cache and invoke the methods

lilgreenbird
lilgreenbird previously approved these changes Jun 21, 2022
David-Engel
David-Engel previously approved these changes Jun 21, 2022
Copy link
Collaborator

@David-Engel David-Engel left a comment

Choose a reason for hiding this comment

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

LGTM

…s problems, test will still correctly assess the error code path in ParameterMetaDataCache
tkyc
tkyc previously approved these changes Jun 21, 2022
lilgreenbird
lilgreenbird previously approved these changes Jun 21, 2022
@Jeffery-Wasty Jeffery-Wasty dismissed stale reviews from lilgreenbird and tkyc via 64c4783 June 22, 2022 16:14
@lilgreenbird lilgreenbird merged commit a417222 into main Jun 22, 2022
@Jeffery-Wasty Jeffery-Wasty removed the Under Review Used for pull requests under review label Jun 22, 2022
@Jeffery-Wasty Jeffery-Wasty deleted the cache-parameter-metadata branch June 22, 2022 21:14
@alex-bochkov
Copy link

alex-bochkov commented Jun 22, 2022

@Jeffery-Wasty this is an exciting change! Thank you for working on it!
Are there documents that explain how exactly this process works?

  1. What's the cache TTL?
  2. Is it possible to disable cache if needed? For example, if we need to make schema changes and make sure the app won't break in the middle.

@Jeffery-Wasty
Copy link
Contributor Author

Hi @alex-bochkov,

We'll be releasing documentation regarding new features shortly, in the meantime I can answer the questions you have included:

1. What's the cache TTL?

The cache doesn't have an independent TTL, rather the enclave session it belongs to has a TTL of eight hours.

2. Is it possible to disable cache if needed? For example, if we need to make schema changes and make sure the app won't break in the middle.

It's not possible to disable the cache at the moment. This is something we can potentially add a later time, however current efforts are towards extending caching for secure enclave scenarios.

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.

6 participants