On Fri, Sep 26, 2014 at 5:29 PM, Nirmal Fernando <nirmal070...@gmail.com> wrote:
> > > On Fri, Sep 26, 2014 at 4:54 PM, Akila Ravihansa Perera < > raviha...@wso2.com> wrote: > >> Hi >> >> > Found that some of the AutoscalerImpl operations are invoked by directly >> > instantiating the AutoscalerImpl. I think this is not good. >> > AutoscalerImpl meant to be a web service, which needs to be accessed >> only >> > via service stubs, referring to the WSDL. >> >> +1. This is completely against the web services model. Just noticed that >> >> org/apache/stratos/autoscaler/client/cloud/controller/CloudControllerClient.java >> class is creating an instance of AutoscalerImpl. > > > Why do CloudControllerClient needs to call Autoscaler Service in the first > place? Also can you point to the exact code segment? > We are calling to get kubernetesMasterIP and portRange. These should be set to the MemberContext when calling startContainers() CC API. I guess CC can call Autoscaler APIs get these? > >> Will have to fix >> this. Nice finding Sajith! >> >> > Another small thing to be highlight, regarding the interface naming. >> > Currently Autoscaler service's interface and CC 's interface naming are >> not >> > consistent. >> > IMO the part 'interface' should not be go in the name of the interface, >> its >> > no hard and fast rule, but I think its better to maintain the >> consistency, >> > So I would like to suggest to rename AutoScalerServiceInterface as >> > AutoScalerService. >> > >> > thoughts? >> >> +1 >> >> I''ll create JIRAs to track these tasks. I too think that it is >> important to clean up this code in a proper way. >> >> >> Also, how about in "updateX" operations if the updated object is >> returned >> >> instead of the boolean? That will make the programming in client side >> easy, >> >> otherwise it will need to do another "getX" call to retrieve the >> updated >> >> object ? >> >> +1 >> Yes, that would be make things easy in client side. >> >> >> >>> One KubernetesHost is associated with only one KubernetesGroup. Hence >> >>> ideally KubernetesHost class should have a reference to >> KubernetesGroup. I >> >>> mean KubernetesHost should have a KubernetesGroupId filed. >> >>> >> >>> There are some advantages of doing this. >> >>> >> >>> We need to pass only KubernetesHost object when calling >> >>> addKubernetesHost() method. Currently we are passing both >> KubernetesGroupId >> >>> and KubernetesHost object. >> >>> removeKubernetesHost() method will become more efficient. I could see >> >>> that you are looping through all Kubernetes Groups in-order to >> identify >> >>> which Group has the given host. Then you are removing the host from >> that >> >>> group. If you store the KubernetesGroupId in the KubernetesHost >> object, >> >>> then no need to loop through all the KubernetesGroups. Of course we >> need to >> >>> pass the KubernetesHost object to the removeKubernetesHost() method. >> >>> updateKubernetesHost() would work faster because of the same reason as >> >>> above. >> >>> >> >>> It will improve the performance. >> >> IMO, doing that will only add more complexity to the code. I don't see >> a beneficial performance gain for that kind of increase in the >> complexity. >> >> If we do that, then we will have to add more annotations to hide >> kubernetesGroupId in the json object serialization. Because we do not >> want users to change the kubernetes groupId when updating host >> objects. This is un-necessary complexity for me. So I'm - 0 on this. >> What do others think? >> >> Thanks >> > > > > -- > Best Regards, > Nirmal > > Nirmal Fernando. > PPMC Member & Committer of Apache Stratos, > Senior Software Engineer, WSO2 Inc. > > Blog: http://nirmalfdo.blogspot.com/ > -- Rajkumar Rajaratnam Software Engineer | WSO2, Inc. Mobile +94777568639 | +94783498120
