GH-1334: Support for eagerly initializing zuul Ribbon contexts #1749
Conversation
|
This PR is more of a POC again - this is to see if the direction is okay. |
Codecov Report
@@ Coverage Diff @@
## master #1749 +/- ##
==========================================
+ Coverage 75.22% 75.39% +0.17%
==========================================
Files 200 203 +3
Lines 6208 6243 +35
Branches 777 781 +4
==========================================
+ Hits 4670 4707 +37
+ Misses 1199 1196 -3
- Partials 339 340 +1
Continue to review full report at Codecov.
|
| @@ -80,28 +80,29 @@ public RibbonLoadBalancerContext getLoadBalancerContext(String serviceId) { | |||
| } | |||
|
|
|||
| static <C> C instantiateWithConfig(AnnotationConfigApplicationContext context, | |||
| Class<C> clazz, IClientConfig config) { | |||
| Class<C> clazz, IClientConfig config) { | |||
There was a problem hiding this comment.
Please revert unneeded formatting, makes it hard to see meaningful changes.
There was a problem hiding this comment.
Yes, not sure how the re-formatting snuck in - will revert back.
| @@ -161,6 +163,14 @@ public SendForwardFilter sendForwardFilter() { | |||
| return new SendForwardFilter(); | |||
| } | |||
|
|
|||
| @Bean | |||
| @ConditionalOnProperty(value = "zuul.context.startup.enabled", matchIfMissing = false) | |||
There was a problem hiding this comment.
I'd probably have a different name. zuul.ribbon.eager-load=true or something with more meaning.
| * | ||
| * @author Biju Kunjummen | ||
| */ | ||
| public class RibbonApplicationContextInitializer |
There was a problem hiding this comment.
I think there should be a generic ribbon initializer as well. That just takes a simple list of ribbon client names (for non-zuul).
There was a problem hiding this comment.
Yes, makes sense. This class does that though - taking in a list of Ribbon Client names and initializing the application contexts - would a different name help?
There was a problem hiding this comment.
I'm talking about configuration properties.
There was a problem hiding this comment.
I have introduced a configuration properties now @spencergibb - RibbonEagerLoadProperties with the following properties:
ribbon.eager-load.enabled, ribbon.eager-load.serviceIds and driving the eager registration of Ribbon Client configurations based on these
bijukunjummen
force-pushed
the
GH-1334
branch
4 times, most recently
from
77e850c to
43c41e1
Compare
| } | ||
| } | ||
|
|
||
| // @Override |
There was a problem hiding this comment.
Commented out @Override?
There was a problem hiding this comment.
Ouch, yes, will restore it :-)
| * | ||
| * @author Biju Kunjummen | ||
| */ | ||
| @ConfigurationProperties(prefix = "ribbon.eager-load") |
There was a problem hiding this comment.
Should the namespace be ribbon? Since these properties are specific to Spring Cloud Ribbon, should they be spring.cloud.ribbon? Not sure on the conventions we have used in the past. @spencergibb, what do you think?
There was a problem hiding this comment.
s-c-netflix doesn't follow that convention because we followed the existing netflix configuration
There was a problem hiding this comment.
OK wasnt sure, thanks for clarifying.
There was a problem hiding this comment.
Thanks @spencergibb, @ryanjbaxter, this discussion was useful to me too. Anything else that comes to mind on this. Quick question, this PR affects the documentation also, so is how is that typically handled - should I update that too as part of this PR?
There was a problem hiding this comment.
@bijukunjummen it would be great if the docs were part of this.
There was a problem hiding this comment.
Done @spencergibb, @ryanjbaxter - I have added in the documentation now.
…ed client contexts
#1334 - Supports a way to eagerly initialize Zuul Ribbon Contexts
Based on availability of a new flag -
zuul.ribbon.eager-load.enableda Spring listener (ApplicationReadyEvent) goes through the service id's in theZuulPropertiesand initializes the Application contexts registered for themBased on the availability of a flag -
ribbon.eager-load.enabled, a listener goes through the serviceid's available viaribbon.eager-load.serviceIdsand initializes the application contexts registered.