Provide the fluent API for CentralDogma client #651

minwoox · 2021-12-02T09:22:08Z

Motivation:
Currently, CentralDogma client has a bunch of methods that take a bunch of parameters.
It makes it harder to add another optional parameter to the existing methods because we need to add another pair of existing methods. For example:

// Existing methods:
void watch(String a);
void watch(String a, String b);
void watch(String a, String b, String c);

// If we want to add an optional long value then we need another three methods:
void watch(String a, long d);
void watch(String a, String b, long d);
void watch(String a, String b, String c, long d);

Modifications:

Add PathPattern interface to pass as the parameter instead of String.

Add CentralDogma.forRepo(...) method that returns CentralDogmaRepository.

Users can use the instance to send a request:

centralDogma.forRepo("foo", "bar").file("/foo.json").get();
centralDogma.forRepo("foo", "bar").watch("/foo.json").start();
centralDogma.forRepo("foo", "bar").watcher(PathPattern.all()).start();

Result:

You can now send a request to the Central Dogma server via fluent APIs.
(Deprecated) Various methods in CentralDogma client are deprecated. Use fluent APIs.

codecov · 2021-12-02T09:39:27Z

Codecov Report

Merging #651 (c6d5393) into master (e46e977) will increase coverage by 0.21%.
The diff coverage is 80.21%.

@@             Coverage Diff              @@
##             master     #651      +/-   ##
============================================
+ Coverage     69.87%   70.09%   +0.21%     
- Complexity     3298     3399     +101     
============================================
  Files           333      349      +16     
  Lines         13136    13464     +328     
  Branches       1425     1448      +23     
============================================
+ Hits           9179     9437     +258     
- Misses         3081     3151      +70     
  Partials        876      876

Impacted Files	Coverage Δ
...ogma/client/armeria/CentralDogmaEndpointGroup.java	`60.71% <0.00%> (-4.68%)`	⬇️
.../java/com/linecorp/centraldogma/client/Latest.java	`76.47% <ø> (+11.76%)`	⬆️
...m/linecorp/centraldogma/client/WatchConstants.java	`100.00% <ø> (ø)`
...com/linecorp/centraldogma/client/CentralDogma.java	`3.03% <11.11%> (-56.23%)`	⬇️
...corp/centraldogma/client/AbstractCentralDogma.java	`25.00% <27.27%> (-45.97%)`	⬇️
...m/linecorp/centraldogma/client/WatcherRequest.java	`64.91% <64.91%> (ø)`
...com/linecorp/centraldogma/client/FilesRequest.java	`66.66% <66.66%> (ø)`
...com/linecorp/centraldogma/client/WatchOptions.java	`69.23% <69.23%> (ø)`
.../linecorp/centraldogma/client/AbstractWatcher.java	`79.47% <72.05%> (ø)`
...ldogma/client/updater/CentralDogmaBeanFactory.java	`59.70% <72.72%> (+0.61%)`	⬆️
... and 36 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 e46e977...c6d5393. Read the comment docs.

minwoox · 2021-12-03T03:48:29Z

This is ready for review. 😉

ikhoon

Looks fluent!

client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogmaRequestPreparation.java

client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogmaFilesRequest.java

client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogmaCommitRequest.java

client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogmaRequestPreparation.java

### Motivation - line#532 - To-Do of line#610 ### Modifications - Put value of `notify-entry-not-found` on `Prefer` request header to get error that entries are not found on watching file/repository - Propagate error that entries are not found into `Watcher#initialValueFuture` ### Result - You can get error on watching file/repository if the entry doesn't exist.

Motivation: I found out while I implementing line#651. The child watcher created via `Watcher#newChild(mapper)` executes the specified mapper every time when a listener is notified. If there are 1000 listeners, mapper is called 1000 times. This could be a problem if the mapper has a haevy logic. Modifications: - Execute mapper onle once when the entry is chaned. - Add `Watcher.newWatcher(Function, Executor)` that executes the mapper using the executor. Result: - The mapper `Function` of a Child watcher is executed only once when the child watcher get notified.

Motivation: I found out while I implementing line#651. The child watcher created via `Watcher#newChild(mapper)` executes the specified mapper every time when a listener is notified. If there are 1000 listeners, mapper is called 1000 times. This could be a problem if the mapper has heavy logic. Modifications: - Execute mapper only once when the entry is changed. - Add `Watcher.newWatcher(Function, Executor)` that executes the mapper using the executor. Result: - The mapper `Function` of a Child watcher is executed only once when the child watcher gets notified.

