Add CentralDogma backed EndpointGroup #105

imasahiro · 2017-12-13T10:00:14Z

Note: Special thanks to @huydx who is the initial author of
CentralDogmaEndpointGroup(line/armeria#828)

huydx · 2017-12-13T10:08:01Z

there's no unit test yet, or you will add it later?

trustin · 2017-12-14T01:54:25Z

...t/java/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java

+/**
+ * A CentralDogma based {@link EndpointGroup} implementation. This {@link EndpointGroup} retrieves the list of
+ * {@link Endpoint}s from a route file served by CentralDogma, and update the list when upstream data changes.
+ * Route file could be json file or normal text file.


s/json/JSON/g

trustin · 2017-12-14T01:55:27Z

...t/java/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java

+ * {@link Endpoint}s from a route file served by CentralDogma, and update the list when upstream data changes.
+ * Route file could be json file or normal text file.
+ * <p>
+ * <p>In below example, json file with below content will be served as route file:


For example, the following JSON file will be served as a route file:

trustin · 2017-12-14T01:55:54Z

...t/java/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java

+ *      "host3:port3"
+ *  ]
+ * }
+ * </pre>


}</pre> (no newline between } and </pre>)

trustin · 2017-12-14T01:56:23Z

...t/java/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java

+ * A CentralDogma based {@link EndpointGroup} implementation. This {@link EndpointGroup} retrieves the list of
+ * {@link Endpoint}s from a route file served by CentralDogma, and update the list when upstream data changes.
+ * Route file could be json file or normal text file.
+ * <p>


An empty <p> is redundant. Please just use an empty line.

trustin · 2017-12-14T01:56:50Z

...t/java/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java

+ * }
+ * </pre>
+ * <p>
+ * <p>The route file could be retrieve as {@link EndpointGroup} using below code:


retrieve -> retrieved

as -> as an

below code -> the following code

trustin · 2017-12-14T02:13:00Z

...ava/src/main/java/com/linecorp/centraldogma/client/armeria/DefaultCentralDogmaJsonCodec.java

+
+import com.linecorp.armeria.client.Endpoint;
+
+public final class DefaultCentralDogmaJsonCodec implements EndpointListCodec<JsonNode> {


How about making it package-local?

JsonEndpointListDecoder?

trustin · 2017-12-14T02:13:05Z

...ava/src/main/java/com/linecorp/centraldogma/client/armeria/DefaultCentralDogmaTextCodec.java

+
+import com.linecorp.armeria.client.Endpoint;
+
+public final class DefaultCentralDogmaTextCodec implements EndpointListCodec<String> {


How about making it package-local?

TextEndpointListDecoder?

trustin · 2017-12-14T02:14:18Z

client/java/src/main/java/com/linecorp/centraldogma/client/armeria/EndpointListCodec.java

+ *
+ * @param <T> Type of CentralDomgma file (could be JsonNode or String)
+ */
+public interface EndpointListCodec<T> {


EndpointListDecoder?

trustin · 2017-12-14T02:14:27Z

client/java/src/main/java/com/linecorp/centraldogma/client/armeria/EndpointListCodec.java

+public interface EndpointListCodec<T> {
+
+    /**
+     * Default {@link EndpointListCodec} implementation for JsonNode object.


{@link JsonNode}

trustin · 2017-12-14T02:21:45Z

...ava/src/main/java/com/linecorp/centraldogma/client/armeria/DefaultCentralDogmaTextCodec.java

+    public List<Endpoint> decode(String object) {
+        final List<String> endpoints;
+        try {
+            endpoints = OBJECT_MAPPER.readValue(object, new TypeReference<List<String>>() {});


If an entry being watched is a JSON, decoding would be done by DefaultCentralDogmaJsonCodec. How about making this class decode a text file? e.g. a text file each line contains an endpoint:

foo.com:8080 bar.com:8443 ...

trustin

Thanks!

trustin · 2017-12-14T02:24:09Z

client/java/src/main/java/com/linecorp/centraldogma/client/armeria/EndpointListCodec.java

+     * </ul>
+     * Note that the port number must be specified when you want to specify the weight.
+     */
+    EndpointListCodec<JsonNode> DEFAULT_JSON_CODEC = new DefaultCentralDogmaJsonCodec();


trustin · 2017-12-14T02:24:18Z

client/java/src/main/java/com/linecorp/centraldogma/client/armeria/EndpointListCodec.java

+     * </ul>
+     * Note that the port number must be specified when you want to specify the weight.
+     */
+    EndpointListCodec<String> DEFAULT_STRING_CODEC = new DefaultCentralDogmaTextCodec();


trustin · 2017-12-14T02:24:53Z

client/java/src/main/java/com/linecorp/centraldogma/client/armeria/EndpointListCodecUtils.java

+import com.linecorp.armeria.client.Endpoint;
+
+final class EndpointListCodecUtils {
+    static final ObjectMapper OBJECT_MAPPER = new ObjectMapper().disable(FAIL_ON_UNKNOWN_PROPERTIES);


objectMapper, because it's not really a constant?

trustin

I see a checkstyle failure:

[ERROR] C:\projects\centraldogma\client\java\src\main\java\com\linecorp\centraldogma\client\armeria\JsonEndpointListDecoder.java:19: Wrong order for 'com.linecorp.centraldogma.client.armeria.EndpointListCodecUtils.convertToEndpointList' import. [ImportOrder]

trustin · 2018-03-08T01:42:29Z

client/java/src/main/java/com/linecorp/centraldogma/client/armeria/TextEndpointListDecoder.java

+import com.linecorp.armeria.client.Endpoint;
+
+final class TextEndpointListDecoder implements EndpointListDecoder<String> {
+    private static final Splitter NEWLINE_SPLITTER = Splitter.on(System.getProperty("line.separator", "\n"))


Could we make the splitter split on both \r\n and \n rather than only on one?

trustin · 2018-03-08T01:42:55Z

...t/java/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java

+ * <pre>{@code
+ *  CentralDogmaEndpointGroup<JsonNode> endpointGroup = CentralDogmaEndpointGroup.ofJsonFile(
+ *      centralDogma, "myProject", "myRepo",
+ *      CentralDogmaCodec.DEFAULT_JSON_CODEC,


Example needs update.

trustin · 2018-04-16T10:02:30Z

@imasahiro Thanks for an update! Shall I review now or wait for another update?

imasahiro · 2018-04-23T06:15:03Z

Maybe not ready yet. Let me update my internal usage to confirm the changes at first.

@huydx

Note: Special thanks to @huydx who is the initial author of CentralDogmaEndpointGroup(line/armeria#828)

codecov-io · 2018-05-01T16:44:35Z

Codecov Report

Merging #105 into master will increase coverage by 0.1%.
The diff coverage is 71.73%.

@@            Coverage Diff            @@
##           master     #105     +/-   ##
=========================================
+ Coverage   64.47%   64.58%   +0.1%     
=========================================
  Files         271      276      +5     
  Lines        9695     9741     +46     
  Branches     1098     1099      +1     
=========================================
+ Hits         6251     6291     +40     
- Misses       2812     2818      +6     
  Partials      632      632

Impacted Files	Coverage Δ
...ldogma/client/armeria/TextEndpointListDecoder.java	`100% <100%> (ø)`
...ntraldogma/client/armeria/EndpointListDecoder.java	`100% <100%> (ø)`
...ogma/client/armeria/CentralDogmaEndpointGroup.java	`61.53% <61.53%> (ø)`
...ldogma/client/armeria/JsonEndpointListDecoder.java	`71.42% <71.42%> (ø)`
...aldogma/client/armeria/EndpointListCodecUtils.java	`83.33% <83.33%> (ø)`
...server/internal/thrift/CentralDogmaExceptions.java	`47.82% <0%> (-13.05%)`	⬇️
...internal/replication/ZooKeeperCommandExecutor.java	`71.94% <0%> (ø)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18a74e1...91a6fd6. Read the comment docs.

imasahiro · 2018-05-02T03:05:03Z

@trustin Now ready for review.

trustin

Mostly nits. Great job!

trustin · 2018-05-02T06:45:35Z

...java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/EndpointListDecoder.java

+import com.linecorp.armeria.client.Endpoint;
+
+/**
+ * Decode and encode between CentralDogma upstream file content and list of {@link Endpoint}s.


Decodes the content of a file in Central Dogma into a list of {@link Endpoint}s.?

trustin · 2018-05-02T06:45:51Z

...java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/EndpointListDecoder.java

+/**
+ * Decode and encode between CentralDogma upstream file content and list of {@link Endpoint}s.
+ *
+ * @param <T> Type of CentralDogma file (could be {@link JsonNode} or {@link String})


the type of the file in Central Dogma?

trustin · 2018-05-02T06:46:17Z

...java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/EndpointListDecoder.java

+public interface EndpointListDecoder<T> {
+
+    /**
+     * Default {@link EndpointListDecoder} implementation for JsonNode object.


{@link JsonNode}

OK to remove 'object'?

trustin · 2018-05-02T06:47:50Z

...java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/EndpointListDecoder.java

+
+    /**
+     * Default {@link EndpointListDecoder} implementation for JsonNode object.
+     * Retrive object must be a json array (which has format as "[ \"segment1\", \"segment2\" ]"


Retrieve -> Retrieved

json -> JSON

( has no matching ).

trustin · 2018-05-02T06:48:09Z

...java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/EndpointListDecoder.java

+    EndpointListDecoder<JsonNode> JSON = new JsonEndpointListDecoder();
+
+    /**
+     * Default {@link EndpointListDecoder} implementation for String object.


String object -> {@link String}?

trustin · 2018-05-02T06:56:40Z

...rmeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java

+ *  ]
+ * }</pre>
+ *
+ * <p>The route file could be retrieved as an {@link EndpointGroup} using the following code:


The JSON array file could be..

trustin · 2018-05-02T06:57:28Z

...rmeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java

+ *
+ * <p>The route file could be retrieved as an {@link EndpointGroup} using the following code:
+ * <pre>{@code
+ *  CentralDogmaEndpointGroup<JsonNode> endpointGroup = CentralDogmaEndpointGroup.of(


Could be indented one less column?

trustin · 2018-05-02T06:58:32Z

...rmeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java

+ *  endpointGroup.endpoints();
+ * }</pre>
+ *
+ * @param <T> Type of CentralDomgma file (could be {@link JsonNode} or {@link String})


the type of the file in Central Dogma?

trustin · 2018-05-02T06:59:21Z

it/src/test/java/com/linecorp/centraldogma/it/CentralDogmaEndpointGroupTest.java

+    @Test(timeout = 10000)
+    public void text() throws Exception {
+        Watcher<String> watcher = dogma.client().fileWatcher("directory", "my-service",
+                                                             Query.ofText("/endpoint.txt"));


endpoints.txt?

trustin · 2018-05-02T06:59:36Z

it/src/test/java/com/linecorp/centraldogma/it/CentralDogmaEndpointGroupTest.java

+    @Test
+    public void json() {
+        Watcher<JsonNode> watcher = dogma.client().fileWatcher("directory", "my-service",
+                                                               Query.ofJson("/endpoint.json"));


/endpoints.json?

minwoox · 2018-05-08T03:49:22Z

...java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/EndpointListDecoder.java

@@ -0,0 +1,69 @@
+/*
+ * Copyright 2017 LINE Corporation


minwoox · 2018-05-08T03:52:38Z

...java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/EndpointListDecoder.java

+
+    /**
+     * Default {@link EndpointListDecoder} implementation for {@link String}.
+     * Retrieved object must be a string which is a list of segments separated by by a newline character.


minwoox · 2018-05-08T03:53:47Z

...java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/EndpointListDecoder.java

+    /**
+     * Decodes an object into a set of {@link Endpoint}s.
+     *
+     * @param object an object retrieve from Central Dogma.


minwoox · 2018-05-08T03:57:11Z

...rmeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java

+ *      centralDogma, "myProject", "myRepo",
+ *      Query.ofJson("/endpoints.json"),
+ *      EndpointListDecoder.JSON
+ * )


) -> );
You can append it after EndpointListDecoder.JSON without the newline. :)

minwoox · 2018-05-08T04:00:53Z

...rmeria/src/main/java/com/linecorp/centraldogma/client/armeria/CentralDogmaEndpointGroup.java

+    private static final long WATCH_INITIALIZATION_TIMEOUT_MILLIS = TimeUnit.SECONDS.toMillis(10);
+
+    private final Watcher<T> instanceListWatcher;
+    private final EndpointListDecoder<T> endpointCodec;


I'm not sure if we need a different name for this. Codec means encoder and decoder, but this seems like only decoding now.
Is there any chance to use this as encoder later?

minwoox

Great job!

trustin

Just some nits. Great job!

trustin · 2018-05-09T05:49:32Z

it/src/test/java/com/linecorp/centraldogma/it/CentralDogmaEndpointGroupTest.java

+             .join();
+
+        await().atMost(10, TimeUnit.SECONDS).untilAsserted(() -> assertThat(latch.getCount()).isZero());
+        assertThat(endpointGroup.endpoints()).isEqualTo(ImmutableList.of(Endpoint.of("foo.bar", 1234)));


nit: Could use containsExactly()?

trustin · 2018-05-09T05:49:47Z

it/src/test/java/com/linecorp/centraldogma/it/CentralDogmaEndpointGroupTest.java

+        dogma.client().push("directory", "my-service",
+                            Revision.HEAD, "commit",
+                            Change.ofTextUpsert("/endpoints.txt",
+                                                "foo.bar:1234"))


nit: Could be merged into a single line

trustin · 2018-05-09T05:50:19Z

it/src/test/java/com/linecorp/centraldogma/it/CentralDogmaEndpointGroupTest.java

+
+    @Test(timeout = 10000)
+    public void text() throws Exception {
+        Watcher<String> watcher = dogma.client().fileWatcher("directory", "my-service",


Could use try-resources?

trustin · 2018-05-09T05:50:28Z

it/src/test/java/com/linecorp/centraldogma/it/CentralDogmaEndpointGroupTest.java

+
+    @Test
+    public void json() throws Exception {
+        Watcher<JsonNode> watcher = dogma.client().fileWatcher("directory", "my-service",


Could use try-resources?

trustin

Thanks, @imasahiro and @huydx!

trustin · 2018-05-10T08:55:41Z

@hyangtack Comments?

trustin · 2018-05-11T09:30:13Z

Thanks, @imasahiro, @huydx and reviewers :-)

imasahiro requested review from trustin, hyangtack and minwoox December 13, 2017 10:00

imasahiro added the in progress label Dec 13, 2017

trustin reviewed Dec 14, 2017

View reviewed changes

trustin requested changes Dec 14, 2017

View reviewed changes

trustin removed the in progress label Jan 8, 2018

imasahiro force-pushed the armeria branch from 40c8ebc to 983b437 Compare March 7, 2018 07:37

trustin requested changes Mar 8, 2018

View reviewed changes

trustin added the new feature label Mar 8, 2018

trustin modified the milestones: 0.22.0, 0.23.0 Mar 8, 2018

trustin removed this from the 0.23.0 milestone Mar 30, 2018

imasahiro force-pushed the armeria branch from 983b437 to 8f22d4e Compare April 12, 2018 10:47

imasahiro added 3 commits May 1, 2018 23:44

Add CentralDogma backed EndpointGroup

dc4157d

Note: Special thanks to @huydx who is the initial author of CentralDogmaEndpointGroup(line/armeria#828)

Reflect review

b063815

Add more tests

ec755f4

imasahiro force-pushed the armeria branch from 8f22d4e to ec755f4 Compare May 1, 2018 16:16

imasahiro added 2 commits May 2, 2018 01:24

Close watcher

5eff202

Fix copyright

3390f9c

imasahiro force-pushed the armeria branch from 3640208 to 3390f9c Compare May 1, 2018 16:44

trustin requested changes May 2, 2018

View reviewed changes

Update documents

4dc4e5d

imasahiro force-pushed the armeria branch from 8109887 to 4dc4e5d Compare May 7, 2018 05:47

Fix checkstyle error

abfb110

minwoox reviewed May 8, 2018

View reviewed changes

Fix documents

5fabf32

minwoox approved these changes May 8, 2018

View reviewed changes

trustin added this to the 0.25.0 milestone May 9, 2018

trustin requested changes May 9, 2018

View reviewed changes

Use try-with-resource

91a6fd6

trustin approved these changes May 10, 2018

View reviewed changes

hyangtack approved these changes May 11, 2018

View reviewed changes

trustin merged commit aed4dff into line:master May 11, 2018

imasahiro deleted the armeria branch May 11, 2018 10:34


		import com.linecorp.armeria.client.Endpoint;

		public final class DefaultCentralDogmaJsonCodec implements EndpointListCodec<JsonNode> {


		import com.linecorp.armeria.client.Endpoint;

		public final class DefaultCentralDogmaTextCodec implements EndpointListCodec<String> {

Add CentralDogma backed EndpointGroup #105

Add CentralDogma backed EndpointGroup #105

Conversation

imasahiro commented Dec 13, 2017

huydx commented Dec 13, 2017 • edited

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

trustin Dec 14, 2017 • edited

Choose a reason for hiding this comment

trustin Dec 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin Dec 14, 2017 • edited

Choose a reason for hiding this comment

trustin 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

trustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin commented Apr 16, 2018

imasahiro commented Apr 23, 2018

codecov-io commented May 1, 2018 • edited

Codecov Report

imasahiro commented May 2, 2018

trustin 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

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

minwoox left a comment

Choose a reason for hiding this comment

trustin 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

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

trustin commented May 10, 2018

trustin commented May 11, 2018

huydx commented Dec 13, 2017 •

edited

trustin Dec 14, 2017 •

edited

trustin Dec 14, 2017 •

edited

trustin Dec 14, 2017 •

edited

codecov-io commented May 1, 2018 •

edited