You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Rémy Maucherat <re...@apache.org> on 2018/10/24 09:54:50 UTC
"Embedded" improvements
Hi,
I'm starting to review embedded. Maybe I won't make many changes in the end
and won't need a separate API.
However, I have another post 9.0.13 refactoring with a redoing of
ConfigFileLoader and various places which try to load the configuration
from seemingly random locations (the winner is "server-embed.xml", which
IMO everyone forgot about aeons ago). So this redoes it with a pluggable
API as ConfigFileLoader was not. Minor additional work is needed to use it
in more places, since all configuration/resource loading from conf would
need to go through it (ideally) (and except logging, since it's obviously
pluggable enough).
https://github.com/rmaucher/tomcat/commit/c386eb2fc3b2f42b3a307cbc2d0ab1a72581f56a
Comments ?
Rémy
Re: "Embedded" improvements
Posted by Rémy Maucherat <re...@apache.org>.
On Wed, Oct 24, 2018 at 2:47 PM Coty Sutherland <cs...@apache.org> wrote:
> I had a trivial comment that I put on the GitHub commit :) Otherwise that
> looks OK to me.
>
Yes, the code could be nicer, so I improved it.
Rémy
Re: "Embedded" improvements
Posted by Coty Sutherland <cs...@apache.org>.
I had a trivial comment that I put on the GitHub commit :) Otherwise that
looks OK to me.
On Wed, Oct 24, 2018 at 5:55 AM Rémy Maucherat <re...@apache.org> wrote:
> Hi,
>
> I'm starting to review embedded. Maybe I won't make many changes in the end
> and won't need a separate API.
>
> However, I have another post 9.0.13 refactoring with a redoing of
> ConfigFileLoader and various places which try to load the configuration
> from seemingly random locations (the winner is "server-embed.xml", which
> IMO everyone forgot about aeons ago). So this redoes it with a pluggable
> API as ConfigFileLoader was not. Minor additional work is needed to use it
> in more places, since all configuration/resource loading from conf would
> need to go through it (ideally) (and except logging, since it's obviously
> pluggable enough).
>
>
> https://github.com/rmaucher/tomcat/commit/c386eb2fc3b2f42b3a307cbc2d0ab1a72581f56a
>
> Comments ?
>
> Rémy
>
Re: "Embedded" improvements
Posted by Mark Thomas <ma...@apache.org>.
On 29/10/2018 18:01, Rémy Maucherat wrote:
> On Mon, Oct 29, 2018 at 6:46 PM Mark Thomas <ma...@apache.org> wrote:
>
>> On 29/10/2018 10:52, Rémy Maucherat wrote:
>>> On Wed, Oct 24, 2018 at 11:54 AM Rémy Maucherat <re...@apache.org> wrote:
>>>
>>>> Hi,
>>>>
>>>> I'm starting to review embedded. Maybe I won't make many changes in the
>>>> end and won't need a separate API.
>>>>
>>>> However, I have another post 9.0.13 refactoring with a redoing of
>>>> ConfigFileLoader and various places which try to load the configuration
>>>> from seemingly random locations (the winner is "server-embed.xml", which
>>>> IMO everyone forgot about aeons ago). So this redoes it with a pluggable
>>>> API as ConfigFileLoader was not. Minor additional work is needed to use
>> it
>>>> in more places, since all configuration/resource loading from conf would
>>>> need to go through it (ideally) (and except logging, since it's
>> obviously
>>>> pluggable enough).
>>>>
>>>>
>>>>
>> https://github.com/rmaucher/tomcat/commit/c386eb2fc3b2f42b3a307cbc2d0ab1a72581f56a
>>>>
>>>> Comments ?
>>>>
>>>
>>> I think I'm done (?) with the review. Even if not super modern, the
>>> "Tomcat" class provides useful methods and ways to deploy your app
>> embedded.
>>> However, the process of configuring a Tomcat with code (and maintaining
>> it)
>>> is way way too involving (even for "basic" needs), and then degrades into
>>> (partially) reinventing the code config files like the obvious
>> server.xml.
>>> So instead this new API allows providing that server.xml (and most
>> others)
>>> to the Tomcat class and then use its methods to do what you need to fill
>>> the blanks and manage the lifecycle of the embedded server.
>>>
>>> Not covered by the pluggability:
>>> - jaspic uses load/store a bit too much, it would still need its regular
>>> config file
>>> - storeconfig
>>> - other items that need files (OpenSSL, cloud)
>>> - anything else I missed
>>>
>>> Still at https://github.com/rmaucher/tomcat/commits/trunk
>>
>> The commits at the moment appear to revert a number of unrelated changes
>> (o.a.t.jni.[Library|SSL] are the ones that jumped out at me). Worth a
>> careful check when applying these changes.
>>
>
> Yes, it's fixed in the next commit, sorry, so there's no actual change
> there. I'll use svn to commit anyway so it should be ok.
Ah. I missed that. Sorry for the noise.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: "Embedded" improvements
Posted by Rémy Maucherat <re...@apache.org>.
On Mon, Oct 29, 2018 at 6:46 PM Mark Thomas <ma...@apache.org> wrote:
> On 29/10/2018 10:52, Rémy Maucherat wrote:
> > On Wed, Oct 24, 2018 at 11:54 AM Rémy Maucherat <re...@apache.org> wrote:
> >
> >> Hi,
> >>
> >> I'm starting to review embedded. Maybe I won't make many changes in the
> >> end and won't need a separate API.
> >>
> >> However, I have another post 9.0.13 refactoring with a redoing of
> >> ConfigFileLoader and various places which try to load the configuration
> >> from seemingly random locations (the winner is "server-embed.xml", which
> >> IMO everyone forgot about aeons ago). So this redoes it with a pluggable
> >> API as ConfigFileLoader was not. Minor additional work is needed to use
> it
> >> in more places, since all configuration/resource loading from conf would
> >> need to go through it (ideally) (and except logging, since it's
> obviously
> >> pluggable enough).
> >>
> >>
> >>
> https://github.com/rmaucher/tomcat/commit/c386eb2fc3b2f42b3a307cbc2d0ab1a72581f56a
> >>
> >> Comments ?
> >>
> >
> > I think I'm done (?) with the review. Even if not super modern, the
> > "Tomcat" class provides useful methods and ways to deploy your app
> embedded.
> > However, the process of configuring a Tomcat with code (and maintaining
> it)
> > is way way too involving (even for "basic" needs), and then degrades into
> > (partially) reinventing the code config files like the obvious
> server.xml.
> > So instead this new API allows providing that server.xml (and most
> others)
> > to the Tomcat class and then use its methods to do what you need to fill
> > the blanks and manage the lifecycle of the embedded server.
> >
> > Not covered by the pluggability:
> > - jaspic uses load/store a bit too much, it would still need its regular
> > config file
> > - storeconfig
> > - other items that need files (OpenSSL, cloud)
> > - anything else I missed
> >
> > Still at https://github.com/rmaucher/tomcat/commits/trunk
>
> The commits at the moment appear to revert a number of unrelated changes
> (o.a.t.jni.[Library|SSL] are the ones that jumped out at me). Worth a
> careful check when applying these changes.
>
Yes, it's fixed in the next commit, sorry, so there's no actual change
there. I'll use svn to commit anyway so it should be ok.
Rémy
>
> Overall, looks good.
>
> > And the plan is still to add it post 9.0.13.
>
> +1
>
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: "Embedded" improvements
Posted by Mark Thomas <ma...@apache.org>.
On 29/10/2018 10:52, Rémy Maucherat wrote:
> On Wed, Oct 24, 2018 at 11:54 AM Rémy Maucherat <re...@apache.org> wrote:
>
>> Hi,
>>
>> I'm starting to review embedded. Maybe I won't make many changes in the
>> end and won't need a separate API.
>>
>> However, I have another post 9.0.13 refactoring with a redoing of
>> ConfigFileLoader and various places which try to load the configuration
>> from seemingly random locations (the winner is "server-embed.xml", which
>> IMO everyone forgot about aeons ago). So this redoes it with a pluggable
>> API as ConfigFileLoader was not. Minor additional work is needed to use it
>> in more places, since all configuration/resource loading from conf would
>> need to go through it (ideally) (and except logging, since it's obviously
>> pluggable enough).
>>
>>
>> https://github.com/rmaucher/tomcat/commit/c386eb2fc3b2f42b3a307cbc2d0ab1a72581f56a
>>
>> Comments ?
>>
>
> I think I'm done (?) with the review. Even if not super modern, the
> "Tomcat" class provides useful methods and ways to deploy your app embedded.
> However, the process of configuring a Tomcat with code (and maintaining it)
> is way way too involving (even for "basic" needs), and then degrades into
> (partially) reinventing the code config files like the obvious server.xml.
> So instead this new API allows providing that server.xml (and most others)
> to the Tomcat class and then use its methods to do what you need to fill
> the blanks and manage the lifecycle of the embedded server.
>
> Not covered by the pluggability:
> - jaspic uses load/store a bit too much, it would still need its regular
> config file
> - storeconfig
> - other items that need files (OpenSSL, cloud)
> - anything else I missed
>
> Still at https://github.com/rmaucher/tomcat/commits/trunk
The commits at the moment appear to revert a number of unrelated changes
(o.a.t.jni.[Library|SSL] are the ones that jumped out at me). Worth a
careful check when applying these changes.
Overall, looks good.
> And the plan is still to add it post 9.0.13.
+1
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: "Embedded" improvements
Posted by Rémy Maucherat <re...@apache.org>.
On Wed, Oct 24, 2018 at 11:54 AM Rémy Maucherat <re...@apache.org> wrote:
> Hi,
>
> I'm starting to review embedded. Maybe I won't make many changes in the
> end and won't need a separate API.
>
> However, I have another post 9.0.13 refactoring with a redoing of
> ConfigFileLoader and various places which try to load the configuration
> from seemingly random locations (the winner is "server-embed.xml", which
> IMO everyone forgot about aeons ago). So this redoes it with a pluggable
> API as ConfigFileLoader was not. Minor additional work is needed to use it
> in more places, since all configuration/resource loading from conf would
> need to go through it (ideally) (and except logging, since it's obviously
> pluggable enough).
>
>
> https://github.com/rmaucher/tomcat/commit/c386eb2fc3b2f42b3a307cbc2d0ab1a72581f56a
>
> Comments ?
>
I think I'm done (?) with the review. Even if not super modern, the
"Tomcat" class provides useful methods and ways to deploy your app embedded.
However, the process of configuring a Tomcat with code (and maintaining it)
is way way too involving (even for "basic" needs), and then degrades into
(partially) reinventing the code config files like the obvious server.xml.
So instead this new API allows providing that server.xml (and most others)
to the Tomcat class and then use its methods to do what you need to fill
the blanks and manage the lifecycle of the embedded server.
Not covered by the pluggability:
- jaspic uses load/store a bit too much, it would still need its regular
config file
- storeconfig
- other items that need files (OpenSSL, cloud)
- anything else I missed
Still at https://github.com/rmaucher/tomcat/commits/trunk
And the plan is still to add it post 9.0.13.
Rémy
Re: "Embedded" improvements
Posted by Igal Sapir <ig...@lucee.org>.
On Wed, Oct 24, 2018 at 2:55 AM Rémy Maucherat <re...@apache.org> wrote:
> Hi,
>
> I'm starting to review embedded. Maybe I won't make many changes in the end
> and won't need a separate API.
>
> However, I have another post 9.0.13 refactoring with a redoing of
> ConfigFileLoader and various places which try to load the configuration
> from seemingly random locations (the winner is "server-embed.xml", which
> IMO everyone forgot about aeons ago). So this redoes it with a pluggable
> API as ConfigFileLoader was not. Minor additional work is needed to use it
> in more places, since all configuration/resource loading from conf would
> need to go through it (ideally) (and except logging, since it's obviously
> pluggable enough).
>
>
> https://github.com/rmaucher/tomcat/commit/c386eb2fc3b2f42b3a307cbc2d0ab1a72581f56a
>
> Comments ?
>
I like "Embedded", and I like "Pluggable", so +1 from me.
The code looks good to me, and even more so after addressing Coty's comment.
Igal
>
> Rémy
>