open visibility of JavaClassCacheEntry

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

open visibility of JavaClassCacheEntry

Romain Manni-Bucau
Hi guys,

can you make ContextConfig.JavaClassCacheEntry public please? Idea is to be
able to override ContextConfig and potentially customize processAnnotations
methods. Currently it is a pain and it is preventing to upgrade meecrowave
and likely tomee to java11+jlink support

Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> |  Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Reply | Threaded
Open this post in threaded view
|

Re: open visibility of JavaClassCacheEntry

Romain Manni-Bucau
PS: alternative which can be saner would be to split webConfig() in a set
of small protected methods instead of a monolitic block, typically here a
processAnnotations(webXmls) not depending on internals can work

Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> |  Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>


Le jeu. 6 déc. 2018 à 11:51, Romain Manni-Bucau <[hidden email]> a
écrit :

> Hi guys,
>
> can you make ContextConfig.JavaClassCacheEntry public please? Idea is to
> be able to override ContextConfig and potentially customize
> processAnnotations methods. Currently it is a pain and it is preventing to
> upgrade meecrowave and likely tomee to java11+jlink support
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github
> <https://github.com/rmannibucau> | LinkedIn
> <https://www.linkedin.com/in/rmannibucau> | Book
> <https://www.packtpub.com/application-development/java-ee-8-high-performance>
>
Reply | Threaded
Open this post in threaded view
|

Re: open visibility of JavaClassCacheEntry

remm
In reply to this post by Romain Manni-Bucau
On Thu, Dec 6, 2018 at 11:51 AM Romain Manni-Bucau <[hidden email]>
wrote:

> Hi guys,
>
> can you make ContextConfig.JavaClassCacheEntry public please? Idea is to be
> able to override ContextConfig and potentially customize processAnnotations
> methods. Currently it is a pain and it is preventing to upgrade meecrowave
> and likely tomee to java11+jlink support
>

Ok, so I had a look at the meecrowave main class after doing my embedded
refactoring and I was a bit horrified by the amount of hacks :) Not that
there were too many options to do the things that were done (some I didn't
think were possible), and luckily my changes would have been a very good
help there.

So here, it looks like a bit scary as well, since JavaClassCacheEntry is
package private (not ok usually), but only instantiated from a private
method (what would you do about that ?). So I'd rather not do that, or not
just that, since it would be good to reduce the amount of hacks.

For starters, I don't like "private" in that kind of class (same with
"final"), I prefer "protected" usually. Shouldn't JavaClassCacheEntry be a
non static protected class ?

About webConfig, I'm not sure. A good way to do it would be to add calls to
a few intermediate empty methods in appropriate locations, rather than a
real refactoring.

Rémy
Reply | Threaded
Open this post in threaded view
|

Re: open visibility of JavaClassCacheEntry

Romain Manni-Bucau
Hello Rémy,


Le jeu. 6 déc. 2018 à 14:13, Rémy Maucherat <[hidden email]> a écrit :

> On Thu, Dec 6, 2018 at 11:51 AM Romain Manni-Bucau <[hidden email]>
> wrote:
>
> > Hi guys,
> >
> > can you make ContextConfig.JavaClassCacheEntry public please? Idea is to
> be
> > able to override ContextConfig and potentially customize
> processAnnotations
> > methods. Currently it is a pain and it is preventing to upgrade
> meecrowave
> > and likely tomee to java11+jlink support
> >
>
> Ok, so I had a look at the meecrowave main class after doing my embedded
> refactoring and I was a bit horrified by the amount of hacks :) Not that
> there were too many options to do the things that were done (some I didn't
> think were possible), and luckily my changes would have been a very good
> help there.
>
> So here, it looks like a bit scary as well, since JavaClassCacheEntry is
> package private (not ok usually), but only instantiated from a private
> method (what would you do about that ?). So I'd rather not do that, or not
> just that, since it would be good to reduce the amount of hacks.
>
> For starters, I don't like "private" in that kind of class (same with
> "final"), I prefer "protected" usually. Shouldn't JavaClassCacheEntry be a
> non static protected class ?
>
> About webConfig, I'm not sure. A good way to do it would be to add calls to
> a few intermediate empty methods in appropriate locations, rather than a
> real refactoring.
>

not empty cause here the idea is to reuse an existing scanning and not let
tomcat scan classes but yes a doScanMagic() would work
the part which would be great to reuse is the mapping between bytecode (the
annotation entry) and the tomcat model (FilterDef etc)


>
> Rémy
>
Reply | Threaded
Open this post in threaded view
|

Re: open visibility of JavaClassCacheEntry

markt
On 06/12/2018 13:19, Romain Manni-Bucau wrote:

