Skip to content
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.

Sign up for GitHub

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

Namespace should be determined by the Template's configuration (fixes multiple namespaces scenario) #269

Merged
merged 3 commits into from
Aug 17, 2021
Merged

Namespace should be determined by the Template's configuration (fixes multiple namespaces scenario) #269

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2631dde
Override the namespace, the aerospike template's namespace should be …
roimenashe Aug 9, 2021
6fc6ee8
Modify keyValueTemplateRef to aerospikeTemplateRef.
roimenashe Aug 9, 2021
ea7b251
* namespace should only be configured at the AerospikeTemplate level.
roimenashe Aug 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/main/java/org/springframework/data/aerospike/core/BaseAerospikeTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,12 @@ RuntimeException translateError(AerospikeException e) {
<T> AerospikeWriteData writeData(T document) {
AerospikeWriteData data = AerospikeWriteData.forWrite();
converter.write(document, data);

// Override the namespace, the aerospike template's namespace should be used in case its different from the persistent entity's namespace (default)
Copy link

@Aloren Aloren Aug 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the following issues with the code.

  1. User will not be able to use repositories anymore if he/she wants to save data into different namespaces.
  2. It looks like this is not the best place for this code to live in (since this is what converter is currently responsible for).

I think that the better approach would be to provide an additional step into the conversion logic:
https://github.com/aerospike-community/spring-data-aerospike/blob/master/src/main/java/org/springframework/data/aerospike/convert/MappingAerospikeWriteConverter.java#L77-L82

So that instead of:
data.setKey(new Key(entity.getNamespace(), entity.getSetName(), id));
we could have something like:

String namespace = namespaceProvider.ifPresent(p -> p.getNamespace(entity)).orElseGet(entity.getNamespace());

data.setKey(new Key(namespace, entity.getSetName(), id));

At a first glance it looks to me that this approach is more straightforward and also gives user possibility to still use repositories concept.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I don't see how it can happen, you cant use different namespaces with repositories ATM, each repository is configured with an AerospikeTemplate behind the scenes and the "defaultNamespace" (entity.getNamespace()) of the MappingAerospikeWriteConverter is using the write() method, the fix I added solves it by providing the namespace from the AerospikeTemplate (which is used by the repository) instead of using the entity's default, the example project I added demonstrates multiple repositories that uses different namespaces using this fix (https://github.com/roimenashe/spring-data-multiple-namespaces-new-example).

  2. I agree that the converter is more suitable place but, not every call to write() method is done by the AerospikeTemplate (BaseAerospikeTemplate), so we need to provide a namespace from each flow as you mentioned - namespaceProvider,
    I think we can also use the existing AerospikeWriteData class that is passed to the write() method, it contains a Key which has a namespace field, most flows are using forWrite() method of the AerospikeWriteData class that causes the key field to be null - so replacing the key with an edited key that has namespace (in the flows that can provide a namespace).
    similar to my initial solution - but, in the converter to add something like that:

String providedNamespace = null;
Key providedKey = data.getKey();
if (providedKey != null) {
	providedNamespace = providedKey.namespace;
}
data.setKey(new Key(providedNamespace != null ? providedNamespace : entity.getNamespace(), entity.getSetName(), id));

Meaning, use the provided key only incase its provided - otherwise use the entity.getNamespace() (the default MappingAerospikeContext namespace).

@Aloren What do you think?

Copy link

@Aloren Aloren Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. You gave me an idea that looks very obvious to me now and I think everybody will be happy with it.
How about we add an optional field namespace to the @Document annotation? Currently it gives an ability to specify only set name, but I don't see any issues with supporting a namespace as well. So it will look like that:

@Document(namespace = "namespace-v1", collection = "my-set")
public class MyDocumentV1 {}

@Document(namespace = "namespace-v2", collection = "my-set")
public class MyDocumentV2 {}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will also leave all logic for choosing needed namespace tied to the persistent entity.

Copy link
Member Author

@roimenashe roimenashe Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounded great for a second,
But i'm worried about couple of things:

  1. What if a user wants different configuration per namespace? for example, custom conversion for a specific namespace? in your solution 1 AerospikeTemplate will be used when writing to all namespaces, maybe users wants the flexibility of using more than 1 AerospikeTemplate (different policies, different conversions etc...).
  2. Namespaces are preconfigured in the aerospike.conf file - I get the feeling that it belongs at the configuration level (@Configuration) of the application, users will use this property in the @Document annotation and get a "namespace doesn't exists" exception, namespaces are not created on the flight as sets.

It is very intuitive place for the user but I think configuration of a template per namespace might be better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. What if a user configured both? persistent entity wins or an AerospikeTemplate wins? to which namespace the data will be written? I think we should go with the configuration one for flexibility but also clean and understandable.
    The idea of RepositoryOperationsMapping is nice and helps to map between an object and a configuration (AerospikeTemplate) but I think the current solution might be sufficient for now: define repositories, define configuration classes that links to that repositories (with @EnableAerospikeRepositories) and each configuration class extends the default data configuration beans, you can create an AerospikeTemplate of your own with the configurations/policies/converters you choose and specify it as a property (aerospikeTemplateRef) in the @EnableAerospikeRepositories annotation.
    We can definitely give it some thought - I'll consult with Ronen and Yevgeny about it.

  2. Yes that can happen here too, I just wanted to point out that beans in the application configuration feels more "linked" to something defined in the aerospike.conf file than a property at the @document annotation level, thats a very minor reason of course, the first one is much more meaningful.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I got your arguments. We can leave namespace handling on template side. Then shouldn’t we remove namespace handling from persistent entity completely?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think you are right - I don't see why the persistent entity should be aware of the namespace - an AerospikeTemplate is always configured with a namespace even a default single AerospikeTemplate, we can use it instead of the entity's namespace and completely remove it from the entity. I'll work on that and re-ask for a review.
Thanks a lot Anastasiia :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, this was disturbing me. Now if we will have namespace handling in one place everything will be clear. Thanks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed some changes and tested it (unit/integration tests and the example project), check out the commit comment I -added a detailed explanation there, I used something different since we are now relying that the namespace is provided from the outside (at the AerospikeTemplate level) since the converter is not exposed to the namespace directly.

if (!data.getKey().namespace.equals(getNamespace())) {
Key newKey = new Key(getNamespace(), data.getKey().setName, data.getKey().userKey);
data.setKey(newKey);
}
return data;
}

Expand Down