In-Memory Store Refactoring

[DominikPinsel]

As discussed here the proposal to refactor the in-memory store implementations.

In Memory Store Refactoring

At the time of writing we have 9 different in-memory store extensions.

Assumed issues

• It should not be necessary to create a new extension every time a different kind of object should be stored (for each type of storage).
• It should not be necessary to include several in-memory extensions to get a launcher or an integration test in a runnable state.
• Integrating an external storage system into the Eclipse Dataspace Connector should require to reference and configure only one extension.
• The interface to an external system should be really thin.

Why is this an improvement

• If its not necessary to create a new extension for each stored object & kind of storage
  • the total number of extensions would decrease.
  • the amount of duplicated code would decrease.
• If it would not be necessary to include several in-memory extensions to get a launcher or integration test running
  • it would increase the maintainability.
  • it would decrease complexity.
• If integrating an external storage system into the Eclipse Dataspace Connector would requires only one extension
  • external storage systems become easier to integrate.
  • the logic to access an external storage would not be spread over multiple extensions (single point of failure).
  • integration tests would need to fake less dependencies.
• If the interface to an external dependency would be really thin
  • it would make external dependencies easier fake-/mockable.
  • it would ensure that parts of the business logic stay consistent over different storage systems (e.g. query implementation of the AssetSelectorExpression in the AssetIndex).

Proposal

• Create generic persistence interface that is able to store and retrieve objects.
• Move the interfaces, that are currently implemented by the in-memory storages, to their corresponding core extensions.
• Implement a generic in-memory persistence, that may be used as fallback solution by the core extensions.
• Implement a generic azure cosmos persistence extension.

Please Note

• In this proposal it is not possible to use multiple storage systems at once. It’s probably not that much work to make this possible, too, but it would increase the amount of work of this already big refactoring. Therefore it was left out.

_{Dominik Pinsel [email protected], Daimler TSS GmbH, legal info/Impressum}

[ndr-brt]

I have a question about the "persistence interface", is possibile to have an example?

[DominikPinsel]

Maybe something like this

interface Identifiable {
    String getId();
}

interface DocumentStore {
    DocumentPartition getPartition(String name);
}

interface DocumentPartition {
    <T extends Identifiable> Stream<T> query(List<Criterion> criteria, Class<T> c);

    <T extends Identifiable> void update(T obj);

    <T extends Identifiable> void create(T obj);

    <T extends Identifiable> void delete(T obj);
}

[paullatzelsperger]

While I like the idea of having an in-memory storage "by default", there are a few false assumptions and problematic statements in this proposal. These are:

It should not be necessary to include several in-memory extensions to get a launcher or an integration test in a runnable state.

Tests are not what they are intended for. Tests should always use mocks/stubs.

Integrating an external storage system into the Eclipse Dataspace Connector should require to reference and configure only one extension.

Using an external storage only requires the extension in which that external storage api is implemented.

If its not necessary to create a new extension for each stored object & kind of storage

Even if we used something like a generic (as in Java Generics) storage, we would still have to define at least a marker class, otherwise
service lookup would not work. That's a good thing though because it increases explicitness. A separate extension would not be necessary though.

external storage systems become easier to integrate.

Why are they hard to integrate now? You add one extension, provide the necessary config values and you're done.

the logic to access an external storage would not be spread over multiple extensions (single point of failure).

It doesn't. Please give an example, where access logic spreads over >1 extension, except maybe for abstraction layers like the CosmosDbApi.

it would ensure that parts of the business logic stay consistent over different storage systems (e.g. query implementation of the AssetSelectorExpression in the AssetIndex).

No it would not, because different storage systems will still have to be implemented differently.

Create generic persistence interface that is able to store and retrieve objects.

I agree that a generic CRUD API might be convenient, and we might even get away with that as a basis, but neither will it be sufficient nor can we guarantee that the same implementation will fit all needs (e.g. save() vs create()+update()). So modifications, overrides and such will be necessary. At which point we can re-evaluate whether that API was necessary in the first place.

Implement a generic azure cosmos persistence extension.

We have that. It's called CosmosDbApi. It may not be function-complete yet, but it's there, and we use it extensively.

The core idea of this proposal, i.e. to unify and harmonize storage interfaces, is a good one and we should strive toward it, at least for rather trivial cases like CRUD operations. We need to be very careful not to stick to a pattern simply for pattern's sake.

Further, I agree that in-memory storages can be generalized, since most of them are Java Maps in one form or the other anyways and that those in-mem impls should be the "default".

