Play should NOT clear cache after every code change about play1 HOT 18 CLOSED

I believe we want to clear because Cache relies on java Serializable. Class changes can easily break java serialization resulting in entries in the cache that the application is unable to deserialize.

from play1.

@theo Yes, this reason is clear. Sometimes cache can contain classes that have been modified. But it happens rarely. In most cases, cache still contains valid objects.

At the same time, the reason "cache may contain lot of data that is hard to load again" is still very important for us. In our project this is a problem.

So I suggest to solve this problem in another way. Let's leave cache untouched, but add try/catch to method Cache.get(). This method can return null if it failed to deserialize object which has been recently changed.

from play1.

Would this be the case even when not running in dev mode? We’ve actually
been looking for a solution recently to how we handle rolling deployments
of our application with a central memcached. When we deploy a version with
changes to cached objects we have serialisation errors until we clear the
cache. This could be a cleaner solution for us as existing code would just
re-add the new version to the cache for each object if it returned null.

On 28 June 2016 at 06:26, Andrei Solntsev [email protected] wrote:

@theo https://github.com/theo Yes, this reason is clear. Sometimes
cache can contain classes that have been modified. But in happens rarely.
In most cases, cache still contains valid objects.

At the same time, the reason "cache may contain lot of data that is hard
to load again" is still very important for us. In our project this is a
problem.

So I suggest to solve this problem in another way. Let's leave cache
untouched, but add try/catch to method Cache.get(). This method can
return null if it failed to deserialize object which has been recently
changed.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#985 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABFgvdyQuEdvzD2OYe0HPloypvRCmEwHks5qQLCNgaJpZM4I_ika
.

from play1.

Yes,
we also experience the same problem.
When we install new release, we get lot of warnings in logs (we have a
wrapper for Cache with try/catch inside).
Yes, I hope we can solve this problem in that way.

Andrei Solntsev

2016-06-30 21:08 GMT+03:00 Fraserhardy [email protected]:

Would this be the case even when not running in dev mode? We’ve actually
been looking for a solution recently to how we handle rolling deployments
of our application with a central memcached. When we deploy a version with
changes to cached objects we have serialisation errors until we clear the
cache. This could be a cleaner solution for us as existing code would just
re-add the new version to the cache for each object if it returned null.

On 28 June 2016 at 06:26, Andrei Solntsev [email protected]
wrote:

@theo https://github.com/theo Yes, this reason is clear. Sometimes
cache can contain classes that have been modified. But in happens rarely.
In most cases, cache still contains valid objects.

At the same time, the reason "cache may contain lot of data that is hard
to load again" is still very important for us. In our project this is a
problem.

So I suggest to solve this problem in another way. Let's leave cache
untouched, but add try/catch to method Cache.get(). This method can
return null if it failed to deserialize object which has been recently
changed.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<
https://github.com/playframework/play1/issues/985#issuecomment-228951704>,
or mute the thread
<
https://github.com/notifications/unsubscribe/ABFgvdyQuEdvzD2OYe0HPloypvRCmEwHks5qQLCNgaJpZM4I_ika

.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#985 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AARE3eKkPsRUoD0_xk9a-UKKHAT7Uu3mks5qRAYxgaJpZM4I_ika
.

from play1.

@theo @Fraserhardy @xael-fry
Please review the pull request #999 that should fix this problem.

Proposal is simple: Play does not clear cache on reload, but ignores all deserialization errors during 1 minute after restart. This period is configurable via play.cache.warmupPeriodMs property (default value is 60000).

from play1.

It seems bad practice to swallow exceptions, especially during startup.

This seems like something the client needs to handle. If a client attempts to pull something from cache and is not able to deserialize, the client is in the best position to handle the error (ignore or some other remediation)

from play1.

@theo I agree with you, I also don't like swallowing exceptions.
But in this case, Play swallows exceptions only during 1 minute. If you have a problem, you will start getting it after a minute. You will not miss it.

Anyway, I would like to implement a better solution, but I failed to do it.
My ideal algorithm would be:

