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

fix: schemaVersion called with old context #293

Merged
merged 1 commit into from
Dec 24, 2019

Conversation

willhausman
Copy link
Contributor

@willhausman willhausman commented Dec 14, 2019

Fix for #294

If you override schemaVersion(context), context is not the current request's context. This is because the schemaVersion() called in CompilerApi is a closure. By adding the schemaVersion to the key for the compilerCache, CompilerApi will effectively recompile with new schemaVersions.

Note: If the server has not specified a maxCompilerCacheKeepAlive, the old compilers will remain in memory until compilerCacheSize is reached. At that point, the LRU cache will start evicting the old schemas.

Note for OrchestratorApi: Current workaround for incorrect context in schemaVersion is to tack the schemaVersion into your contextToAppId. This introduces many OrchestratorApis when there should be far fewer. Passing the correct context to schemaVersion(context) resolves this issue.

Note for CompilerApi: This was left untouched to allow customizations directly calling createCompilerApi to still behave the same without a call to schemaVersion(context), and because options.devServer is handled there. It's starting to feel messy, so probably schemaVersion needs to be restructured overall.

@paveltiunov
Copy link
Member

@willhausman Hey Will! Thanks for contributing this! Let's reiterate on this one: could you please provide example scenario, explanation of a problem and how it is solved by this change. Preferably with implementations of affected functions and values they return on each request just to make sure we're thinking about the same problem.

@willhausman
Copy link
Contributor Author

Created #294 with more detail. This implementation is one way I envision it can be solved.

@willhausman
Copy link
Contributor Author

Now simply passes current context along through CompilerApi to schemaVersion.

@paveltiunov
Copy link
Member

@willhausman Looks good! The only thing: can we set schemaVersion for CompilerApi in getCompilerApi method instead of adding additional arguments to all methods? It'll be a little bit less changes and api-gateway won't be involved. We can introduce explicit setter for schemaVersion in CompilerApi for that.

@paveltiunov
Copy link
Member

@willhausman Looks great! Thanks for contributing this one!

@paveltiunov paveltiunov merged commit da10e39 into cube-js:master Dec 24, 2019
@willhausman willhausman deleted the fix-schemaVersion-context branch December 24, 2019 05:18
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.

2 participants