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> 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> 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> 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> 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> 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_resource
>>
>> Cheers,
>> Wilder
>>
>> On 23 Apr 2015, at 17:26, Marcus <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> 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+XenServer+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> 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> 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/%3c54eef6be.5040...@shapeblue.com%3E
>> <http://mail-archives.apache.org/mod_mbox/cloudstack-dev/201502.mbox/<
>> 54eef6be.5040...@shapeblue.com>>
>>
>>
>>
>>
>>
>
