FMWK-278 Refactor configuration #684

agrgr · 2024-01-01T18:48:42Z

No description provided.

…perties (has precedence)

roimenashe · 2024-01-03T08:30:51Z

src/main/java/org/springframework/data/aerospike/utility/Utils.java

+        return null;
+    }
+
+    public static String getNamespace(String prioritizedNamespace, String fallbackNamespace) {


This is because a namespace can be configured with both application.properties and @OverRide? but when a user wants to override nameSpace() he override nameSpace() (with only 1 param) right? its completely internal

Yes, and it is the same mechanism we had before this change, so those who don't use application.properties (let's say, they use default settings) will not feel any change.
On the other hand, if they opt to use application.properties, it will have precedence over overridden namespace and/or hosts.

roimenashe · 2024-01-03T08:32:38Z

src/main/java/org/springframework/data/aerospike/config/AbstractAerospikeDataConfiguration.java

-        return new AerospikeTemplate(aerospikeClient, nameSpace(), mappingAerospikeConverter,
-            aerospikeMappingContext, aerospikeExceptionTranslator, queryEngine, indexRefresher, serverVersionSupport);
+                                               ServerVersionSupport serverVersionSupport, AerospikeSettings settings) {
+        return new AerospikeTemplate(aerospikeClient, getNamespace(settings.getNamespace(), nameSpace()),


The only aerospikeSettings we need in template is currently namespace but it makes sense to pass the entire aerospikeSettings to the template for more modular code flow (in case we want to use an aerospike setting in the template), WDYT?

Overall agree, but it might be more suitable to do in a separate ticket.

roimenashe · 2024-01-03T08:38:46Z

src/main/resources/application.properties

@@ -0,0 +1,10 @@
+spring-data-aerospike.hosts=localhost:3000
+spring-data-aerospike.namespace=TEST


I would use lowercase test as a default namespace

Agree, will use lowercase here (if there is any difference). However, the embedded testcontainer has its namespace configured as "TEST".

I know, but in most aerospike documentation we use lowercase so I figure thats the go-to naming convention we should align with, will probably be more clean and less confusion for the users.

I've also made this change in the Spring Data Aerospike README and in the testers project but it got lost in the testers project since we worked on the same branch and probably overriding the change. I want to align it in the next release of starters as well (so everything will be aligned Aerospike official docs, Spring Data Aerospike, Spring Data Aerospike Starters).

roimenashe · 2024-01-03T08:39:50Z

src/test/java/org/springframework/data/aerospike/BaseIntegrationTests.java

@@ -13,7 +13,7 @@ public abstract class BaseIntegrationTests {
    public static final String OVERRIDE_SET_NAME = "testSet1";
    protected static final int MILLIS_TO_NANO = 1_000_000;

-    @Value("${embedded.aerospike.namespace}")
+    @Value("${spring-data-aerospike.namespace}")


In tests I'm not sure we want to change embedded name since we are using Playtika's embedded-aerospike

This is done for unification in the tests, we now read the same parameter from test application.properties rather than have scattered readings of embedded namespace and hosts.
Embedded Aerospike namespace and hosts are now specified only in one place - the application.properties file.

I just read embedded-aerospike documentation, it's actually produces embedded.aerospike.namespace so you can use spring-data-aerospike.namespace and its basically the same thing

roimenashe · 2024-01-03T08:42:37Z

src/test/java/org/springframework/data/aerospike/config/CommonTestConfig.java

@@ -42,7 +42,7 @@
 @EnableAutoConfiguration
 public class CommonTestConfig {

-    @Value("${embedded.aerospike.namespace}")


Same here, why remove embedded if it refers to embedded-aerospike namespace config?

We now have it in application.properties.

roimenashe · 2024-01-03T08:45:07Z

src/test/java/org/springframework/data/aerospike/repository/PersonRepositoryQueryTests.java

@@ -2271,7 +2267,7 @@ void findPersonsByFriendFriendAddressZipCode() {
    }

    @Test
-    // find by deeply nested String POJO field
+        // find by deeply nested String POJO field


Redundant tabs/spaces

Changing, was autoformatted this way.

roimenashe · 2024-01-03T08:45:12Z

src/test/java/org/springframework/data/aerospike/repository/PersonRepositoryQueryTests.java

@@ -2310,7 +2306,7 @@ void findPersonsByFriendFriendFriendFriendFriendFriendFriendFriendBestFriendFrie
    }

    @Test
-    // find by deeply nested Integer POJO field
+        // find by deeply nested Integer POJO field


Redundant tabs/spaces

roimenashe · 2024-01-03T08:45:17Z

src/test/java/org/springframework/data/aerospike/repository/PersonRepositoryQueryTests.java

@@ -2349,7 +2345,7 @@ void findPersonsByFriendFriendFriendFriendFriendFriendFriendFriendBestFriendAddr
    }

    @Test
-    // find by deeply nested POJO
+        // find by deeply nested POJO


Redundant tabs/spaces

roimenashe · 2024-01-03T08:47:35Z

src/main/java/org/springframework/data/aerospike/config/AerospikeSettings.java

+
+@Setter
+@Getter
+public class AerospikeSettings {


If we have no default values here and we put them in application.properties, the user will have to define values for each of these in his application.properties.

I think this class must have values for each field except hosts and namespaces and only in the application.properties of the /tests we will have test namespace and localhost:3000

Agreed, bringing the defaults back.

roimenashe · 2024-01-03T08:49:28Z

Changing aerospikeDataSettings method to aerospikeSettings bean introduces a breaking change that is very likely to impact users, need to see how to make it as smooth as possible (rethinking, 2 config options [with @deprecated?], documentation,...)

agrgr · 2024-01-03T10:30:11Z

Changing aerospikeDataSettings method to aerospikeSettings bean introduces a breaking change that is very likely to impact users, need to see how to make it as smooth as possible (rethinking, 2 config options [with @deprecated?], documentation,...)

I think there is sense to address it via a blog post and/or an article on developer hub, along with having comments in the code. Users who don't use settings other than hosts and namespace will not feel the change because we still support overriding these values. Others will need to do some changes anyway (remove configureDataSettings() and use application.properties), which is (hopefully) straightforward.

roimenashe · 2024-01-03T12:26:25Z

src/main/java/org/springframework/data/aerospike/utility/Utils.java

+    public static Collection<Host> parseHosts(String hostsString) {
+        if (StringUtils.hasText(hostsString)) return Arrays.stream(hostsString.split(","))
+            .map(host -> host.split(":"))
+            .map(hostArr -> new Host(hostArr[0], Integer.parseInt(hostArr[1])))


This doesn't support TLS.

Host can be constructed of:

Host name/IP address

tls name (optional)

port

Previously we override getHosts() and created an Host object via code which could then be created with TLS.

I think that application.properties parsing automatically supports List instead of String which makes it easier to parse. and we should parse each String (Host) with the following logic:

If it contains a single : it means no tls configured.

If it contains two :'s it means TLS is configured so create an Host object with TLS.

Added using Java client's method for hosts parsing which includes reading optional TLS name.

reugn · 2024-01-07T16:35:01Z

src/main/java/org/springframework/data/aerospike/config/AerospikeDataConfigurationSupport.java

+     *
+     * @return Collection of Host objects for Aerospike client to connect
+     */
+    protected AerospikeDataSettings configureDataSettings() {


WDYT about renaming it to dataSettings?

I am ok with it, but this is the method we have previously had. I think it is better to have the same name.

reugn · 2024-01-07T16:36:24Z

src/main/java/org/springframework/data/aerospike/config/AerospikeDataConfigurationSupport.java

-        clientPolicy.writePolicyDefault.sendKey = aerospikeDataSettings().isSendKey();
-        clientPolicy.readPolicyDefault.sendKey = aerospikeDataSettings().isSendKey();
+        clientPolicy.writePolicyDefault.sendKey = true;
+        clientPolicy.readPolicyDefault.sendKey = true;


Add

clientPolicy.batchPolicyDefault.sendKey = true;

reugn · 2024-01-07T16:41:52Z

src/main/java/org/springframework/data/aerospike/config/AerospikeDataConfigurationSupport.java

-        clientPolicy.writePolicyDefault.sendKey = aerospikeDataSettings().isSendKey();
-        clientPolicy.readPolicyDefault.sendKey = aerospikeDataSettings().isSendKey();
+        clientPolicy.writePolicyDefault.sendKey = true;
+        clientPolicy.readPolicyDefault.sendKey = true;
        log.debug("AerospikeDataSettings.sendKey: {}", clientPolicy.writePolicyDefault.sendKey);


This log print is no longer relevant

reugn · 2024-01-07T16:42:59Z

src/main/java/org/springframework/data/aerospike/config/AerospikeDataConfigurationSupport.java

+        }
+
+        // getHosts() return value has precedence over hosts parameter from application.properties
+        if (getHosts() != null) {


It's better to read getHosts() and nameSpace() into a variable once

reugn · 2024-01-07T16:44:10Z

src/main/java/org/springframework/data/aerospike/config/AerospikeSettings.java

+import lombok.Data;
+
+@Data
+public class AerospikeSettings {


Add a brief Javadoc

reugn · 2024-01-07T16:45:52Z

src/main/java/org/springframework/data/aerospike/config/AerospikeDataSettings.java

    // Send user defined key in addition to hash digest on both reads and writes
-    boolean sendKey = true;
-    @Builder.Default
+    boolean sendKey = false;


It is true by default in the properties file

… from AerospikeDataSettings

… into FMWK-278-external-configuration

refactor configuration, read from application.properties

dd86104

agrgr added the enhancement label Jan 1, 2024

agrgr requested review from roimenashe and reugn January 1, 2024 18:48

cleanup, roll back getClientPolicy() method

0ff2585

agrgr marked this pull request as ready for review January 2, 2024 13:16

support getHosts() and nameSpace() methods along with application.pro…

1976133

…perties (has precedence)

roimenashe reviewed Jan 3, 2024

View reviewed changes

add default values for settings, cleanup

a3e0a0d

roimenashe reviewed Jan 3, 2024

View reviewed changes

agrgr added 4 commits January 3, 2024 19:09

parse hosts using Java client's method, documentation update

0a71d01

cleanup

5be0c15

rename settings class, documentation update

d3246e0

cleanup

d3ab858

roimenashe approved these changes Jan 7, 2024

View reviewed changes

agrgr added 3 commits January 7, 2024 14:49

set priority to configuration methods

80ff22a

cleanup

66e8dea

cleanup, documentation update

d0c3b57

reugn reviewed Jan 7, 2024

View reviewed changes

cleanup

872fbb1

agrgr and others added 4 commits January 8, 2024 11:06

support modular override of data settings, remove 'sendKey' parameter…

a8290b3

… from AerospikeDataSettings

replace overrideable method for data settings, cleanup

0877dd8

cleanup

6df65db

add copyright headers

701861e

reugn approved these changes Jan 8, 2024

View reviewed changes

agrgr and others added 3 commits January 8, 2024 15:31

remove sendKey from application.properties

2c50f58

Merge remote-tracking branch 'origin/FMWK-278-external-configuration'…

2cc2cb9

… into FMWK-278-external-configuration

Merge branch 'main' into FMWK-278-external-configuration

925cd78

agrgr merged commit 8ef5cc0 into main Jan 8, 2024
10 checks passed

agrgr deleted the FMWK-278-external-configuration branch January 8, 2024 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FMWK-278 Refactor configuration #684

FMWK-278 Refactor configuration #684

agrgr commented Jan 1, 2024

roimenashe Jan 3, 2024

agrgr Jan 3, 2024 •

edited

Loading

roimenashe Jan 3, 2024

agrgr Jan 3, 2024

roimenashe Jan 3, 2024

agrgr Jan 3, 2024

roimenashe Jan 3, 2024 •

edited

Loading

roimenashe Jan 3, 2024

agrgr Jan 3, 2024

roimenashe Jan 3, 2024

roimenashe Jan 3, 2024

agrgr Jan 3, 2024

roimenashe Jan 3, 2024

agrgr Jan 3, 2024

roimenashe Jan 3, 2024

roimenashe Jan 3, 2024

roimenashe Jan 3, 2024

agrgr Jan 3, 2024

roimenashe commented Jan 3, 2024

agrgr commented Jan 3, 2024

roimenashe Jan 3, 2024

agrgr Jan 3, 2024

reugn Jan 7, 2024

agrgr Jan 7, 2024 •

edited

Loading

reugn Jan 7, 2024

reugn Jan 7, 2024

reugn Jan 7, 2024

reugn Jan 7, 2024

reugn Jan 7, 2024

		@@ -0,0 +1,10 @@
		spring-data-aerospike.hosts=localhost:3000
		spring-data-aerospike.namespace=TEST

FMWK-278 Refactor configuration #684

FMWK-278 Refactor configuration #684

Conversation

agrgr commented Jan 1, 2024

Choose a reason for hiding this comment

agrgr Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roimenashe Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roimenashe commented Jan 3, 2024

agrgr commented Jan 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrgr Jan 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrgr Jan 3, 2024 •

edited

Loading

roimenashe Jan 3, 2024 •

edited

Loading

agrgr Jan 7, 2024 •

edited

Loading