@ikhoon

…tern & address comments by @ikhoon

minwoox · 2021-12-23T03:51:28Z

@di-seo Please, let me know if there's anything we can improve. 😄

jrhee17

The overall API looks really nice! 🙇 👍 Left some minor questions on the Watcher implementation 🙏

client/java/src/main/java/com/linecorp/centraldogma/client/MappingWatcher.java

common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java

client/java/src/main/java/com/linecorp/centraldogma/client/MappingWatcher.java

di-seo

I left some comments.
I look forward to this changes. :)

client/java/src/main/java/com/linecorp/centraldogma/client/WatchFilesRequest.java

client/java/src/main/java/com/linecorp/centraldogma/client/DefaultWatcher.java

client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogmaRepository.java

client/java/src/main/java/com/linecorp/centraldogma/client/WatcherOptionsBuilder.java

client/java/src/main/java/com/linecorp/centraldogma/client/Watcher.java

common/src/main/java/com/linecorp/centraldogma/common/PathPattern.java

ikhoon

Mostly looks good.

client/java/src/main/java/com/linecorp/centraldogma/client/AbstractWatcher.java

client/java/src/main/java/com/linecorp/centraldogma/client/CommitRequest.java

client/java/src/main/java/com/linecorp/centraldogma/client/Watcher.java

client/java/src/main/java/com/linecorp/centraldogma/client/WatchRequest.java

ikhoon · 2021-12-30T08:29:12Z

...meria/src/test/java/com/linecorp/centraldogma/client/armeria/CentralDogmaRepositoryTest.java

+        @Override
+        protected void scaffold(CentralDogma client) {
+            client.createProject("foo").join();
+            client.createRepository("foo", "bar").join();


Now, it might be natual that client.createRepository("foo", "bar") returns a CompletableFuture<CentralDogmaRepository>, although it is a breaking change. 😅

Let me do that in the following PR. 😅

client/java/src/main/java/com/linecorp/centraldogma/client/FileRequest.java

jrhee17

Overall looks good! Left some minor comments 🙏

common/src/main/java/com/linecorp/centraldogma/common/DefaultPathPattern.java

client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogma.java

client/java/src/main/java/com/linecorp/centraldogma/client/WatchRequest.java

minwoox · 2021-12-31T05:26:38Z

@ikhoon and @jrhee17 All addressed, PTAL. 🙇‍♂️

jrhee17

Left a minor question, but overall looks good to me! Thank you @minwoox 🙇 👍 🙇

client/java/src/main/java/com/linecorp/centraldogma/client/MappingWatcher.java

client/java/src/main/java/com/linecorp/centraldogma/client/WatcherRequest.java

ikhoon

Good shot!

client/java/src/main/java/com/linecorp/centraldogma/client/Watcher.java

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

minwoox · 2022-01-04T15:00:09Z

Thanks, reviewers for the valuable comments. 🙇‍♂️

Motivation: We introduced `CentralDogmaRepository` class in line#651 and it would be nice if we return it from `centralDogma.createRepository(...)` so users can chain the next call. ```java dogma.createRepository("foo", "bar) .join() .commit(...) .push(); ``` Modifications: - Change to return `CentralDogmaRepository` from `centralDogma.createRepository(...)` and `centralDogma.unremoveRepository(...). - Fix flaky test in `WatchTest`.` Result: - (Breaking) `createRepository` and `unremoveRepository` from `CentralDogma` now returns `CentralDogmaRepository`.

…662) Motivation: We introduced `CentralDogmaRepository` class in #651 and it would be nice if we return it from `centralDogma.createRepository(...)` so users can chain the next call. ```java dogma.createRepository("foo", "bar) .join() .commit(...) .push(); ``` Modifications: - Change to return `CentralDogmaRepository` from `centralDogma.createRepository(...)` and `centralDogma.unremoveRepository(...). - Fix flaky test in `WatchTest`. Result: - (Breaking) `createRepository` and `unremoveRepository` from `CentralDogma` now returns `CentralDogmaRepository`.

minwoox requested review from ikhoon, jrhee17 and trustin as code owners December 2, 2021 09:22

minwoox mentioned this pull request Dec 2, 2021

Throw error when entries are not found on watching if want #650

Closed

(WIP) Provide the fluent API for CentralDogma client

74309d3

minwoox force-pushed the fluentApi_client branch from a7f0c4e to 74309d3 Compare December 3, 2021 03:22

minwoox added the new feature label Dec 3, 2021

