-
Notifications
You must be signed in to change notification settings - Fork 28
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
FMWK-459 Fix requiring id property from entities by AerospikeCache #752
Conversation
@agrgr As we talked about, I'm not sure we want to allow it - if new versions of Spring Data Aerospike doesn't allow to store/read a POJO without @id field why would the Spring Data Cache be any different? Also according to AerospikeCache's code we are always treating the given key as an Aerospike Key so it doesn't support cache by bins or basically anything other than the Aerospike Key itself (it probably shouldn't). I don't think this change makes much sense, we need to ask the guys who requested this change for their motivation - if we missing a validation or accidentally broke the API that's a different story and can be fixed in a different way (possibly a simple upgrade to latest) but I think we shouldn't include this change and keep validating @id when interacting with Spring Cache (otherwise providing a none PK field would still fail but with less self-explanatory error). |
Asked them on GitHub, let's freeze this PR for now and wait. We will also continue discussions in the meantime. |
@agrgr After consideration, let's go with this approach which says Spring Data and Spring Cache are separated and we are not enforcing schema annotations in Spring Cache (in this case @id) and set the key from the given key directly. Once you change this PR to "Ready for review" I'll review and let's merge |
Added using plain hash codes from Object, let's discuss whether to use cryptographic hash functions or external libraries etc. to approach the balance of computational complexity and collision resistance. |
@agrgr Few questions about the new hashcode change:
|
|
… hashing methods overridable
@agrgr Are we sure we want to introduce |
@roimenashe, as far as I can say, it actually is the most popular vulnerabilities-free (at least, at the moment) xxHash library for Java. Despite the latest release in 2021, it does not have notifications about stopped support, and the latest commit is a year ago. Besides, we would be able to replace this implementation with another one if needed as AerospikeCacheKeyProcessor is designed to return AerospikeCacheKey which is implementation-agnostic (can take a String or a long number). It looks like Apache Commons has 32 bit xxHash implementation, however, it has no 64 bit, and it is not mentioned on xxHash page. Let's discuss, theoretically we can introduce something else instead of very fast xxHash (like Murmur3), however, not sure whether the fact that the library had latest release 3 years ago is decisive. |
@roimenashe, replaced hashing with Apache's Murmur3 128 bit. The library looks up-to-date, they have a recent release. |
# Conflicts: # src/test/java/org/springframework/data/aerospike/cache/AerospikeCacheManagerIntegrationTests.java
… different implementation if needed, cleanup
Tested overriding AerospikeCacheKeyProcessor bean with a custom implementation in a demo project |
AerospikeCacheConfiguration defaultCacheConfiguration) { | ||
this(aerospikeClient, aerospikeConverter, defaultCacheConfiguration, new LinkedHashMap<>()); | ||
AerospikeCacheConfiguration defaultCacheConfiguration, | ||
AerospikeCacheKeyProcessor cacheKeyProcessor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding AerospikeCacheKeyProcessor
to both constructors introduces a breaking change (the user needs to add this param when defining AerospikeCacheManager
bean).
We probably need to update the "Caching with Spring Boot and Aerospike" blog posts:
public class AerospikeCacheKeyProcessorImpl implements AerospikeCacheKeyProcessor { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this class is a singleton so why not use a bean by marking this class as @Component
, to execute configureKryo()
on initialization we can use @PostConstruct
or an alternative
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sense here is this class is an implementation of AerospikeCacheKeyProcessor. If users want to use a different implementation, they can do so by overriding AerospikeCacheKeyProcessor bean
/** | ||
* Wrapper class used for caching with methods that receive either a String or a long number | ||
*/ | ||
public class AerospikeCacheKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only String
and long
? if the purpose is to align with Aerospike supported PK types - "blob" (byte[]
) should be supported as well: https://aerospike.com/docs/reference/limitations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a wrapper class that receives hash of the cache key. However, we can also support byte array, added it
No description provided.