Add new method(overload) for CentralDogmaPropertySupplier#createPropertyFile/register #134

lazmond3 · 2021-12-02T09:31:54Z

Relevant Issue

Motivation

CentralDogmaPropertySupplier#register is used to create a property file with default settings.
The existing implementation of the function does not accept user-customized arguments.

If you know the appropriate values at the beginning and want to customize your settings, you need to take extra steps.

First, create a property file with CentralDogmaPropertySupplier#register.
Next, modify the values on CentralDogma.

This extra step is a bit annoying, so we want to let the function accept user-customized settings.

Summary of Changes

overload two new methods in CentralDogmaPropertySupplier
1. register accepts StaticPropertySupplier supplier
  1. register converts supplier to List<Property<?>> for providing values to the next function: createPropertyFile.
2. createPropertyFile accepts List<Property<?>> properties.

Usage Example

CentralDogmaPropertySupplier.register(
    dogma,
    "sample-project",
    "app-using-decaton",
    "/decaton/settings.json",
    StaticPropertySupplier.of(
        Property.ofStatic(
            ProcessorProperties.CONFIG_PARTITION_CONCURRENCY,
            100
        ),
        Property.ofStatic(
            ProcessorProperties.CONFIG_MAX_PENDING_RECORDS,
            10000
        )
    )
)

CLAassistant · 2021-12-02T09:32:00Z

All committers have signed the CLA.

kawamuray

Thanks for working on this! Left a minor comment WRT interface, but overall looks good 👍

kawamuray · 2021-12-03T10:38:06Z

centraldogma/src/main/java/com/linecorp/decaton/centraldogma/CentralDogmaPropertySupplier.java

@@ -145,19 +149,43 @@ public static CentralDogmaPropertySupplier register(CentralDogma centralDogma, S
        return new CentralDogmaPropertySupplier(centralDogma, project, repository, filename);
    }

