Feature/172 create assetloader #199
Conversation
...c/test/java/org/eclipse/dataspaceconnector/metadata/memory/InMemoryAssetIndexLoaderTest.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/eclipse/dataspaceconnector/metadata/memory/InMemoryAssetIndexLoaderTest.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/eclipse/dataspaceconnector/metadata/memory/InMemoryAssetIndexLoaderTest.java
Show resolved
Hide resolved
...c/test/java/org/eclipse/dataspaceconnector/metadata/memory/InMemoryAssetIndexLoaderTest.java
Outdated
Show resolved
Hide resolved
spi/src/main/java/org/eclipse/dataspaceconnector/spi/asset/AssetIndexLoader.java
Outdated
Show resolved
Hide resolved
| private final CriterionToPredicateConverter predicateFactory; | ||
| private final ReentrantLock lock; |
There was a problem hiding this comment.
why do we need this lock in addition to ConcurrentHashMaps? Just to ensure both Maps stay consistent?
If so maybe a ReentrantReadWriteLock would be better... but just a minor-minor improvement for an extension that shouldn't be used in production...
There was a problem hiding this comment.
Are you after the supposed added performance since we're expecting much more read operations than write operations?
There was a problem hiding this comment.
yes. but more than that I wanted to ensure I understood why we need the lock at all? Is it for consistency over those two maps?
There was a problem hiding this comment.
If so my next question would be: why is DataAddress not just an attribute at Asset? From a DDD perspective: does a DataAddress has it's own identity? Don't think so - but maybe I'm wrong...
There was a problem hiding this comment.
Actually looking from a DDD perspective the Asset is an entity, it has an identity, it has an history and, why not, their attributes could be modified without changing its identity.
Seems like DataAddress in this context is more of a value object, because it is just related to the Asset, and in fact it does not own an identity so I agree with your reasoning @MoritzKeppler .
Don't remember why we extracted DataAddress from Asset...
There was a problem hiding this comment.
I would not model the DataAddress as a property of the Asset, because the DataAddress will probably private information - we wouldn't want to expose where our data is stored. Thus, if the DataAddress is a property on the Asset, we would have to prevent exposing it during serialization, which I find dangerous.
There was a problem hiding this comment.
What we offer to outside is e.g. an IDS Resource or something similar. In my eyes it would be an antipattern to provide the internal model 1:1 to the world outside.
There was a problem hiding this comment.
But a DataAdress can also be used in the information passed to others: it that case it would be the address under which the other participant can retrieve the data.
As soon as a consumer retrieved an asset it will be added to an internal catalog (as it now needs to be managed) and will get a DataAddress pointing to the local copy of the asset.
There was a problem hiding this comment.
But a DataAdress can also be used in the information passed to others: it that case it would be the address under which the other participant can retrieve the data.
that would imply giving consumers direct access to, say, a database, right? Because I think the DataAddress would contain database name, etc...
As soon as a consumer retrieved an ...
sure, but then there will also be a different instance of the Asset within the consumer's realm, even if that may contain the same metadata.
There was a problem hiding this comment.
started a discussion, maybe we can copy over the different views.
#214
paullatzelsperger
force-pushed
the
feature/172-create-assetloader
branch
from
910863c
to
6a85b55
Compare
This PR implements an
AssetIndexLoaderplus an in-memory implementation for that.Since the loader must guarantee consistency between an
Assetand the correspondingDataAddress, the in-mem implementation also implements theAssetIndexandDataAddressResolverinterfaces.Fixes issue #172