updated redis version and provided serialization option#1710
Merged
rfecher merged 1 commit intolocationtech:masterfrom Apr 22, 2020
Merged
updated redis version and provided serialization option#1710rfecher merged 1 commit intolocationtech:masterfrom
rfecher merged 1 commit intolocationtech:masterfrom
Conversation
c64daa9 to
6c9f303
Compare
Signed-off-by: Rich Fecher <rich.fecher@blacklynx.tech>
6c9f303 to
5f4545e
Compare
jdgarrett
approved these changes
Apr 22, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There are several github issues and stackoverflows referring to a memory leak when using FST with redisson. We were seriously running up against this issue and despite what this [1] says, the latest version does not simply resolve it. However, I figure it won't hurt to update to the latest version. We actually only use a provided serialization codec for the data index when secondary indexing is enabled - unfortunately that is our use case. As it stood GeoWave just always uses FST codec for that, which is Redisson's default codec. The only thing GeoWave is serializing for the data index is a
byte[]so its very straightforward - I'm not sure the advantages/disadvantages of different codec when serializing an object as simple as an array of primitives. My first instinct was to just have our own custom codec just like we have our own codec for every other redisson collection such as the index rows and the metadata rows. However, maintaining complete backwards compatibility with any data store prior to this PR would mean that whatever new code path that circumvents this FST codec issue has to be optional and the FST codec has to be the default behavior. So then I provided the option for a series of 9 serialization options from this list [2]. However, when running with that there'd be a ClassNotFoundException on a Kryo class from our apps without adding more to the classpath which seemed odd (even though we weren't using Kryo and I was using a Supplier interface so no instantiation happened statically it was just resolving the codec class). Considering this is only to serialize abyte[]for secondary indexing and I don't really know. the advantage of using any of these codecs aside from avoiding memory leaks with FST, I backed it down to whats in this PR, using FST by default and the JDK serialization as an option. We enable JDK serialization through that option and the memory leaks go away so in the end I am sufficiently happy with giving the JDK option, and should we choose to add more, its very easy (without benchmarks I'm not sure to what end though).[1] redisson/redisson#1927
[2] https://github.com/redisson/redisson/wiki/4.-data-serialization