I strongly disagree with the notion to have one in-memory extension that provides all in-mem-impls. That goes against the way we decided
to cut our code base, it would massively bloat the compiled code and it would simply move the decision out to the config, which is not where it belongs.

So here's my counter-proposal: why not have all features (such as the AssetIndex subsystem) provide an (in-mem) implementation, which is by default registered with the context. Extensions can then replace that with a specific one during their initialization, like CosmosDB. So we have sane defaults and an easy way of extending.
Sticking with the AssetIndex example that would mean that as long as the runtime has no asset-index storage extension defined, the in-mem one is used< and as soon as the build.gradle.kts defines a assetindex-XYZ extension, that one is used.

[DominikPinsel]

While I like the idea of having an in-memory storage "by default", there are a few false assumptions and problematic statements in this proposal. These are:

It should not be necessary to include several in-memory extensions to get a launcher or an integration test in a runnable state.

Tests are not what they are intended for. Tests should always use mocks/stubs.

Always stubbing and mocking in integration tests is not intended. Also the in-memory story is basically a fake implementation.

Integrating an external storage system into the Eclipse Dataspace Connector should require to reference and configure only one extension.

Using an external storage only requires the extension in which that external storage api is implemented.

But AssetIndex.Cosmos and TransferProcessStore.Cosmos would be already two extensions? Makes two extensions to use an Azure Cosmos DB (possibly more).

If its not necessary to create a new extension for each stored object & kind of storage

Even if we used something like a generic (as in Java Generics) storage, we would still have to define at least a marker class, otherwise service lookup would not work. That's a good thing though because it increases explicitness. A separate extension would not be necessary though.

I don't think so. In the proposal I made there would be only one

• AssetIndex
• TransferProcessStore
• Persistence Class
• etc.
  at the same time. Therefore the service lookup would work as intended.

external storage systems become easier to integrate.

Why are they hard to integrate now? You add one extension, provide the necessary config values and you're done.

I never said they are hard to integrate, but it would become easier.

the logic to access an external storage would not be spread over multiple extensions (single point of failure).

It doesn't. Please give an example, where access logic spreads over >1 extension, except maybe for abstraction layers like the CosmosDbApi.

You gave the example yourself. For other storage systems it would probably follow a similar pattern.
Example CosmosDb:

• CosmosDbApi
• TransferProcessStore.Cosmos
• ContractNegotationStore.Cosmos

Makes three extensions with knowledge about the Azure Cosmos Db.

it would ensure that parts of the business logic stay consistent over different storage systems (e.g. query implementation of the AssetSelectorExpression in the AssetIndex).

No it would not, because different storage systems will still have to be implemented differently.

Instead of having an implementation for criteria in the CosmosDbAssetIndex and possibly CosmosDbContractDefinitionStore etc. there would be only one in the CosmosDbPersistenceClass.

Create generic persistence interface that is able to store and retrieve objects.

I agree that a generic CRUD API might be convenient, and we might even get away with that as a basis, but neither will it be sufficient nor can we guarantee that the same implementation will fit all needs (e.g. save() vs create()+update()). So modifications, overrides and such will be necessary. At which point we can re-evaluate whether that API was necessary in the first place.

I would assume a CRUD API has proven its worth and would only use something different until proven otherwise.

Implement a generic azure cosmos persistence extension.

We have that. It's called CosmosDbApi. It may not be function-complete yet, but it's there, and we use it extensively.

