Will fix them today
---
Luca Burgazzoli
On Wed, Jun 1, 2016 at 8:56 AM, Claus Ibsen <claus.ib...@gmail.com> wrote:
> Hi
>
> Looks good there is only a few spots I have some comments.
>
> - There is some code that has not null check for load balancer /
> server list strategy. The kubernetes component can do without that
> when you use ENV or DNS as lookup.
>
> - There was a place where you do some logging and it says Consul when
> its in fact generic
>
> - You have label("ServiceCall") but that label name seems wrong. eg
> notice that label is like a "tag" not a leading label in a UI form
> etc. Yeah we should maybe have named it "tag" or "group" instead.
>
> - In the Java DSL we often have it return Type so you do not change
> the type and makes the fluent easier to use. Only if you need to do
> some more advanced configuration then we have other methods that
> return the ServiceCallDefinition type so you can call specialized
> method on that in the Java DSL.
>
>
>
> serviceCall().name("foo").loadBalancer("random").end()
> // here you have the ServiceCallDefinition in the DSL to call
> specialized methods. But have to call end() to end it
>
> And then with short hand that just return type
>
> serviceCall("foo")
> // here the Type is not changed and you just want to call the
> service foo, and NOT do any other configuration. Then you do not need
> the end()
>
>
>
>
>
> On Tue, May 31, 2016 at 4:54 PM, Luca Burgazzoli <lburgazz...@gmail.com>
> wrote:
>> I've committed some more code to my branch [2], it is not yet complete
>> (i.e. I still have to make ribbon code to use the new common classes
>> and some APIs are not clean enough) but a feedback would be really
>> welcome.
>>
>> ---
>> Luca Burgazzoli
>>
>>
>> On Mon, May 30, 2016 at 10:55 AM, Claus Ibsen <claus.ib...@gmail.com> wrote:
>>> On Mon, May 30, 2016 at 10:49 AM, Luca Burgazzoli <lburgazz...@gmail.com>
>>> wrote:
>>>> So like serviceCall("myServiceCall").configurationRef("myConf") ?
>>>
>>> Yeah there is already a DSL for that named
>>> serviceCallConfiguration("myConf")
>>>
>>>
>>>
>>>
>>>> ---
>>>> Luca Burgazzoli
>>>>
>>>>
>>>> On Mon, May 30, 2016 at 10:26 AM, Claus Ibsen <claus.ib...@gmail.com>
>>>> wrote:
>>>>> On Mon, May 30, 2016 at 10:16 AM, Luca Burgazzoli <lburgazz...@gmail.com>
>>>>> wrote:
>>>>>> do you mean something like serviceCallRef("myServiceCall") ?
>>>>>> ---
>>>>>
>>>>> No you need to provide
>>>>>
>>>>> - a) name of service to call
>>>>> - b) reference to configuration of service
>>>>>
>>>>> a = mandatory
>>>>> b = optional. As if there is only 1 configuration then use that.
>>>>>
>>>>>
>>>>>
>>>>>> Luca Burgazzoli
>>>>>>
>>>>>>
>>>>>> On Sun, May 29, 2016 at 9:42 AM, Claus Ibsen <claus.ib...@gmail.com>
>>>>>> wrote:
>>>>>>> On Thu, May 26, 2016 at 7:30 PM, Luca Burgazzoli
>>>>>>> <lburgazz...@gmail.com> wrote:
>>>>>>>> ---
>>>>>>>> Luca Burgazzoli
>>>>>>>>
>>>>>>>>
>>>>>>>> On Thu, May 26, 2016 at 7:06 PM, Claus Ibsen <claus.ib...@gmail.com>
>>>>>>>> wrote:
>>>>>>>>> Hi Luca
>>>>>>>>>
>>>>>>>>> Yeah its good to get more eyes on this new set of code. When I created
>>>>>>>>> kubernetes and ribbon there was sure some overlap of code that could
>>>>>>>>> be shared, but I didn't push for much default/abstract code in
>>>>>>>>> camel-core because there its new code and I also wanted to see what
>>>>>>>>> consul, etcd, zookeeper and other distributed systems may
>>>>>>>>> need/require.
>>>>>>>>>
>>>>>>>>> I like the idea of the impl.remote package to have some base
>>>>>>>>> implementation there.
>>>>>>>>>
>>>>>>>>> Your current branch [2] has a good set of reusable code although its
>>>>>>>>> tied to consul currently, so that would need to be made abstract so it
>>>>>>>>> can be reuse by kuernetes and maybe also ribbon as well (where it
>>>>>>>>> makes sense).
>>>>>>>>
>>>>>>>> I've removed some consul specific stuffs that I left by mistake,
>>>>>>>> should be
>>>>>>>> a little tidy now.
>>>>>>>>
>>>>>>>> An aspect to take into account is how to make it easy to configure
>>>>>>>> ServiceCallServerListStrategy in case we use
>>>>>>>> DefaultServiceCallProcessor
>>>>>>>> maybe something like:
>>>>>>>>
>>>>>>>> serviceCall()
>>>>>>>> .name("my-service")
>>>>>>>> .roundRobinLoadBalancer()
>>>>>>>> .consulServerListStrategy()
>>>>>>>> .type(Strategy.ON_DEMAND)
>>>>>>>> .url("http://consul-host:8500";)
>>>>>>>> .dc("west")
>>>>>>>> .end()
>>>>>>>>
>>>>>>>> Too ugly ?
>>>>>>>>
>>>>>>>
>>>>>>> Yeah possible - its always tricky to find the right balance.
>>>>>>>
>>>>>>> I wonder if you may want to do this in the configuration, and then in
>>>>>>> the routes with serviceCall you then just need to refer to the service
>>>>>>> name / url to be used - then all the round robin, service list, and so
>>>>>>> on are configured outside the route in the configuration.
>>>>>>>
>>>>>>> We could also leave those in the route DSLs as well so you can
>>>>>>> override the configuration, so you can use
>>>>>>>
>>>>>>> serviceCall().name("foo").consulConfiguraiton().dc("west").end()
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> But then on the other hand if you just want to call a single service
>>>>>>> you may want to do it all in the route without the configuration. But
>>>>>>> if we look at rest-dsl then it separates the configuration from the
>>>>>>> REST endpoint.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>> An aspect we haven't added yet could be to find out if we can expose
>>>>>>>>> some JMX attributes and operations for thise service call EIP as well?
>>>>>>>>> And then maybe some Camel commands so you can manage/list it from
>>>>>>>>> karaf and other CLIs. But this part is more "nice to have" and a bit
>>>>>>>>> "eye candy" but still somewhat cool.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Thu, May 26, 2016 at 4:13 PM, Luca Burgazzoli
>>>>>>>>> <lburgazz...@gmail.com> wrote:
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> I'm playing a little bit with the new ServiceCall EIP by adding
>>>>>>>>>> support for
>>>>>>>>>> consul service discovery and I've committed some code in my own
>>>>>>>>>> branch [1].
>>>>>>>>>>
>>>>>>>>>> I borrowed most of the code from camel-kubernetes and as it ended up
>>>>>>>>>> being
>>>>>>>>>> almost a clone, I've tried to make some base/default classes as what
>>>>>>>>>> really
>>>>>>>>>> make the difference is the implementation of
>>>>>>>>>> ServiceCallServerListStrategy
>>>>>>>>>> and ServiceCallLoadBalancer so to add a simple discovery engine you
>>>>>>>>>> only
>>>>>>>>>> need to implement your own ServiceCallServerListStrategy and
>>>>>>>>>> eventually your
>>>>>>>>>> own ServiceCallLoadBalancer (i.e. for ribbon).
>>>>>>>>>>
>>>>>>>>>> Does it make sense ?
>>>>>>>>>>
>>>>>>>>>> [1] https://github.com/lburgazzoli/apache-camel/tree/CAMEL-9989
>>>>>>>>>> [2]
>>>>>>>>>> https://github.com/apache/camel/compare/master...lburgazzoli:CAMEL-9989?expand=1
>>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>> Luca Burgazzoli
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Claus Ibsen
>>>>>>>>> -----------------
>>>>>>>>> http://davsclaus.com @davsclaus
>>>>>>>>> Camel in Action 2: https://www.manning.com/ibsen2
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Claus Ibsen
>>>>>>> -----------------
>>>>>>> http://davsclaus.com @davsclaus
>>>>>>> Camel in Action 2: https://www.manning.com/ibsen2
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Claus Ibsen
>>>>> -----------------
>>>>> http://davsclaus.com @davsclaus
>>>>> Camel in Action 2: https://www.manning.com/ibsen2
>>>
>>>
>>>
>>> --
>>> Claus Ibsen
>>> -----------------
>>> http://davsclaus.com @davsclaus
>>> Camel in Action 2: https://www.manning.com/ibsen2
>
>
>
> --
> Claus Ibsen
> -----------------
> http://davsclaus.com @davsclaus
> Camel in Action 2: https://www.manning.com/ibsen2