1. Get object from cache
2. If failed to deserialize it, check when it was put to cache
3. If it was put to cache before last restart of play, ignore the exception.
   The problem is that I don't know how to get p.2: when the object was put to cache?
   So current solution seems to be reasonable compromise.

PS. BY the way, Play 1.4.3 (and earlier) swallows many other important exceptions... :(

from play1.

There seems to be too many failure scenarios for this to be a good idea. The cache could be used to attempt to pull something during startup to initialize prior to restart. If there a swallowed exception and we initialize the state to null without knowing it, that could result in unintended consequences and it would all happen silently.

Fail-fast seems like the best option here. The Cache API should throw exceptions when its unable to serialize/deserialize from cache at all times. The client needs to determine what to do in cases of failure. I don't think changing the Cache API contract is appropriate.

Another proposal - Allow passing in a play application property/config option (play.cache.clearOnStartup) that skips the Cache clearing on startup.

Default is existing behavior (play.cache.clearOnStartup == true)

public void detectChanges() {
        ...
        if (!newDefinitions.isEmpty() && "true".equals(Play.configuration.getProperty("play.cache.clearOnStartup", "true")) {
            Cache.clear();
        ...
}

from play1.

@theo Well, yes, it's one option.
I can set play.cache.clearOnStartup to false in my application.
But what then I should do?
I still need to ignore deserialization errors after startup. How I will do that?

Probably we could just merge these 2 properties?
If aNewProperty is false, then Play

1. will not clear cache on reload
2. will ignore deserialization errors on startup

By default If aNewProperty is true, meaning that Play behavies as earlier:

1. clears cache on reload
2. does not swallow deserialization errors

from play1.

I see two different issues we're trying to address.

1. Choosing to clear or not clear the cache on startup in DEV mode (solvable via a property that is checked on startup)
2. Ignoring serialization/deserialization errors when using the play Cache API

If one choses to clear the cache on startup, there shouldn't be getting deserialization errors on startup since there is nothing in the cache.

If one choses to retain the cache on startup, there is risk of the current Cache API attempting to deserialize something that has changed and throw a deserialization exception and your application should handle it. This is something you would have to manage in PROD mode.

If one needs a Cache implementation that is silently ignores errors, it seems like implementing a custom CacheImpl is the appropriate path forward. One can wrap the existing implementations of CacheImpl with try-catch blocks. If the implementation needs to be sensitive to start time, that seems doable with the same approach.

I don't believe it should require a change to the existing Cache implementation though since the current behavior adheres to the Principle Of Least Surprise.

from play1.

@theo Sorry for the delay.

My point is that it's not only DEV problem. It's also a problem in production.

Use case in production:

1. Play application (v.1) puts to cache some objects, say, instances of User class.
2. Admin installs a version 2 of application (where class User has been modified)
3. Play application (v.2) tries to get users from cache and gets million of deserialization errors in logs
4. But still, application load all users again and continuous working.

What I want is just to get rid of those million errors in logs.
Again, I agree that swallowing errors is not the best idea. But I currently don't see any other way.

NB! Please don't forget that cache is by design structure that can lose data. Absolutely every application should be designed so that it continues working if some data has been disappeared from cache.

from play1.

@asolntsev I understand the use case and I believe we are allowing the cache to lose data in all cases. I don't think we're arguing on correctness of the cache behavior. We're attempting to address verbosity of error logging. I believe that the decision to ignore or react to the exceptions is application concern, not a framework concern. The framework needs to raise those errors when they occur and the application needs to determine how it should behave based on those errors.

I assume we're talking about a specific cache implementation here (MemcachedImpl or EhCacheImpl) provided by Play.

I would support being able to enable or disable error logging to these implementations.

For MemCached, it would probably mean allowing to provide your own SerializingTranscoder which is responsible for how it reports errors. We could allow for MemCachedImpl.getInstance(SerializingTranscoder transcoder) and the application developer could provide a SerializingTranscoder that ignores any deserialization or serialization errors (or some other entirely alternative serialization method for that matter).

Alternatively, we could have getInstance() methods for both implementations take in a boolean, ignoreExceptions, and update those implementations to not log any exceptions if the flag is set to true (default: false).

If exception logging is the primary concern, I think the root cause is the implementation relies on the Play logger so any logging threshold you have is globally set to Play.logger and can't be targeted for just the Cache methods. Allowing for a flag to determine whether or not we log at all in those cache implementations would be a way to ignore errors. We could even make it mutable (although, I'm not sure if that would be a great idea) to support ignoring Exceptions on startup but have a job that runs X mins/seconds after startup to flip the flag.

Taking it a step further, we could create a different Logger (not use Play's Logger) so that log4j configuration can be set to ignore ERRORS. Something like Cache.Logger should be used in those implementations and applications can configure log4j with play.cache.Cache.Logger=OFF if they want to ignore exceptions.

Thoughts?

from play1.

@theo First of all, logging is not the primary concern. The primary concern is the title of current issue: "Play should NOT clear cache after every code change". Clearing cache makes development process much slower. And I know that Cache.clear(); was added just to avoid deserialization errors.

Probably it's reasonable:

I believe that the decision to ignore or react to the exceptions is application concern, not a framework concern.

But currently it's not true! Play cache implementation does catch deserialization errors and write errors in log. This is my problem: my application cannot determine how to behave in case of deserialization errors.

Yes, adding a method for providing custom SerializingTranscoder would solve my problem.

The following suggestions do not. You suggest to totally ignore some errors, but it's not a good idea. I don't want to ignore all caching errors. I only need to ignore objects that were put to cache by previous version of application.

from play1.

The primary concern is the title of current issue: "Play should NOT clear cache after every code change". Clearing cache makes development process much slower.

That seems solvable by the configuration flag: play.cache.clearOnStartup or some other name. Whether in DEV or PROD mode, Play can honor this flag on application start.

But currently it's not true! Play cache implementation does catch deserialization errors and write errors in log. This is my problem: my application cannot determine how to behave in case of deserialization errors.

I'm using the redis module (https://www.playframework.com/modules/redis) which does not catch exceptions without propagating. I believe the Play EhcacheImpl implementation propagates exceptions as well. There is nothing inherit in the CacheImpl API that says it should return null when exceptions occur.

The issue seems specific to the Memcached implementation. It seems reasonable to move forward by providing a MemcachedImpl implementation that propagates errors up via a custom SerializingTranscoder and setting Cache.forcedCacheImpl to that instance.

Because supporting this within Play itself will lead to having Play configuration specifying some class (ex: play.cache.memcache.transcoder=com.example.MySerializingTranscoder), I still believe having applications providing their own Cache implementation is the right approach. It gets unwieldy if com.example.MySerializingTranscoder requires constructor args.

Alternatively, we could update the current SerializingTranscoder to throw exceptions instead of log, but I'd be concern on backwards compatibility with lots of applications that currently rely on getting null instead of a thrown exception.

from play1.

@theo We discussed this problem with my colleagues and created another solution.
We will add git revision to all cache key.
It means that every new release in production will use its unique cache keys, and different releases will not causes deserialization errors. So, the problem is solved for production mode.

And I will send another pull request for development mode.
I agree with your suggestion to make "clean cache on every Play reload" a disableable feature. We will disable it via application.conf.

from play1.

That sounds great. To be clear, the revision-based key is an application-specific implementation and you are not suggesting to add it as a framework feature?

from play1.

Yes, exactly.

On Oct 21, 2016 2:28 AM, "Theodore Nguyen-Cao" [email protected]
wrote:

That sounds great. To be clear, the revision-based key is an
application-specific implementation and you are not suggesting to add it as
a framework feature?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#985 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARE3QPb1KX8-2ev8SpF0uzjCOu-UERlks5q1_kzgaJpZM4I_ika
.

from play1.

Merged with PR #999

from play1.