-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 07/10/2015 09:22 PM, Rohit Yadav wrote: > Ping Wilder - any progress/plan on moving forward (perhaps after > 4.6?). > I don't think there is? Since Ubuntu 14.04 doesn't support Java 8 in any package form we can't really continue. Ubuntu 16.04 will ship with Java 8 and that will be the next LTS. Wido > On 01-May-2015, at 4:07 pm, Wilder Rodrigues > <wrodrig...@schubergphilis.com<mailto:wrodrig...@schubergphilis.com>> > wrote: > > Hi Marcus, > > Sorry for the push, I think after doing the whole > CitrixResourceBase refactor I also got a bit attached to the whole > thing/solution. ;) > > Thanks for the input you gave. I will finish the refactor and apply > it to both implementations. > > Cheers, Wilder > > > On 01 May 2015, at 09:06, Marcus > <shadow...@gmail.com<mailto:shadow...@gmail.com><mailto:shadowsor@gmai l.com>> > wrote: > > Oh, and of course the annotation added to the wrapper looks like: > > > ... > > @ResourceWrapper(handles = CheckHealthCommand.class) > > public final class LibvirtCheckHealthCommandWrapper > > ... > > > maybe 'wraps' or 'wrapperfor' would be better than 'handles' in > your naming scheme. You get the idea. > > On Thu, Apr 30, 2015 at 11:59 PM, Marcus > <shadow...@gmail.com<mailto:shadow...@gmail.com>> wrote: I agree, > this wrapper is a good step forward. It's totally fine to continue > on that path because it is obviously better and makes it easy to > switch to autodetection anytime later by simply adding the > annotation. Sorry if I got a bit passionate about that, but as you > mention I also get tired of adding things in multiple places, and > the annotations have worked well in the API and provide a good > model to emulate for consistency. > > I can't share code, because these extensions to > LibvirtComputingResource that I've provided for other companies > have not been open sourced. I can speak more generically though > about methods. > > To answer question "a", reflection allows you to do something > like: > > Reflections reflections = new > Reflections("com.cloud.hypervisor.kvm.resource.wrapper"); > Set<Class<? extends CommandWrapper>> wrappers = > reflections.getSubTypesOf(CommandWrapper.class); > > So here in "new Reflections" we are automatically filtering for > just the wrappers that would apply to the KVM plugin. Then to > finish it off, you iterate through the wrappers and do: > > ResourceWrapper annotation = > wrapper.getAnnotation(ResourceWrapper.class); > citrixCommands.put(annotation.handles(), wrapper.newInstance()); > > Sorry, I guess that's four lines, plus the relevant for loop. And > probably a null check or something for the annotation. You also > have to add the annotation class itself, and add a line for the > annotation in each wrapper, but in the end when we add new > Commands, we won't have to touch anything but the new class that > handles the command. > > > public @interface ResourceWrapper { > > Class<? extends Command> handles(); > > } > > There's an example of something similar to this in > KVMStoragePoolManager.java (annotation is StoragePoolInfo.java). > This example has actually been adapted from that. Also to a lesser > extent in the API server, but it is spread across a bunch of > classes. > > On Thu, Apr 30, 2015 at 10:41 PM, Wilder Rodrigues > <wrodrig...@schubergphilis.com<mailto:wrodrig...@schubergphilis.com>> > wrote: Hi Marcus, > > Thanks for the email… I’m always in for improvements. But why can’t > you share the code? > > Few points below: > > 1. I added an subclassing example of LibvirtComputingResource > because you mentioned it in a previous email: > > On 23 Apr 2015, at 17:26, Marcus > <shadow...@gmail.com<mailto:shadow...@gmail.com>> wrote: > > I mentioned the reflection model because that's how I tend to > handle the commands when subclassing LibvirtComputingResource. > > 2. Current situation with LibvirtComputingResource on Master is: > > a. 67 IFs b. 67 private/protected methods that are used only there > c. If a new Command is added it means we will have a new IF and a > new private method e. Maintenance is hell, test is close to zero > and code quality is below expectations > > That being said, the main idea with the refactor is to change > structure only, not behaviour. So what I’m doing is to simply move > the code out the LibvirtCompRes and write tests for it, keeping the > behaviour the same - to be done in a next phase. If you look at the > changes you will see that some wrappers are already 100% covered. > However, some others have 4% or 8% (not that much though). I would > like to refactor that as well, but that could change behaviour > (mentioned above) which I don’t want to touch now. > > 3. With the new situation: > > a. No IFs b. All methods wrapped by other classes (command > wrappers) - loosely coupled, easier to test and maintain c. If a > new Command is added we would have to add a command wrapper and 1 > line in the request wrapper implementation ( I know, it hurts you a > bit) - but please bear with me for the good news. > > 4. the warnings are due to that: Hashtable<Class<? extends > Command>, CommandWrapper>() > > No big deal. > > As I understood from your first paragraph we would have to > annotated the commands classes, right? I mean, all of them. > > That’s something I wouldn’t do in this phase, to be honest. It > might seem harmless to do, but I like to break things down a bit > and have more isolation in my changes. > > What’s next: I will finish the refactor with the request wrapper as > it is. For me it is no problem do add the lines now and remove them > in 1 week. Most of the work is concentrated in the tests, which I’m > trying as hard as I can to get them in the best way possible. Once > it’s done and pushed to master, I will analyse what we would need > to apply the annotation. > > But before I go to bring the kids to school, just one question: > > a. The “handle” value, in the annotation, would have the wrapper > class that would be used for that command, right? Now let’s get 1 > command as example: CheckHealthCommand. Its wrapper implementation > differs per hypervisor (just like all the other wrapper commands > do). I’m not taking the time to really think about it now, but how > would we annotated the different wrappers per command? > > Thanks again for your time. > > Cheers, Wilder > > > On 30 Apr 2015, at 22:52, Marcus > <shadow...@gmail.com<mailto:shadow...@gmail.com>> wrote: > > Ok. I wish I could share some code, because it isn't really as big > of a deal as it sounds from your reasoning. It is literally just 3 > lines on startup that fetch anything with the '@AgentExecutor' > annotation and stores it in a hash whose key is the value from > @AgentExecutor's 'handles' property. Then when a *Command comes it > it is passed to the appropriate Executor class. > > Looking at CitrixRequestWrapper, the 3 lines I mention are almost > identical in function to your init method, just that it uses the > annotation to find all of the commands, rather than hardcoding > them. We use the same annotation design for the api side of the > code on the management server, which allows the api commands to be > easier to write and self-contained (you don't have to update other > code to add a new api call). It makes things easier for novice > developers. > > This implementation is no less typesafe than the previous design > (the one with all of the instanceof). It didn't require any casting > or warning suppression, either, as the wrapper does. > > Extending LibvirtComputingResource is not ideal, and doesn't work > if multiple third parties are involved. Granted, there hasn't been > a lot of demand for this, nevertheless it's particularly important > for KVM, where the Command classes are executed on the hypervisor > it's not really feasible to just dump the code in your management > server-side plugin like some plugins do. > > In reviewing the code, the two implementations are really very > close. If you just updated init to fetch the wrappers based on > either an annotation or the class they extend, or something along > those lines so this method doesn't have to be edited every time a > command is added, that would be more or less the same thing. The > the KVM agent would be pluggable like the management server side > is. > > On Thu, Apr 30, 2015 at 12:55 PM, Wilder Rodrigues > <wrodrig...@schubergphilis.com<mailto:wrodrig...@schubergphilis.com>> > wrote: Hi Marcus, > > Apologies for taking so much time to reply to your email, but was, > and still am, quite busy. :) > > I would only use reflection if that was the only way to do it. The > use of reflection usually makes the code more complex, which is not > good when we have java developers in all different levels (from jr. > do sr) working with cloudstack. It also makes us lose the type > safety, which might also harm the exception handling if not done > well. In addition, if we need to refactor something, the IDE is no > longer going to do few things because the refection code cannot be > found. > > If someone will need to extend the LibvirtComputingResource that > would be no problem with the approach I’m using. The > CitrixResourceBase also has quite few sub-classes and it works just > fine. > > I will document on the wiki page how it should be done when > sub-classing the LibvirtComputingResource class. > > In a quick note/snippet, one would do: > > public class EkhoComputingResource extends LibvirtComputingResource > { > > @Override public Answer executeRequest(final Command cmd) { > > final LibvirtRequestWrapper wrapper = > LibvirtRequestWrapper.getInstance(); try { return > wrapper.execute(cmd, this); } catch (final Exception e) { return > Answer.createUnsupportedCommandAnswer(cmd); } } } > > > In the flyweight where I keep the wrapper we could have (): > > final Hashtable<Class<? extends Command>, CommandWrapper> > linbvirtCommands = new Hashtable<Class<? extends Command>, > CommandWrapper>(); linbvirtCommands.put(StopCommand.class, new > LibvirtStopCommandWrapper()); > > final Hashtable<Class<? extends Command>, CommandWrapper> > ekhoCommands = new Hashtable<Class<? extends Command>, > CommandWrapper>(); linbvirtCommands.put(StopCommand.class, new > EkhoStopCommandWrapper()); > > resources.put(LibvirtComputingResource.class, linbvirtCommands); > resources.put(EkhoComputingResource.class, ekhoCommands); > > But that is needed only if the StopCommand has a different > behaviour for the EkhoComputingResource. > > Once a better version of the documentation is on the wiki, I will > let you know. > > On other matters, I’m also adding unit tests for all the changes. > We already went from 4% to 13.6% coverage in the KVM hypervisor > plugin. The code I already refactored has 56% of coverage. > > You can see all the commits here: > https://github.com/schubergphilis/cloudstack/tree/refactor/libvirt_res ource > > Cheers, Wilder > > On 23 Apr 2015, at 17:26, Marcus > <shadow...@gmail.com<mailto:shadow...@gmail.com>> wrote: > > Great to see someone working on it. What sorts of roadblocks came > out of reflection? How does the wrapper design solve the > pluggability issue? This is pretty important to me, since I've > worked with several companies now that end up subclassing > LibvirtComputingResource in order to handle their own Commands on > the hypervisor from their server-side plugins, and changing their > 'resource' to that in agent.properties. Since the main agent class > needs to be set at agent join, this is harder to manage than it > should be. > > I mentioned the reflection model because that's how I tend to > handle the commands when subclassing LibvirtComputingResource. I > haven't had any problems with it, but then again I haven't tried to > refactor 5500 lines into that model, either. > > On Thu, Apr 23, 2015 at 1:17 AM, Wilder Rodrigues > <wrodrig...@schubergphilis.com<mailto:wrodrig...@schubergphilis.com>> > wrote: > > Hi Marcus, > > I like the annotation idea, but reflection is trick because it > hides some information about the code. > > Please, have a look at the CitrixResourceBase after the refactor I > did. It became quite smaller and test coverage was improved. > > URL: > https://cwiki.apache.org/confluence/display/CLOUDSTACK/Refactoring+Xen Server+Hypervisor+Plugin > > The same patter is being about to Libvirt stuff. The coverage on > the KVM hypervisor plugin already went from 4 to 10.5% after > refactoring 6 commands > > Cheers, Wilder > > On 22 Apr 2015, at 23:06, Marcus > <shadow...@gmail.com<mailto:shadow...@gmail.com>> wrote: > > Kind of a tangent, but I'd actually like to see some work done to > clean up LibvirtComputing resource. One model I've prototyped that > seems to work is to create an annotation, such as > 'KVMCommandExecutor', with a 'handles' property. With this > annotation, you implement a class that handles, e.g. StartCommand, > etc. Then in LibvirtComputingResource, the 'configure' method > fetches all of these executors via reflection and stores them in an > object. Then, instead of having all of the 'instanceof' lines in > LibvirtComputingResource, the executeRequest method fetches the > executor that handles the incoming command and runs it. > > I think this would break up LibvirtComputingResource into smaller, > more testable and manageable chunks, and force things like config > and utility methods to move to a more sane location, as well. As a > bonus, this model makes things pluggable. Someone could ship KVM > plugin code containing standalone command executors that are > discovered at runtime for things they need to run at the hypervisor > level. > > On Tue, Apr 21, 2015 at 6:27 AM, Wilder Rodrigues > <wrodrig...@schubergphilis.com<mailto:wrodrig...@schubergphilis.com>> > wrote: > > Hi all, > > Yesterday I started working on the LibvirtComputingResource class > in order to apply the same patterns I used in the > CitrixResourceBase + add more unit tests to it After 10 hours of > work I got a bit stuck with the 1st test, which would cover the > refactored LibvirtStopCommandWrapper. Why did I get stuck? The > class used a few static methods that call native libraries, which I > would like to mock. However, when writing the tests I faced > problems with the current Mockito/PowerMock we are using: they are > simply not enough for the task. > > What did I do then? I added a dependency to EasyMock and > PowerMock-EasyMock API. It worked almost fine, but I had to add a > “-noverify” to both my Eclipse Runtime configuration and also to > the cloud-plugin-hypervisor-kvm/pom.xml file. I agree that’s not > nice, but was my first attempt of getting it to work. After trying > to first full build I faced more problems related to > ClassDefNotFoundExpcetion which were complaining about Mockito > classes. I then found out that adding the PowerMockRunner to all > the tests classes was going to be a heavy burden and would also > mess up future changes (e.g. the -noverify flag was removed from > Java 8, thus adding it now would be a problem soon). > > Now that the first 2 paragraphs explain a bit about the problem, > let’s get to the solution: Java 8 > > The VerifyError that I was getting was due to the use of the latest > EasyMock release (3.3.1). I tried to downgrade it to 3.1/3.2 but it > also did not work. My decision: do not refactor if the proper tests > cannot be added. This left me with one action: migrate to Java 8. > > There were mentions about Java 8 in february[1] and now I will put > some energy in making it happen. > > What is your opinion on it? > > Thanks in advance. > > Cheers, Wilder > > http://mail-archives.apache.org/mod_mbox/cloudstack-dev/201502.mbox/%3 c54eef6be.5040...@shapeblue.com%3E<http://mail-archives.apache.org/mod_m box/cloudstack-dev/201502.mbox/<54eef6be.5040...@shapeblue.com<mailto:54 eef6be.5040...@shapeblue.com>>> > > Regards, Rohit Yadav Software Architect, ShapeBlue > > > [cid:9DD97B41-04C5-45F0-92A7-951F3E962F7A] > > > M. +91 88 262 30892 | > rohit.ya...@shapeblue.com<mailto:rohit.ya...@shapeblue.com> Blog: > bhaisaab.org<http://bhaisaab.org> | Twitter: @_bhaisaab > > > > > Find out more about ShapeBlue and our range of CloudStack related > services > > IaaS Cloud Design & > Build<http://shapeblue.com/iaas-cloud-design-and-build//> CSForge – > rapid IaaS deployment framework<http://shapeblue.com/csforge/> > CloudStack > Consulting<http://shapeblue.com/cloudstack-consultancy/> CloudStack > Software > Engineering<http://shapeblue.com/cloudstack-software-engineering/> > CloudStack Infrastructure > Support<http://shapeblue.com/cloudstack-infrastructure-support/> > CloudStack Bootcamp Training > Courses<http://shapeblue.com/cloudstack-training/> > > This email and any attachments to it may be confidential and are > intended solely for the use of the individual to whom it is > addressed. Any views or opinions expressed are solely those of the > author and do not necessarily represent those of Shape Blue Ltd or > related companies. If you are not the intended recipient of this > email, you must neither take any action based upon its contents, > nor copy or show it to anyone. Please contact the sender if you > believe you have received this email in error. Shape Blue Ltd is a > company incorporated in England & Wales. ShapeBlue Services India > LLP is a company incorporated in India and is operated under > license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is > a company incorporated in Brasil and is operated under license from > Shape Blue Ltd. ShapeBlue SA Pty Ltd is a company registered by The > Republic of South Africa and is traded under license from Shape > Blue Ltd. ShapeBlue is a registered trademark. > -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVoD1hAAoJEAGbWC3bPspCft0P/jA0VgMdGxdWGYCaJxJWEQcn u+9nXyQfrLOmejiYmjVuY0o9rX31iGGJDqEEnokebh0Mq6JVMnLzQ4TCOHx2sDPg /aKKFg6QOq3fiR38/l/8QCvYGnyEz+IxATqcaTd/821h+3EEeKpj6UJLbjmABa9F canevcCSUPrWUwpTrhMncWZs416fzR+2cYjblGlBIwWi6n1d8kIvGyaIALvOhOvl /sl0o8/635Uy7LywdgGE/Hg+EcAmyRH6W0LhNaZNjrc/ioj7hIIWja0L+HGjcCc7 nEtA1T4mJkXfMSjFYP6LcVMKcrXDwfTHEBo1rL+xtXVrtrUARg8VDdrxSkezc68X fMaFFoXmjCjGRzF7xqzK02Mn2kuK/owlqvCXwmfp5PglW4kP97m6vnq3h1cRFHxl fpANd1ZYSSWamKsHEDYM7U28aopdYI2jnQavlWlwKXNQjc+8G5ZkgbQyFQSPFlkA +DaYA0LFqx71PRbwwRo/+DxR31XB1ZjfJqtSO49e87YdiAfUowyGET7fxGLlq18I FU5MVCO9gwXVV/1x88PWmjxmmyFvPXOPb30ftStHimSeGajAms4aZPOo/aaBe55H aMzfTeNpcajJ6HAT9hETMqKkAVuFe4zHu8GG3bO8f4jN5eu4vk2ACy+E7iVoZove rILQJfo0P3fsA7MyLZPG =bcDV -----END PGP SIGNATURE-----
