New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better cold classes management #175

Merged
merged 9 commits into from Apr 1, 2015

Conversation

Projects
None yet
3 participants
@a-peyrard
Contributor

a-peyrard commented Mar 15, 2015

This PR permits to manage several new things about cold classes.

  • It permits to add inherited classes of cold components (components being loaded in the main factory, such as AutoStartable ones) in cold classes list.
  • It adds a way to declare cold classes using a property: restx.cold.classes. The property value is a list of fqcn separated by :.
  • It adds a way to declare cold classes using a resource file: META-INF/cold-classes.list. Resources will be loaded from the classloader, and every classes listed will be added to the cold classes list. The file must contains a list of fqcn, one per line.
  • It provides a way to fulfil the resource file using an annotation: @Cold. So every classes annotated with @Cold will be added to the resource file (thanks to a new annotation processor).

a-peyrard added some commits Feb 15, 2015

Adds method to get inherited classes
This method recursively analyse a class to get all its super classes, and
all its interfaces
adds full inheritance tree in cold classes
Instead of just adding the component class, also adds its inherited classes.
introduces restx.cold.classes property
This property contains a string with all FQCN of cold classes, separated by ':'.

This property might be specified using System.properties like that:
```
System.setProperty("restx.cold.classes", "foo.ColdClass1:bar.ColdClass2");
```
It meant that `foo.ColdClass1` and `bar.ColdClass2` must be ignored by hot-reload
mechanism.

As cold classes are supposed to be used by components, they must be loaded with the same classloader
as the one used for components, that's why the classloader used to initialize the factory's components
is kept, and re-used to transform cold classes FQCN into real classes.
introduces ColdClasses helper class
Currently only one method is defined, a method to extract cold classes
from a string. The string needs to contain fqcn separated by the ':'
character.
introduces @Cold annotation
This annotation permits to declare the class as cold. Once declared
the class will not be part of the hot-reloading mechanism.
adds an abstract class for resources generation in AP
In factory annotation processor a class was used to generate the
machine service file.
This process use from now on the abstract class ResourceDeclaration
defined in RestxAbstractProcessor.

Like this other processors having the same need, might also use the
ResourceDeclaration class.
introduces annotation processor for @Cold
This annotation processor will produces a "cold-classes.list" file,
containing the list of all classes annotated with @Cold.
permits to declare cold classes in a resource file
Using a resource `META-INF/cold-classes.list` permits to declare a list
of cold classes.
The file must contain a fqcn per line.

Mind that this file will be read only once, so new cold classes declaration will
requires a restart of the application.
@xhanin

This comment has been minimized.

Contributor

xhanin commented Mar 16, 2015

Sounds good to me. One question: is there any reason you use : as separator? I usually prefer using a comma to list values (when it's not conflicting with the values), I think it's more readable, especially if you trim the values you can have a very readable list.

@a-peyrard

This comment has been minimized.

Contributor

a-peyrard commented Mar 16, 2015

No reason, I just use to much ux systems :)

So I will change to , soon.

@a-peyrard

This comment has been minimized.

Contributor

a-peyrard commented Mar 29, 2015

Separator changed, if it's okay for you I will merge this tomorrow.

@xhanin

This comment has been minimized.

Contributor

xhanin commented Mar 30, 2015

Fine for me

@fcamblor

This comment has been minimized.

Wow this seems really weird case ! :-)

Since the ex.getMessage() seems a very weird/tricky thing, I would use an intermediate variable (with explicit naming) to describe the fact that the filename is carried by the exception message (people don't always read javadoc, it will enforce this thing in code :-))

Last note : why not Throwables.propagate(exceptions) in catch clauses ?

This comment has been minimized.

fcamblor replied Mar 31, 2015

Oh .. didn't noticed the code was already here before...

Thus, my remarks might concern @xhanin as well :)

This comment has been minimized.

xhanin replied Apr 1, 2015

