Add support for count projections and KeySpaceStore abstraction
#654
Conversation
Add KeySpaceStore to retrieve a Map for each keyspace allowing to use a different implementation instead of using generic Java maps. Also, allow configuring KeySpaceStore as bean.
Use RepositoryConfigurationSource for annotation lookup directly.
| * @since 4.0 | ||
| */ | ||
| @SuppressWarnings("rawtypes") | ||
| record MapKeySpaceStore(Map<String, Map<Object, Object>> store, Class<? extends Map> keySpaceMapType, |
There was a problem hiding this comment.
Interesting use of record as the impl for the store. Any limitations of using record for this? Will the equals auto-implemented by record be sufficient? It may go through the entire store contents to check equality.
There was a problem hiding this comment.
Using records as interface implementations is a neat pattern as our API surface is limited by the interface. Clearly, Object methods are inherited and their functionality can be surfaced in various ways. We don't define a contract regarding equality and hash code but we might fill in these blanks as we see fit.
| * @param store reference to the map of maps holding the keyspace data. | ||
| * @param keySpaceMapType map type to be used for each keyspace map. | ||
| * @param initialCapacity initial keyspace map capacity to optimize allocations. | ||
| * @since 4.0 |
| */ | ||
| public MapKeyValueAdapter() { | ||
| this(ConcurrentHashMap.class); | ||
| this(MapKeySpaceStore.create()); |
There was a problem hiding this comment.
Any reason to not use the KeySpaceStore static factory methods?
There was a problem hiding this comment.
Initially, a left-over from evolving the interface while moving things around. It lead to some more thoughts on the design and I refined create vs of method naming removing the default ConcurrentHashMap convenience method at KeySpaceStore. We can add it later if necessary and for the time being, we can be less opinionated with the public API while retaining some convenience in our internal code. It's an assumption in the same sense as MapKeySpaceStore is good enough for our own perspective on map storage making it sufficient to pass all tests. Yet, I would not want to expose MapKeySpaceStore as public API because it is an implementation detail. If we see need to evolve an API around the storage itself, then we can take action, but for now, less exposure gives us more flexibility and less API surface.
| Optional<String> keySpaceStoreRef = source.getAttribute("keySpaceStoreRef", String.class); | ||
|
|
||
| return (Class<? extends Map>) getAnnotationAttributes(source).get("mapType"); | ||
| return keySpaceStoreRef.map(beanName -> new RuntimeBeanReference(beanName, KeySpaceStore.class)) // |
There was a problem hiding this comment.
I meant to ask (in general) the ending the line w/ // I am guessing makes the formatter not reformat the lines smashed onto a single line and respects the multi-line fluent style that is soothing on the eyes. Is this correct?
There was a problem hiding this comment.
trailing comments force the formatter to keep line breaks. I know that's a style one has to get used to. The motivation is that we want our formatter to generally reformat linebreaks unless we explicitly want to take control. NOFORMAT blocks apply to all aspects of formatting and that is too much exclusion for aspects that matter.
| * @author Mark Paluch | ||
| */ | ||
| @SpringJUnitConfig | ||
| class MapDbIntegrationTests { |
There was a problem hiding this comment.
That has been the actual motivation to explore a MapDB integration. MapDB seems to face the face of not being maintained anymore (at least that's my perception) so the outcome of this exercise is an SPI verified against MapDb.
| assertThat(adapter).isInstanceOf(MapKeyValueAdapter.class); | ||
| assertThat(ReflectionTestUtils.getField(adapter, "store")).isInstanceOf(mapType); | ||
|
|
||
| KeySpaceStore store = (KeySpaceStore) ReflectionTestUtils.getField(adapter, "store"); |
There was a problem hiding this comment.
This can also be done w/ assertJ directly as it can dig into the fields w/ something like:
assertThat(adapter)
.extracting("store.store")
.isInstanceOf(KeySpaceStore.class);I am guessing you already know this and prefer the more direct error that will result if/when one of the reflected fields is not available?
|
LGTM |
Original Pull Request: #654
Original Pull Request: #654
Use RepositoryConfigurationSource for annotation lookup directly. Original Pull Request: #654
3 participants
Allow a pluggable store for keyspace storage. Also, add missing functionality.
Closes #71