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

Delete Sessions which cause deserialization errors #36

Merged
merged 1 commit into from
Jan 19, 2014

Conversation

marcust
Copy link
Contributor

@marcust marcust commented Jan 15, 2014

Warum ist das nicht so? Also in Hunter können wir Appserver gut damit auslasten nicht kompatible Sessions zu deserialisieren und ich denke nicht das das alles initial fails sind sonder auch ganz viele die bis zum nächsten Update der Session im Memcached die letzte Session versuchen zu deserialisieren.

Trying to deserialize sessions that are for some reason not
deserializable leads to continous retries at the moment.

As the data is apparently not loadable, it might be a better idea
to remove the data from memcached alltogether in order to avoid
expensive retries.

@magro
Copy link
Owner

magro commented Jan 15, 2014

For deserialization error this makes sense (you probably would also want to add the session id to the invalidSessionsCache), but not for other issues like a timeout. So this case should be handled more specifically and the session should not be deleted for all exceptions.
Having a test for this would be uber cool! :-)

Trying to deserialize sessions that are for some reason not
deserializable leads to continous retries at the moment.

As the data is apparently not loadable, it might be a better idea
to remove the data from memcached alltogether in order to avoid
expensive retries.
@marcust
Copy link
Contributor Author

marcust commented Jan 16, 2014

Added the session Id to the invalidSessionCache and made the exception more specific.

Furthermode I added a simple test for the case of a desiralization error, a more complete integration test is out of scope at the moment.

@magro
Copy link
Owner

magro commented Jan 16, 2014

Looks good, nice test :-)

The TranscoderDeserializationException should also be thrown by other transcoders, but I can do this as well once I'm back from holidays.

@magro magro merged commit 83d9289 into magro:master Jan 19, 2014
@magro
Copy link
Owner

magro commented Jan 19, 2014

Thanx for the PR, I changed other transcoders to throw the TranscoderDeserializationException as well.

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