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.

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

Allow configuring the initialization timeout of CentralDogma client for Spring integration. #692

Merged
merged 7 commits into from
Jul 6, 2022

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented May 11, 2022

Motivation:

3.2 seconds is used for the default timeout if an endpoint is not
determined while executing a request.
While a server is starting up, a resolution of endpoints may take
longer than 3.2 seconds because the server could be busy initializing
the application context.
To solve the problem, CentralDogma.whenEndpointReady() was introduced
to wait for the initial endpoints before starting a request.

However, users found it easy to forget to call the API and use directly a
CentralDogma client without waiting for it. It is difficult to call
whenEndpointReady().get() because it is to block the current thread.
But a CentralDogma bean is a managed instance created in the main
thread when a Spring application context is initialized. So we can
safely call CentralDogma.whenEndpointReady().get() before returning a
client.

Modifications:

  • Add initializationTimeoutMillis to CentralDogmaSettings so as to
    to let users configure the timeout
    • If unspecified, 15 seconds is used by default.
    • 0 disables the waiting for initialization.

Result:

… for Spring integration.

Motivation:
3.2 seconds is used for the default timeout if a endpoint is not
determined while executing a request.
While a server is starting up, a resolution of endpoints may take
longer than 3.2 seconds because the server could be busy initializing
the application context.
To solve the problem, `CentralDogma.whenEndpointReady()` was introduced
to wait for the initial endpoints before starting a request.

However, users found it easy to forget to call the API and use directly a
`CentralDogma` client without waiting it. It is difficult to call
`whenEndpointReady().get()` because it is to block the current thread.
But a `CentralDogma` bean is a managed instance created in the main
thread when a Spring application context is initialized. So we can
safely call `CentralDogma.whenEndpointReady().get()` before returing a
client.

Modifications:

- Add `initializationTimeoutMillis` to `CentralDogmaSettings` so as to
  to let users configure the timeout
  - If unspecifed, 15 seconds is used by default.
  - 0 disables the waiting for initialization.

Result:

- You can not configure the initialization timeout for a `CentralDogma`
  client when using Spring integration module.
  ```yml
  centraldogma:
    hosts:
       - ...
    initialization-timeout-millis: 15000
  ```
- Fixes line#691
@ikhoon ikhoon added this to the 0.56.0 milestone May 11, 2022
@codecov
Copy link

codecov bot commented May 11, 2022

Codecov Report

Merging #692 (821eec8) into master (8a32763) will decrease coverage by 0.04%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##             master     #692      +/-   ##
============================================
- Coverage     70.19%   70.15%   -0.05%     
+ Complexity     3419     3416       -3     
============================================
  Files           349      349              
  Lines         13468    13482      +14     
  Branches       1454     1456       +2     
============================================
+ Hits           9454     9458       +4     
- Misses         3133     3139       +6     
- Partials        881      885       +4     
Impacted Files Coverage Δ
...nt/spring/CentralDogmaClientAutoConfiguration.java 86.27% <60.00%> (-6.59%) ⬇️
...ntraldogma/client/spring/CentralDogmaSettings.java 71.42% <75.00%> (+0.46%) ⬆️
...nt/armeria/AbstractArmeriaCentralDogmaBuilder.java 90.76% <100.00%> (+0.14%) ⬆️
...erver/internal/storage/PurgeSchedulingService.java 70.11% <0.00%> (-6.90%) ⬇️
...centraldogma/server/internal/api/WatchService.java 72.88% <0.00%> (-3.39%) ⬇️
.../linecorp/centraldogma/client/AbstractWatcher.java 78.14% <0.00%> (-1.33%) ⬇️
...raldogma/server/internal/api/ContentServiceV1.java 85.07% <0.00%> (-0.75%) ⬇️
...internal/storage/DirectoryBasedStorageManager.java 64.19% <0.00%> (ø)
...internal/storage/repository/git/GitRepository.java 76.19% <0.00%> (+0.25%) ⬆️
...internal/replication/ZooKeeperCommandExecutor.java 78.78% <0.00%> (+0.73%) ⬆️

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 8a32763...821eec8. Read the comment docs.

ikhoon and others added 5 commits May 11, 2022 18:28
…rp/centraldogma/client/spring/CentralDogmaClientAutoConfiguration.java

Co-authored-by: Klemen Košir <klemen.kosir@kream.io>

// TODO(ikhoon): Randomize the port number to avoid flakiness.
private static final Lock lock = new ReentrantLock();
private static final int TEST_SERVER_PORT = 56463;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this fixed port?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because CentralDogma client bean needs a pre-known hostname (host:port) to access a server.
https://github.com/line/centraldogma/pull/692/files#diff-4264d927674e9eb11e891891fce0d0305d917df8904caff14930b7df456b2a7f

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thanks for the explanation.
I'm a bit worried about the flakiness but we can fix it later if this causes the problem. 😉

Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks, @ikhoon!


// TODO(ikhoon): Randomize the port number to avoid flakiness.
private static final Lock lock = new ReentrantLock();
private static final int TEST_SERVER_PORT = 56463;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thanks for the explanation.
I'm a bit worried about the flakiness but we can fix it later if this causes the problem. 😉

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Overall looks good! Left a minor question 🙇

* @return {@code null} if not specified.
*/
@Nullable
public Long initializationTimeoutMillis() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not strong on this, but initialization sounds a little general (vs. endpointReadyTimeoutMillis or endpointInitializationTimeoutMillis)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because 1) I didn't want to expose the implementation detail and 2) we might add another logic later to warm up CentralDogma somehow.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see 👍 Makes sense

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Thanks @ikhoon 👍 🙇 👍

@minwoox minwoox merged commit 07bae65 into line:master Jul 6, 2022
@minwoox
Copy link
Contributor

minwoox commented Jul 6, 2022

Thanks!

@ikhoon ikhoon deleted the initialization-timeout branch March 31, 2023 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wait for the initial endpoints of a CentralDogma client for Spring integrations
4 participants