minwoox added this to the 0.53.0 milestone Dec 3, 2021

minwoox changed the title ~~(WIP) Provide the fluent API for CentralDogma client~~ Provide the fluent API for CentralDogma client Dec 3, 2021

ikhoon modified the milestones: 0.53.0, 0.54.0 Dec 3, 2021

di-seo mentioned this pull request Dec 3, 2021

Throw error that entries are not found on watching if want #653

Merged

ikhoon reviewed Dec 3, 2021

View reviewed changes

di-seo and others added 2 commits December 10, 2021 00:08

Merge branch 'revampClient' into fluentApi_client

ff5f196

minwoox mentioned this pull request Dec 20, 2021

Execute mapper only once when the child watcher gets notified #657

Merged

minwoox added 6 commits December 22, 2021 16:09

WIP

2b51adb

Merge branch 'revampClient' into fluentApi_client & introduce PathPat…

8f5f724

…tern & address comments by @ikhoon

Merge branch 'master' into fluentApi_client

6c5ff5b

Fix style

c8b89df

Fix style

7b4e04d

minwoox force-pushed the fluentApi_client branch from e5fb6f1 to 7b4e04d Compare December 23, 2021 03:46

jrhee17 reviewed Dec 23, 2021

View reviewed changes

di-seo reviewed Dec 23, 2021

View reviewed changes

client/java/src/main/java/com/linecorp/centraldogma/client/WatchFilesRequest.java Outdated Show resolved Hide resolved

client/java/src/main/java/com/linecorp/centraldogma/client/DefaultWatcher.java Outdated Show resolved Hide resolved

ikhoon reviewed Dec 23, 2021

View reviewed changes

ikhoon mentioned this pull request Dec 24, 2021

Add useful factory methods to PathPattern #659

Open

Address comments and set a mapper to WatcherRequest

cde89ee

ikhoon reviewed Dec 30, 2021

View reviewed changes

common/src/main/java/com/linecorp/centraldogma/common/PathPattern.java Outdated Show resolved Hide resolved

ikhoon reviewed Dec 30, 2021

View reviewed changes

jrhee17 reviewed Dec 30, 2021

View reviewed changes

Address comments by @ikhoon and @jrhee17

09a9cc3

minwoox mentioned this pull request Dec 31, 2021

Implement watch_file and watch_repository line/centraldogma-python#24

Merged

jrhee17 approved these changes Dec 31, 2021

View reviewed changes

Catch the exception in mapper

fa6fbeb

ikhoon approved these changes Jan 4, 2022

View reviewed changes

Address comments by @ikhoon

c6d5393

minwoox merged commit e93bc5a into line:master Jan 4, 2022

minwoox deleted the fluentApi_client branch January 4, 2022 14:59

minwoox mentioned this pull request Jan 5, 2022

Change to return CentralDogmaRepository from client.createRepository #662

Merged

ikhoon modified the milestones: 0.54.0, 0.55.0 Jan 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide the fluent API for CentralDogma client #651

Provide the fluent API for CentralDogma client #651

minwoox commented Dec 2, 2021 •

edited

Loading

codecov bot commented Dec 2, 2021 •

edited

Loading

minwoox commented Dec 3, 2021

ikhoon left a comment

minwoox commented Dec 23, 2021

jrhee17 left a comment

di-seo left a comment

ikhoon left a comment

ikhoon Dec 30, 2021

minwoox Dec 30, 2021

jrhee17 left a comment •

edited

Loading

minwoox commented Dec 31, 2021

jrhee17 left a comment

ikhoon left a comment

minwoox commented Jan 4, 2022

Provide the fluent API for CentralDogma client #651

Provide the fluent API for CentralDogma client #651

Conversation

minwoox commented Dec 2, 2021 • edited Loading

codecov bot commented Dec 2, 2021 • edited Loading

Codecov Report

minwoox commented Dec 3, 2021

ikhoon left a comment

Choose a reason for hiding this comment

minwoox commented Dec 23, 2021

jrhee17 left a comment

Choose a reason for hiding this comment

di-seo left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon Dec 30, 2021

Choose a reason for hiding this comment

minwoox Dec 30, 2021

Choose a reason for hiding this comment

jrhee17 left a comment • edited Loading

Choose a reason for hiding this comment

minwoox commented Dec 31, 2021

jrhee17 left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

minwoox commented Jan 4, 2022

minwoox commented Dec 2, 2021 •

edited

Loading

codecov bot commented Dec 2, 2021 •

edited

Loading

jrhee17 left a comment •

edited

Loading