cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Wido den Hollander <w...@widodh.nl>
Subject Re: [DISCUSS] Moving to Java 8
Date Fri, 10 Jul 2015 21:47:13 GMT
-----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
> <WRodrigues@schubergphilis.com<mailto:WRodrigues@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
> <shadowsor@gmail.com<mailto:shadowsor@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
> <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_res
ource
>
>  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+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
> <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/%3
C54EEF6BE.5040401@shapeblue.com%3E<http://mail-archives.apache.org/mod_m
box/cloudstack-dev/201502.mbox/<54EEF6BE.5040401@shapeblue.com<mailto:54
EEF6BE.5040401@shapeblue.com>>>
>
>  Regards, Rohit Yadav Software Architect, ShapeBlue
> 
> 
> [cid:9DD97B41-04C5-45F0-92A7-951F3E962F7A]
> 
> 
> M. +91 88 262 30892 |
> rohit.yadav@shapeblue.com<mailto:rohit.yadav@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-----

Mime
View raw message