+    public static CentralDogmaPropertySupplier register(CentralDogma centralDogma, String project,
+                                                        String repository, String filename,
+                                                        StaticPropertySupplier supplier) {


I think in most cases people would use StaticPropertySupplier anyway, but we don't necessarily restrict this to be it, and just PropertySupplier (interface) is fine and more flexible?

3efe319
I agree with you, and changed the argument type of supplier from StaticPropertySupplier to PropertySupplier.

lazmond3 · 2021-12-10T02:36:42Z

Difference from the last time

c17b3c7 Changed the return type of CentralDogmaPropertySupplier#defaultProperties() from JsonNode to List<Property<?>>.
df825c9 Added tests for CentralDogmaPropertySupplier#register().

kawamuray

Thanks for your PR! Left some comments.

kawamuray · 2021-12-10T05:15:54Z

centraldogma/src/main/java/com/linecorp/decaton/centraldogma/CentralDogmaPropertySupplier.java

@@ -145,19 +148,43 @@ public static CentralDogmaPropertySupplier register(CentralDogma centralDogma, S
        return new CentralDogmaPropertySupplier(centralDogma, project, repository, filename);
    }

+    public static CentralDogmaPropertySupplier register(CentralDogma centralDogma, String project,


As its a public interface, please give it a javadoc like another overload of register.

I added a javadoc for the new added register method: e8c311b

kawamuray · 2021-12-10T06:12:43Z

centraldogma/src/main/java/com/linecorp/decaton/centraldogma/CentralDogmaPropertySupplier.java

+                                                        PropertySupplier supplier) {
+        List<Property<?>> properties = defaultProperties().stream().map(defaultProperty -> {
+            if (supplier.getProperty(defaultProperty.definition()).isPresent()) {
+                return supplier.getProperty(defaultProperty.definition()).get();


As a call of getProperty may involves I/O with external systems (like centraldogma), it is better to keep count of calling it as minimum as possible.
so let's do it like:

prop = supplier.getProperty(...); if (prop.ifPresent()) { return prop.get(); } ...

I changed the code to use a temporary variable for keeping the value in external systems. 8d38efe

kawamuray · 2021-12-10T06:13:31Z

centraldogma/src/main/java/com/linecorp/decaton/centraldogma/CentralDogmaPropertySupplier.java

+        createPropertyFile(centralDogma, project, repository, filename, properties);
+        return new CentralDogmaPropertySupplier(centralDogma, project, repository, filename);
+    }
+
    private static void createPropertyFile(CentralDogma centralDogma, String project,


I think this overload no longer meaningful. Let's just use the below signature and pass default properties directly.

I deleted this original method createPropertyFile (no longer used), since a new method which has more arguments was added. 6b49b94

kawamuray · 2021-12-10T06:20:58Z

centraldogma/src/main/java/com/linecorp/decaton/centraldogma/CentralDogmaPropertySupplier.java

+    }
+
+    // visible for testing
+    static List<Property<?>> defaultProperties() {


I think this functionality can be required by various supplier implementations? Maybe it's worth putting this method in ProcessorProperties rather than here privately.

I moved this method defaultProperties, which returns List<Property<?>>, to ProcessorProperties. 03ef312

kawamuray · 2021-12-10T06:22:00Z

centraldogma/src/main/java/com/linecorp/decaton/centraldogma/CentralDogmaPropertySupplier.java

-                .forEach(definition -> properties.set(definition.name(),
-                                                      objectMapper.valueToTree(definition.defaultValue()))
-                );
+                .forEach(definition -> properties.add(new DynamicProperty(definition)));


There's no point to use DynamicProperty here. It's for modifiable property instance, so let's use Property.ofStatic() instead.

kawamuray · 2021-12-10T06:24:51Z

...ldogma/src/test/java/com/linecorp/decaton/centraldogma/CentralDogmaPropertySupplierTest.java

+        );
+        when(centralDogma.push(
+                PROJECT_NAME, REPOSITORY_NAME, Revision.HEAD,
+                String.format("[CentralDogmaPropertySupplier] Property file created: %s",


I don't think we need to match this argument and it will just make shit test fragile for tiny change on commit message which is mostly pointless. So let's use type matcher here, like any(String.class)

I changed the code to use mockito any(String.class), since a central dogma's commit message is not target of test for this PR's functionality.
f2ee11d

Hmm, looks like this part and one in below hasn't changed yet?

I'm sorry, I failed to update the code exhaustively.
I changed the when parts to use any: 0f2db81 , e852943

kawamuray · 2021-12-10T06:26:32Z

...ldogma/src/test/java/com/linecorp/decaton/centraldogma/CentralDogmaPropertySupplierTest.java

+
+        CentralDogmaPropertySupplier.register(centralDogma, PROJECT_NAME, REPOSITORY_NAME, FILENAME);
+
+        verify(centralDogma, times(1)).normalizeRevision(PROJECT_NAME, REPOSITORY_NAME, Revision.HEAD);


I think what we wanna test here is, if calling register() actually tries to create an expected file with expected content, so the only check against push() is meaningful to be testing subject?

I deleted the verifying lines: 61cea29

kawamuray · 2021-12-10T06:28:18Z

...ldogma/src/test/java/com/linecorp/decaton/centraldogma/CentralDogmaPropertySupplierTest.java

+
+        when(centralDogma.push(
+                PROJECT_NAME, REPOSITORY_NAME, Revision.HEAD,
+                String.format("[CentralDogmaPropertySupplier] Property file created: %s", FILENAME),


kawamuray · 2021-12-10T06:28:27Z

...ldogma/src/test/java/com/linecorp/decaton/centraldogma/CentralDogmaPropertySupplierTest.java

+        verify(centralDogma, times(1)).listFiles(PROJECT_NAME, REPOSITORY_NAME, Revision.HEAD, FILENAME);
+        verify(centralDogma, times(1))
+                .push(PROJECT_NAME, REPOSITORY_NAME, Revision.HEAD,
+                      String.format("[CentralDogmaPropertySupplier] Property file created: %s", FILENAME),


kawamuray · 2021-12-10T06:29:17Z

...ldogma/src/test/java/com/linecorp/decaton/centraldogma/CentralDogmaPropertySupplierTest.java

+
+    @Test
+    public void testRegisterWithCustomizedSettings() {
+        final Integer settingForPartitionConcurrency = 188;


Let's stick with primitive types when there's no point to use a boxed type. int

I changed the variable type to int from Integer. 382f706

…ister

…Properties

kawamuray

Thanks for follow-up. Just few more things.

kawamuray · 2021-12-13T06:17:20Z

centraldogma/src/main/java/com/linecorp/decaton/centraldogma/CentralDogmaPropertySupplier.java

+     * @param project the project name where the properties are placed.
+     * @param repository the repository name where the properties are placed.
+     * @param filename the name of the file containing properties as top-level fields.
+     * @param supplier user-customized settings for kafka-consumer.


These properties are of decaton, not kafka-consumer. So let's reword it like: a {@link PropertySupplier} which provides set of properties that wants to have a custom initial value

I reworded the javadoc for supplier:
a {@link PropertySupplier} which provides a set of properties with customized initial values
2e70198

kawamuray · 2021-12-13T06:19:08Z

processor/src/main/java/com/linecorp/decaton/processor/runtime/ProcessorProperties.java

@@ -218,4 +219,10 @@
    public ProcessorProperties(Map<PropertyDefinition<?>, Property<?>> properties) {
        super(properties);
    }
+
+    public static List<Property<?>> defaultProperties() {


Please give it even a short javadoc since its also a public interface.

I added a short description for this method: f82bae3

kawamuray · 2021-12-13T06:20:10Z

...ldogma/src/test/java/com/linecorp/decaton/centraldogma/CentralDogmaPropertySupplierTest.java

+        );
+        when(centralDogma.push(
+                PROJECT_NAME, REPOSITORY_NAME, Revision.HEAD,
+                String.format("[CentralDogmaPropertySupplier] Property file created: %s",


Hmm, looks like this part and one in below hasn't changed yet?

kawamuray

LGTM, thanks!

kawamuray · 2021-12-13T11:36:11Z

test is failing.

…mizedSettings in CentralDogmaPropertySupplierTest

…-value-arg-for-register

kawamuray · 2021-12-14T03:57:38Z

...ldogma/src/test/java/com/linecorp/decaton/centraldogma/CentralDogmaPropertySupplierTest.java

+        final int settingForMaxPendingRecords = 121212;
+        final int whenCentralDogmaPushed = 111111;
+
+        List<Property<?>> properties = ProcessorProperties


I don't see why we need full list of proeprties here, not only custom ones but also these with defaults?
Since the spec of newly added register API is such that "Creates a node with default properties, but you can specify some prop's initial value optionally", the realistic usage will look like:

CentralDogmaPropertySupplier.register(..., StaticPropertySupplier.of( Property.ofStatic(...) ));

so what we should test here is passing just partial list of properties that we want to customize? such as:

@Test public void testRegisterWithCustomizedSettings() { final int settingForPartitionConcurrency = 188; final int settingForMaxPendingRecords = 121212; final int whenCentralDogmaPushed = 111111; final PropertySupplier supplier = StaticPropertySupplier.of( Property.ofStatic(ProcessorProperties.CONFIG_PARTITION_CONCURRENCY, settingForPartitionConcurrency), Property.ofStatic(ProcessorProperties.CONFIG_MAX_PENDING_RECORDS, settingForMaxPendingRecords) ); final List<Property<?>> listProperties = ProcessorProperties .defaultProperties() .stream() .map(defaultProperty -> { Optional<? extends Property<?>> prop = supplier.getProperty(defaultProperty.definition()); if (prop.isPresent()) { return prop.get(); } else { return defaultProperty; } }).collect(Collectors.toList()); ...

I agree with you. I don't need to provide all of the properties if I don't change the value of it.

Changed: providing only customized properties
e6ae753 , bea0861

…n public class CentralDogmaPropertySupplierTest

kawamuray

LGTM, once again :)

Add new method overload for createPropertyFile, register

320cd69

kawamuray suggested changes Dec 3, 2021

View reviewed changes

lazmond3 added 3 commits December 9, 2021 17:35

Change argument type StaticPropertySupplier to PropertySupplier

3efe319

Change return type of method: defaultProperties: List

c17b3c7

Add tests for CentralDogmaPropertySupplier#register

df825c9

lazmond3 requested a review from kawamuray December 10, 2021 02:36

kawamuray suggested changes Dec 10, 2021

View reviewed changes

lazmond3 added 8 commits December 10, 2021 18:53

Add Javadoc for CentralDogmaPropertySupplier#register

e8c311b

Delete Unused CentralDogmaPropertySupplier#createPropertyFile

6b49b94

Reduce access to external systems in CentralDogmaPropertySupplier#reg…

8d38efe

…ister

Move defaultProperties From CentralDogmaPropertySupplier To Processor…

03ef312

…Properties

Use Property.ofStatic() instead of DynamicProperty

6d9e69e

Delete not meaningful verify method

61cea29

Use primitive types int: CentralDogmaPropertySupplierTest

382f706

Use any(String.class) for commit message of centraldogma

f2ee11d

lazmond3 marked this pull request as ready for review December 13, 2021 05:46

lazmond3 requested a review from kawamuray December 13, 2021 05:46

kawamuray suggested changes Dec 13, 2021

View reviewed changes

lazmond3 added 3 commits December 13, 2021 15:25

Use any(String.class) for commit message of centraldogma 2

0f2db81

Reword javadoc for CentralDogmaPropertySupplier#register

2e70198

Added a short javadoc for ProcessorProperties#defaultProperties

f82bae3

kawamuray added the new feature Add a new feature label Dec 13, 2021

Fixed format in CentralDogmaPropertySupplierTest

e852943

lazmond3 requested a review from kawamuray December 13, 2021 06:43

kawamuray approved these changes Dec 13, 2021

View reviewed changes

lazmond3 added 2 commits December 14, 2021 10:15

Fix format in CentralDogmaPropertySupplierTest

ecf4284

Rename test private method: defaultPropertiesAsJsonNode

e1952c4

lazmond3 added 2 commits December 14, 2021 11:37

Fix test: default properties list: not fragile: testRegisterWithCusto…

ed02a7c

…mizedSettings in CentralDogmaPropertySupplierTest

Merge remote-tracking branch 'origin/master' into feature/add-initial…

85f64d7

…-value-arg-for-register

kawamuray suggested changes Dec 14, 2021

View reviewed changes

lazmond3 added 2 commits December 14, 2021 13:31

Provide only changed properties: testRegisterWithCustomizedSettings i…

e6ae753

…n public class CentralDogmaPropertySupplierTest

make the code short listPropertiesForVerifyingMock

bea0861

lazmond3 requested a review from kawamuray December 14, 2021 04:34

kawamuray approved these changes Dec 14, 2021

View reviewed changes

kawamuray merged commit 5b49d0f into line:master Dec 14, 2021

kawamuray mentioned this pull request Dec 14, 2021

Add a feature to supply initial value for self CD registration #22

Closed


		CentralDogmaPropertySupplier.register(centralDogma, PROJECT_NAME, REPOSITORY_NAME, FILENAME);

		verify(centralDogma, times(1)).normalizeRevision(PROJECT_NAME, REPOSITORY_NAME, Revision.HEAD);

Add new method(overload) for CentralDogmaPropertySupplier#createPropertyFile/register #134

Add new method(overload) for CentralDogmaPropertySupplier#createPropertyFile/register #134

Conversation

lazmond3 commented Dec 2, 2021 • edited Loading

Relevant Issue

Motivation

Summary of Changes

Usage Example

CLAassistant commented Dec 2, 2021 • edited Loading

kawamuray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazmond3 commented Dec 10, 2021

Difference from the last time

kawamuray left a 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

lazmond3 Dec 13, 2021 • 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

lazmond3 Dec 13, 2021 • 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

kawamuray Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kawamuray left a 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

lazmond3 Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kawamuray left a comment

Choose a reason for hiding this comment

kawamuray commented Dec 13, 2021

Choose a reason for hiding this comment

lazmond3 Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

kawamuray left a comment

Choose a reason for hiding this comment

lazmond3 commented Dec 2, 2021 •

edited

Loading

CLAassistant commented Dec 2, 2021 •

edited

Loading

lazmond3 Dec 13, 2021 •

edited

Loading

lazmond3 Dec 13, 2021 •

edited

Loading

kawamuray Dec 10, 2021 •

edited

Loading

lazmond3 Dec 13, 2021 •

edited

Loading

lazmond3 Dec 14, 2021 •

edited

Loading