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. 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? > You can easily prevent users from changing the kubernetesGroupId. You check whether the given host is in the given group. If not, you simply throw an exception. > > Thanks > -- Rajkumar Rajaratnam Software Engineer | WSO2, Inc. Mobile +94777568639 | +94783498120
