cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Wilder Rodrigues <WRodrig...@schubergphilis.com>
Subject Re: [DISCUSS] Moving to Java 8
Date Fri, 01 May 2015 10:37:22 GMT
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 <shadowsor@gmail.com<mailto:shadowsor@gmail.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 <shadowsor@gmail.com<mailto:shadowsor@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 <WRodrigues@schubergphilis.com<mailto:WRodrigues@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 <shadowsor@gmail.com<mailto:shadowsor@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 <shadowsor@gmail.com<mailto:shadowsor@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
<WRodrigues@schubergphilis.com<mailto:WRodrigues@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 <shadowsor@gmail.com<mailto:shadowsor@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
<WRodrigues@schubergphilis.com<mailto:WRodrigues@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 <shadowsor@gmail.com<mailto:shadowsor@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
<WRodrigues@schubergphilis.com<mailto:WRodrigues@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.5040401@shapeblue.com%3E<http://mail-archives.apache.org/mod_mbox/cloudstack-dev/201502.mbox/<54EEF6BE.5040401@shapeblue.com<mailto:54EEF6BE.5040401@shapeblue.com>>>








Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message