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

Provide a way to configure server for a plugin #883

Merged
merged 7 commits into from
Sep 26, 2023

Conversation

minwoox
Copy link
Member

@minwoox minwoox commented Sep 13, 2023

Motivation:
Currently, there's no way to configure the Armeria server outside of the server module. We can perhaps do this by providing DI, SPI, or adding a method to Plugin.

Modification:

  • Add AllReplicasPlugin class for configuring Central Dogma server

Result:

  • You can now add configurations to the Central Dogma server for your plugin using AllReplicasPlugin.init() method.

Motivation:
Currently, there's no way to configure the Armeria server outside of the server module.
We can perhaps do this by providing DI, SPI, and adding a method to `Plugin`.
I think SPI is the most straightforward so I choosed it.
We might want to change to DI later if detailed customization needs

Modification:
- Add `ArmeriaServerConfigurator` that is loaded via SPI and configures the Armeria Server.

Result:
- You can now configure the server using `ArmeriaServerConfigurator`.
@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (75dfb2b) 65.94% compared to head (1aef397) 65.95%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #883      +/-   ##
============================================
+ Coverage     65.94%   65.95%   +0.01%     
- Complexity     3374     3381       +7     
============================================
  Files           360      362       +2     
  Lines         14020    14037      +17     
  Branches       1502     1503       +1     
============================================
+ Hits           9245     9258      +13     
- Misses         3919     3924       +5     
+ Partials        856      855       -1     
Files Coverage Δ
...com/linecorp/centraldogma/server/CentralDogma.java 75.32% <100.00%> (+0.54%) ⬆️
...om/linecorp/centraldogma/server/plugin/Plugin.java 100.00% <ø> (ø)
...corp/centraldogma/server/plugin/PluginContext.java 100.00% <ø> (ø)
.../centraldogma/server/plugin/PluginInitContext.java 100.00% <100.00%> (ø)
.../centraldogma/server/plugin/AllReplicasPlugin.java 66.66% <66.66%> (ø)

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Changes look good to me 👍 Thanks @minwoox 🙇 👍 🙇

@minwoox minwoox changed the title Add ArmeriaServerConfigurator for configuring server via SPI Provide a way to configure server for a plugin Sep 14, 2023
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks, @minwoox!

/**
* Creates a new instance.
*/
public PluginInitContext(CentralDogmaConfig config, ServerBuilder serverBuilder) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public PluginInitContext(CentralDogmaConfig config, ServerBuilder serverBuilder) {
PluginInitContext(CentralDogmaConfig config, ServerBuilder serverBuilder) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, I cannot remove the public modifier because the class is in a different package.

/**
* A context that is used to pass when calling {@link AllReplicasPlugin#init(PluginInitContext)}.
*/
public final class PluginInitContext {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question) I was thinking we would need at least ProjectManager, CommandExecutor will be needed to build services which can write to CentralDogma storage. Is there no need for these values? (it looks like they are available at init time).

I actually expect we may also want access to MeterRegistry, and actually all values in PluginContext eventually.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, I thought somehow they are not created when the init method is called. 😅
Let me fix that. 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. PTAL. 😉

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.

Sorry about the late check! Looks good to me! 🙇 👍 🚀

@minwoox
Copy link
Member Author

minwoox commented Sep 26, 2023

Let me merge this for now to keep working on the control plane.

@minwoox minwoox merged commit 027f32e into line:main Sep 26, 2023
10 checks passed
@minwoox minwoox deleted the server_configurator branch September 26, 2023 05:39
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.

3 participants