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>>
>
>
>