So all my stores would use an azure cosmos DB by including the `CosmosDbApi´ extensions? This would be my proposal.

The core idea of this proposal, i.e. to unify and harmonize storage interfaces, is a good one and we should strive toward it, at least for rather trivial cases like CRUD operations. We need to be very careful not to stick to a pattern simply for pattern's sake.

Further, I agree that in-memory storages can be generalized, since most of them are Java Maps in one form or the other anyways and that those in-mem impls should be the "default".

I strongly disagree with the notion to have one in-memory extension that provides all in-mem-impls. That goes against the way we decided to cut our code base, it would massively bloat the compiled code and it would simply move the decision out to the config, which is not where it belongs.

I never suggested one in-memory extension in this proposal.

So here's my counter-proposal: why not have all features (such as the AssetIndex subsystem) provide an (in-mem) implementation, which is by default registered with the context. Extensions can then replace that with a specific one during their initialization, like CosmosDB. So we have sane defaults and an easy way of extending. Sticking with the AssetIndex example that would mean that as long as the runtime has no asset-index storage extension defined, the in-mem one is used< and as soon as the build.gradle.kts defines a assetindex-XYZ extension, that one is used.

Using a default in-memory implementation instead of extensions is already part of this proposal.

[paullatzelsperger]

But AssetIndex.Cosmos and TransferProcessStore.Cosmos would be already two extensions? Makes two extensions to use an Azure Cosmos DB (possibly more).

They have different interfaces, different methods. There can be a unified persistence layer though, which is the CosmosDbApi, but satisfying both the TPS and the AssetIndexCosmos in only one extension is not going to work and makes no sense.

I don't think so. In the proposal I made there would be only one...

So if I understand correctly the AssetIndex itself would not directly handle persistence, but would delegate to a persistence object? Sounds similar to JPA's Repository concept - right?

Makes three extensions with knowledge about the Azure Cosmos Db.

nope - makes one extension, the CosmosDbApi. It would contain all cosmos-db SDK dependencies, and handle the CosmosClient, etc. The others would simply use the CosmosDbApi. If that had a more general interface it would also have to be named "cosmos...".
We would have to be very cautios in dealing with e.g. concurrent-locking issues.

Instead of having an implementation for criteria in the CosmosDbAssetIndex and possibly CosmosDbContractDefinitionStore etc. there would be only one in the CosmosDbPersistenceClass.

And probably one in all the other *DbPersistenceClasses, right? Assuming we have those ofc.

I never suggested one in-memory extension in this proposal.

OK, might have inferred from previous discussions.

Do I understand correctly that you want to separate persistence into two things, a "service" and a "repository", the former would contain business logic and the latter the actual persistence operations?
(I'm calling them "service" and "repository" because this feels very Spring-Data-y and that's what people call them there).

[DominikPinsel]

I will just continue the discussion by highlighting things where the suggested abstraction layer would have helped.

Consistency

@MikhailGordienk Created a discussion #387 that there is no possibility to remove data. If the functionality would be added now there is a possibility that it may not be implemented for all kind of storage systems.
If there would be some kind of abstraction layer/interface, like suggested in one of my earlier comments,

• it would be ensured by design that all storage systems share the same functionality at all times.
• the total number of extensions would not increase

Duplications

Another Issue is there with my own pull-requests, that implement different kind of storage interface extensions for PostgresSQL (AssetIndex, ContractDefinitionStore, AssetLoader, etc.).
All these extensions must come with their own settings (by EDC design). This means, each extension has identical settings factories, unit tests, documentation etc.
See #458 #459 #455

_{Dominik Pinsel [email protected], Daimler TSS GmbH, legal info/Impressum}

[jimmarino]

@DominikPinsel I would suggest you discuss with @paullatzelsperger. Specifically, he stated:

The core idea of this proposal, i.e. to unify and harmonize storage interfaces, is a good one and we should strive toward it, at least for rather trivial cases like CRUD operations. We need to be very careful not to stick to a pattern simply for pattern's sake.

Further, I agree that in-memory storages can be generalized, since most of them are Java Maps in one form or the other anyways and that those in-mem impls should be the "default".

The first point has already been implemented with 'CosmosDbApi' and could serve as a pattern for other backing stores.

As I mentioned in #387, asset removal has very unique requirements from a generalized CRUD repository.

[paullatzelsperger]

These are two distinct problems:

1. introduce a somewhat generic persistence interface, which more technology-specific variants like the CosmosDbApi (or a PostgresDbApi, etc.) could then extend and implement
2. provide an in-mem implementation by default, which can then be replaced by other extensions. I guess we can generalize a lot of those in-mem implementations, since they are likely little more than lists or maps, maybe even use Java generics.

Lets not mix up those two, and lets also be careful to copy other DAO/persistence implementations simply because of familiarity.

The CosmosDbApi solves all of the aforementioned problems like configuration, so we should use that as a starting point. If we find it lacking, we can always improve.

[DominikPinsel]

The CosmosDbApi solves all of the aforementioned problems like configuration, so we should use that as a starting point. If we find it lacking, we can always improve.

I think you are mostly right. But I don't think the CosmosDbApi extension solves the described problems.

1. You created a AbstractCosmosConfig class, that does all the config stuff for you. But it violates the EDC Architecture Principles, therefore I assume this is not a real solution.

Architecture Principles I.3. 
Service configuration requirements should always be explicit; as a general rule, do not pass a single configuration object with many values to multiple services.

2. I would try to avoid leaking too much EDC information into a library extension. I think its better design if the libraries are mostly independent.

3. You were still not able to solve the documentation problem. Now some CosmosDb Extensions have the configuration parameters documented in their README.md (but duplicated), and some are missing them completly. There we see the two problems with inconsistency and duplication combined.