> Hello Rémy,
>
>
> Le jeu. 6 déc. 2018 à 14:13, Rémy Maucherat <[hidden email]> a écrit :
>
>> On Thu, Dec 6, 2018 at 11:51 AM Romain Manni-Bucau <[hidden email]>
>> wrote:
>>
>>> Hi guys,
>>>
>>> can you make ContextConfig.JavaClassCacheEntry public please? Idea is to
>> be
>>> able to override ContextConfig and potentially customize
>> processAnnotations
>>> methods. Currently it is a pain and it is preventing to upgrade
>> meecrowave
>>> and likely tomee to java11+jlink support
>>>
>>
>> Ok, so I had a look at the meecrowave main class after doing my embedded
>> refactoring and I was a bit horrified by the amount of hacks :) Not that
>> there were too many options to do the things that were done (some I didn't
>> think were possible), and luckily my changes would have been a very good
>> help there.
>>
>> So here, it looks like a bit scary as well, since JavaClassCacheEntry is
>> package private (not ok usually), but only instantiated from a private
>> method (what would you do about that ?). So I'd rather not do that, or not
>> just that, since it would be good to reduce the amount of hacks.
>>
>> For starters, I don't like "private" in that kind of class (same with
>> "final"), I prefer "protected" usually. Shouldn't JavaClassCacheEntry be a
>> non static protected class ?

No objections to making it protected if that helps but I don't see why
you'd want to make it non-static.

>> About webConfig, I'm not sure. A good way to do it would be to add calls to
>> a few intermediate empty methods in appropriate locations, rather than a
>> real refactoring.
>
> not empty cause here the idea is to reuse an existing scanning and not let
> tomcat scan classes but yes a doScanMagic() would work
> the part which would be great to reuse is the mapping between bytecode (the
> annotation entry) and the tomcat model (FilterDef etc)

Why don't you suggest a patch? As a rough guideline, please aim for the
minimally invasive patch the gives you what you need.

Generally adding to the existing API is OK but changing it is not. The
aim is to minimise the chance of breakage for anyone currently using or
extending that class directly.

Mark

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: open visibility of JavaClassCacheEntry

Romain Manni-Bucau
Fair enough, done in https://bz.apache.org/bugzilla/show_bug.cgi?id=62986

Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> |  Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>


Le jeu. 6 déc. 2018 à 14:32, Mark Thomas <[hidden email]> a écrit :

> On 06/12/2018 13:19, Romain Manni-Bucau wrote:
> > Hello Rémy,
> >
> >
> > Le jeu. 6 déc. 2018 à 14:13, Rémy Maucherat <[hidden email]> a écrit :
> >
> >> On Thu, Dec 6, 2018 at 11:51 AM Romain Manni-Bucau <
> [hidden email]>
> >> wrote:
> >>
> >>> Hi guys,
> >>>
> >>> can you make ContextConfig.JavaClassCacheEntry public please? Idea is
> to
> >> be
> >>> able to override ContextConfig and potentially customize
> >> processAnnotations
> >>> methods. Currently it is a pain and it is preventing to upgrade
> >> meecrowave
> >>> and likely tomee to java11+jlink support
> >>>
> >>
> >> Ok, so I had a look at the meecrowave main class after doing my embedded
> >> refactoring and I was a bit horrified by the amount of hacks :) Not that
> >> there were too many options to do the things that were done (some I
> didn't
> >> think were possible), and luckily my changes would have been a very good
> >> help there.
> >>
> >> So here, it looks like a bit scary as well, since JavaClassCacheEntry is
> >> package private (not ok usually), but only instantiated from a private
> >> method (what would you do about that ?). So I'd rather not do that, or
> not
> >> just that, since it would be good to reduce the amount of hacks.
> >>
> >> For starters, I don't like "private" in that kind of class (same with
> >> "final"), I prefer "protected" usually. Shouldn't JavaClassCacheEntry
> be a
> >> non static protected class ?
>
> No objections to making it protected if that helps but I don't see why
> you'd want to make it non-static.
>
> >> About webConfig, I'm not sure. A good way to do it would be to add
> calls to
> >> a few intermediate empty methods in appropriate locations, rather than a
> >> real refactoring.
> >
> > not empty cause here the idea is to reuse an existing scanning and not
> let
> > tomcat scan classes but yes a doScanMagic() would work
> > the part which would be great to reuse is the mapping between bytecode
> (the
> > annotation entry) and the tomcat model (FilterDef etc)
>
> Why don't you suggest a patch? As a rough guideline, please aim for the
> minimally invasive patch the gives you what you need.
>
> Generally adding to the existing API is OK but changing it is not. The
> aim is to minimise the chance of breakage for anyone currently using or
> extending that class directly.
>
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>