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

[#3923] Simplify render for Flask #3979

Merged
merged 2 commits into from
Jan 22, 2018

Conversation

smotornyuk
Copy link
Member

@wardi, @amercader, PageCacheMiddleware is working for anonymous requests served by pylons, when ckan.page_cache_enabled is set to true. Wouldn't it be better to enable caching middleware for Flask(and migrate allow_cache variable as well) instead?

PS in that case set_pylons_response_headers should be applied to flask response as well. Otherwise, this function can be just ignored as it only controls cache headers

@smotornyuk smotornyuk removed the WIP label Jan 16, 2018
@amercader amercader self-assigned this Jan 16, 2018
@@ -275,7 +276,8 @@ def update_config():
jinja_extensions.LinkForExtension,
jinja_extensions.ResourceExtension,
jinja_extensions.UrlForStaticExtension,
jinja_extensions.UrlForExtension]
jinja_extensions.UrlForExtension],
bytecode_cache=jinja2.FileSystemBytecodeCache()
Copy link
Member

Choose a reason for hiding this comment

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

I can see how cloud or Docker based deployments may not want to use a file system based cache, and use the memcache one shipped by jinja2 or a custom one. Perhaps we could have a ckan.jinja2.bytecode_cache_class config option that points to the class to use (which defaults to jinja2.FileSystemBytecodeCache), but if it's too much hassle that can be a separate feature.

Copy link
Member

@TkTech TkTech Jan 16, 2018

Choose a reason for hiding this comment

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

When I cleaned up the Jinja stuff I originally had this same change in there (but as an INI configurable). We ended up removing it because it was not behaving correctly with translations, resulting in the wrong language text showing up for snippets if it had been previously cached under a different language. Seems like the cache key wasn't including language, which it should.

Not sure if this has been fixed, but this is why we removed it the 1st time.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like the cache key wasn't including language, which it should.

Is this something that should be done at the CKAN level (ie something we can fix) or at the Jinja2 one?

in any case @smotornyuk this seems easy to test, just change langugages a few times with the cache enabled and see if it gets updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

As for different languages, there are two points:

  1. cache key does not include language key, as said Tyler.
  2. Jinja compiles and caches code to produce page, not page itself. So translation points stays there and there are no visible problems with translations.

So we shouldn't worry about custom locale handling cache.

As for docker - doesn't it allow to create files inside container? I thought, that those files won't be preserved during container restart, but inside one session they should be safely stored inside container's temporary folder. Isn't it so?

Copy link
Member

@amercader amercader Jan 22, 2018

Choose a reason for hiding this comment

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

Docker does allow to create files in the container but the aim is to have state-less containers (ie rebuilding the container should not include any state including caches, db etc )

Different backends can be a separate feature later on (@TkTech maybe your ini file based changed is the history somewhere?) I was more concerned about the handling of locales. You seem to suggest that this shouldn't be an issue and I made a quick test switching languages and everything seem to work fine. Let's merge it and keep an eye on it for a few weeks.

@amercader amercader merged commit ec9ce30 into ckan:master Jan 22, 2018
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.

None yet

3 participants