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.

Sign up for GitHub

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

Jump to bottom

Programmatically configured options on ServerBuilder are overridden by ArmeriaSettings #5009

Closed
KarboniteKream opened this issue Jul 7, 2023 · 6 comments · Fixed by #5692
Closed

Programmatically configured options on ServerBuilder are overridden by ArmeriaSettings #5009

KarboniteKream opened this issue Jul 7, 2023 · 6 comments · Fixed by #5692
Labels
defect
Milestone
1.29.0

Comments

@KarboniteKream
Copy link
Member

KarboniteKream commented Jul 7, 2023 •
edited

When using the spring-boot{2,3}-autoconfigure library, the provided ArmeriaSettings autoconfiguration is overridding any user-supplied configuration in code.

For example, when using builder.gracefulShutdownTimeout(x, y), the provided values are applied toServerBuilder, but before the server is built, they're overwritten by the autoconfiguration classes.

This can be reproduced by setting a breakpoint here:

armeria/core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

Lines 824 to 825 in 2e50636

public ServerBuilder gracefulShutdownTimeout(Duration quietPeriod, Duration timeout) {
requireNonNull(quietPeriod, "quietPeriod");

Then, you should set a custom value here:

armeria/examples/spring-boot-minimal/src/main/java/example/springframework/boot/minimal/HelloConfiguration.java

Lines 21 to 24 in 2e50636

@Bean
public ArmeriaServerConfigurator armeriaServerConfigurator(HelloAnnotatedService service) {
// Customize the server using the given ServerBuilder. For example:
return builder -> {

Once the application is started, you can observe the provided value being overriden by ArmeriaSettings.

As a temporary workaround, you can inject ArmeriaSettings and use setGracefulShutdownQuietPeriodMillis()
@jrhee17 jrhee17 added this to the 1.25.0 milestone Jul 7, 2023
@jrhee17 jrhee17 added the defect label Jul 7, 2023
@KarboniteKream KarboniteKream changed the title Programmatically set options on ServerBuilder are overridden by ArmeriaSettings Programmatically configured options on ServerBuilder are overridden by ArmeriaSettings Jul 7, 2023
@ikhoon
Copy link
Contributor

ikhoon commented Jul 13, 2023

Some users may not want to override the values provided by ArmeriaSettings.
How about adding a flag that determines whether the value of Armeriasettings can be overridden?

If overridable is set to true, ArmeriaSettings will be applied before ArmeriaServerConfigurator is applied.

armeria:
  overridable: true

@KarboniteKream Can this approach meet your requirements?

@jrhee17
Copy link
Contributor

jrhee17 commented Jul 13, 2023

If overridable is set to true, ArmeriaSettings will be applied before ArmeriaServerConfigurator is applied.

So if overridable: true, then which settings will be overridable? I think we should clearly define which settings are overridable if we choose to go with this option.

Some users may not want to override the values provided by ArmeriaSettings.

Overall I thought it is reasonable that configuration properties are overridden programmatically. I'm curious what kind of scenario/properties users may not want to override 😄

@ikhoon
Copy link
Contributor

ikhoon commented Jul 13, 2023

If I wrote a novel... Let's say an ArmeriaServerConfigurator bean is injected by other modules or libraries. The code owner may not want ArmeriaServerConfigurator to change. But it would not be the normal case.

I think we should clearly define which settings are overridable if we choose to go with this option.

Good point. ArmeriaServerConfigurator couldn't override some settings, such as decorators or internal services.

Thinking about it again, it seems that it is sufficient to modify to apply ArmeriaSettings first and then apply ArmeriaServerConfigurator later.

@jrhee17
Copy link
Contributor

jrhee17 commented Jul 13, 2023

If I wrote a novel... Let's say an ArmeriaServerConfigurator bean is injected by other modules or libraries. The code owner may not want ArmeriaServerConfigurator to change. But it would not be the normal case.

I agree this may be a case.
Having said this, I think spring provides enough customization (e.g. disabling beans, turning off autoconfiguration, etc.) so that we don't have to worry about this too much

@KarboniteKream
Copy link
Member Author

Can this approach meet your requirements?

That would work, but I agree with @jrhee17. I would expect ArmeriaSettings be the global defaults (which I can modify on a per-application basis), but be able to programmatically override it for specific services.

About ArmeriaServerConfigurator beans, hopefully the module/library would provide the configuration classes that can either be disabled or imported on demand, and not have major impact.

That's just my two cents. I'll trust your final decision, since you have more experience in this area.

@jrhee17 jrhee17 modified the milestones: 1.25.0, 1.26.0 Aug 16, 2023
@minwoox minwoox modified the milestones: 1.26.0, 1.27.0 Oct 11, 2023
@ikhoon ikhoon modified the milestones: 1.27.0, 1.28.0 Jan 24, 2024
@jrhee17 jrhee17 modified the milestones: 1.28.0, 1.29.0 Apr 8, 2024
@minwoox minwoox mentioned this issue May 21, 2024
Merged
@minwoox
Copy link
Member

minwoox commented May 21, 2024

@KarboniteKream My apologies for the delay in addressing this. I have created a PR to resolve this issue. PTAL. 🙇

@minwoox minwoox closed this as completed in fc41ef2 Jun 4, 2024
Dogacel pushed a commit to Dogacel/armeria that referenced this issue Jun 8, 2024
@minwoox @Dogacel
Ensure ArmeriaServerConfigurator overrides properties from `Armeria…
d6c8e07 
…Settings` (line#5692)

Motivation:
The properties from `ArmeriaSettings` should be considered as default values and should be overridden by the beans that implement `ArmeriaServerConfigurator`. This ensures that custom configurations provided by `ArmeriaServerConfigurator` take precedence over the default settings.

Modifications:
- Adjusted the order of configuration application so that `ArmeriaServerConfigurator` is applied last, ensuring it can override properties set by `ArmeriaSettings`.

Result:
- `ArmeriaServerConfigurator` now properly overrides the properties set by `ArmeriaSettings`.
- Close line#5009
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
defect
Projects
None yet
Milestone
1.29.0
Development

Successfully merging a pull request may close this issue.

Ensure ArmeriaServerConfigurator overrides properties from ArmeriaSettings minwoox/armeria
4 participants
@KarboniteKream @ikhoon @jrhee17 @minwoox