Yes, this is for me. Here it's not javadoc, it's just comment, and considering the amount of comment I put in code usually people read it when there is some :) And I name the file variable, so I don't think adding an additional variable is really worth it. But if you prefer I won't object :)

@fcamblor

This comment has been minimized.

Contributor

fcamblor commented Mar 31, 2015

👍 with some delay (sorry)

change cold classes separator from ':' to ','
As a trim is done of tokens of the splitted string, property might be defined like that:
restx.cold.classes="foo.bar.MyFxirColdClass, foo.bar.MySecondColdClass,or.whithout.Spaces"
@a-peyrard

This comment has been minimized.

Contributor

a-peyrard commented Apr 1, 2015

I changed the trim, according to Xavier's remark (thanks to "push -f" :)).

a-peyrard added a commit that referenced this pull request Apr 1, 2015

@a-peyrard a-peyrard merged commit 88e1be3 into restx:master Apr 1, 2015

@fcamblor

This comment has been minimized.

Contributor

fcamblor commented Oct 23, 2015

@a-peyrard Just 1 simple question about @Cold annotation : will it work on only @Component-annotated classes or will it work under all circumstances ?

I'm facing some weird classloader issue and tried to fix it with @Cold annotation but it didn't worked (my classloader issue concerns a "standard" class, not a @Component)

@a-peyrard

This comment has been minimized.

Contributor

a-peyrard commented Oct 23, 2015

I just checked the code, and it should work for any classes, not only
@component. If you set the log level to "debug" for
RestxMainRouterFactory it should log the cold classes like this: "cold
classes: ..."

If it's not working you can try to add the property "restx.cold.classes" to
your JVM options, the value is a list of FQCN separated by commas.

On Fri, Oct 23, 2015 at 5:07 PM Frédéric Camblor notifications@github.com
wrote:

@a-peyrard https://github.com/a-peyrard Just 1 simple question about
@Cold annotation : will it work on only @Component-annotated classes or
will it work under all circumstances ?

I'm facing some weird classloader issue and tried to fix it with @Cold
annotation but it didn't worked (my classloader issue concerns a "standard"
class, not a @component)


Reply to this email directly or view it on GitHub
#175 (comment).

@fcamblor

This comment has been minimized.

Contributor

fcamblor commented Oct 23, 2015

Yep, tried with the restx.cold.classes property as well and it didn't worked for my particular case

But no worries, my problem may not be related to hot reload (I only tried to fix it with cold classes but it didn't helped)

@xhanin

This comment has been minimized.

Contributor

xhanin commented Oct 23, 2015

Fred, I think it would be interesting to diagnose your issue more closely to see if it's not a problem with the new cold classes approach. I will try to check that with you next week or more probably the week after, since I won't be at the office next week.

@fcamblor

This comment has been minimized.

Contributor

fcamblor commented Oct 24, 2015

I just created this repo reproducing my case (needs current restx version to compile well as it requires @Cold annotation)

if you run AppServer from it in dev mode, and go to http://127.0.0.1:8080/api/servletRootRealPath, you will see you have a null path returned whereas the ContainerListener has been initialized (and the App enum singleton initialized as well)

If you retry it in prod mode, everything will go well.

@a-peyrard

This comment has been minimized.

Contributor

a-peyrard commented Oct 24, 2015

I just cloned your repo, and I will try to explain what I understood.

The App.class is loaded a first time by the main classloader (being AppClassLoader), and after that the RestxMainRouterFactory initialize and create a first HotCompile classloader. Unfortunately this is this first HotCompile classloader which is used to load cold classes.
Then the request is made, and another HotReload classloader is created for the request. It finds the cold class (App), but the one loaded in the first HotReload classloader.

I manage to make it work by commenting the line:

mainFactoryClassLoader = hotReloadingClassLoader;

I don't know why I used this classloader to load the cold classes, I don't know what would be the impact, to use the original classloader instead of the one creating the main restx factory.

HTH

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment