Skip to content

[ui/bundles][optimizer] only use caches when in dev mode - #15780

Merged
spalger merged 4 commits into
elastic:masterfrom
spalger:fix/no-caches-in-build
Jan 4, 2018
Merged

spalger merged 4 commits into
elastic:masterfrom
spalger:fix/no-caches-in-build

Conversation

@spalger

@spalger spalger commented Dec 27, 2017

Copy link
Copy Markdown
Contributor

Fixes #14813 by disabling the cache-loaders in the optimizer when running in production mode.

@tylersmalley

Copy link
Copy Markdown
Member

What effect does this have on plugin installation in production?

@spalger

spalger commented Dec 28, 2017

Copy link
Copy Markdown
Contributor Author

None. Caches are unique for each plugin combination, so when a plugin is installed the cache is discarded and a new cache is created. Plugin install will be slower if you install a plugin and then uninstall it.

@tylersmalley tylersmalley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@tylersmalley

tylersmalley commented Jan 3, 2018 •

Copy link
Copy Markdown
Member

In testing, this might actually be a small improvement to build times. Seeing a 6% reduction in plugin installation for 6.1.1, going from an average installation time of 438 seconds to 410. Might be from the disk IO.

getCachePath() {
return this.resolvePath('../.cache', this.hashBundleEntries());
getCacheDirectory(...subPath) {
if (this.isDevMode()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we add a comment that explains the reasoning here? Maybe something along the lines of

Disabling the cache-loader in the optimizer when running in production mode, as it creates cache files in optimize/.cache that are not necessary for distributable versions of Kibana and just make compressing and extracting it more difficult.

That should hopefully make it easier to remember later on why this was works the way it does

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I appreciate your observation that the relationship between this and the cache loader was not clear, so I think I did a bit better than a comment and put the condition in maybeAddCacheLoader() within BaseOptimizer#getConfig() instead.

@tylersmalley

Copy link
Copy Markdown
Member

I believe this should fix the issue mentioned here: https://discuss.elastic.co/t/kibana-6-1-0-starts-with-error/111754

@spalger
spalger force-pushed the fix/no-caches-in-build branch from 7c28e83 to fbd17e0 Compare January 4, 2018 17:55
Comment thread src/optimize/base_optimizer.js Outdated
}

function maybeAddCacheLoader(uiBundles, cacheName, loaders) {
// only use cache-loader in dev mode

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general I prefer comments that describe why we're doing something, not what is being done. E.g. it's obvious from the line below that we're only adding the cache loader in dev mode, but it's not clear why.

I suggest something like:

/**
 * Adds a cache loader if we're running in dev mode. The reason we're not adding
 * the cache-loader when running in production mode is that it creates cache
 * files in optimize/.cache that are not necessary for distributable versions
 * of Kibana and just make compressing and extracting it more difficult.
 */
function maybeAddCacheLoader(uiBundles, cacheName, loaders) {

Now I don't have to know the details of the cache-loader to understand why it's excluded in production (in general you'd think caching is something we want in production, so this helps clarify why it's not something we want)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Alright, that makes total sense. Thanks!

@spalger
spalger merged commit 4dee8fc into elastic:master Jan 4, 2018
spalger added a commit to spalger/kibana that referenced this pull request Jan 4, 2018
* [ui/bundles][optimizer] only use caches when in dev mode

* [optimize/caching] make cache-loader disabling more explicit

* [optimize/caching] clarify why we only want caching in dev
@spalger spalger removed the v6.1.2 label Jan 4, 2018
spalger added a commit that referenced this pull request Jan 5, 2018
… (#15854)

* [ui/bundles][optimizer] only use caches when in dev mode

* [optimize/caching] make cache-loader disabling more explicit

* [optimize/caching] clarify why we only want caching in dev
@spalger

spalger commented Jan 5, 2018

Copy link
Copy Markdown
Contributor Author

6.2/6.x: a29c6e4

@spalger
spalger deleted the fix/no-caches-in-build branch January 5, 2018 18:12
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
* [ui/bundles][optimizer] only use caches when in dev mode

* [optimize/caching] make cache-loader disabling more explicit

* [optimize/caching] clarify why we only want caching in dev
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants