Schema Cache Improvements #5612
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@davimacedo can you take a crack at reviewing? |
Codecov Report
@@ Coverage Diff @@
## master #5612 +/- ##
==========================================
+ Coverage 94.06% 94.11% +0.05%
==========================================
Files 129 129
Lines 9243 9231 -12
==========================================
- Hits 8694 8688 -6
+ Misses 549 543 -6
Continue to review full report at Codecov.
|
|
@acinader This is ready for review. I edit my original post with comments. |
acinader
approved these changes
May 24, 2019
|
@acinader I rewrote the tests and found more places for improvement. I'll post them up-for-grabs for the community. |
acinader
reviewed
May 24, 2019
davimacedo
reviewed
May 24, 2019
acinader
approved these changes
May 24, 2019
davimacedo
reviewed
May 24, 2019
| return this._dbAdapter | ||
| .getAllClasses() | ||
| .then(allSchemas => allSchemas.map(injectDefaultSchema)) | ||
| .then(allSchemas => { |
There was a problem hiding this comment.
It is a small improvement, but I think we don't need to wait the cache promise to resolve. I'd do something like this::
this._cache.setAllClasses(allSchemas).catch(error => console.error('Error saving schema to cache', error));
return allSchemas;
davimacedo
approved these changes
May 24, 2019
UnderratedDev
pushed a commit
to UnderratedDev/parse-server
that referenced
this pull request
Mar 21, 2020
* Cache Improvements * improve tests * more tests * clean-up * test with singlecache * ensure indexes exists * remove ALL_KEYS * Add Insert Test * enableSingleSchemaCache default true * Revert "enableSingleSchemaCache default true" This reverts commit 323e713. * further optimization * refactor enforceFieldExists * coverage improvements * improve tests * remove flaky test * cleanup * Learned something new
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #4530
Tested with Redis and enableSingleSchemaCache: true
Significant reduction on creating, updating and deleting objects.
This can be further improved
schema.enforceFieldExistshould be batched for multiple fieldsCould run in parallel instead of chained, should wait to refresh cache after all fields are checked. (Done)
schema.hasClasscould check if class exists then reload if necessaryThis is a tricky one. Only used for allowClientCreation. AddClass and DeleteField clears the cache but maybe it should reload instead (This may come at a performance cost, I haven't done the research but up for discussion).
enableSingleSchemaCacheshould be true by defaultTried and caused a few tests to fail. Will do a separate PR.
batch / saveAll / directAccessAdding class schema before batch will reduce calls in half if it doesn't exist. Adding batchSize > number of objects will result in fewer GET call but 4x as many PUT calls. (Need investigation)