You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@roller.apache.org by Dave <sn...@gmail.com> on 2007/06/13 15:57:41 UTC
Roller Easy Install - ready to commit
I have already committed part of this work (e.g. DatabaseProvider and
MailProvider classes) and now I'm ready to commit the rest.
I updated the proposal with latest design, status notes and screenshots:
http://cwiki.apache.org/confluence/display/ROLLER/Proposal+Easy+Install
In particular, take a look at the section titled "Design for Option
#1: Property file based configuration" and anything marked READY TO
COMMIT.
I'm not entirely happy with the changes in PersistenceSessionFilter
but I'd like to get other's opinions on the code so I'm ready to
commit. I believe this is safe because the auto-install logic off by
default.
If there are no objections, I will commit the code later today.
- Dave
Re: Roller Easy Install - ready to commit
Posted by Anil Gangolli <an...@busybuddha.org>.
By the way, I really like the new error diagnosis messages on the database
connection setup. This should really help users alot. We might need to add
some other things to check (firewall port open? MySQL set up to listen for
TCP connections?) to check in the Connection Refused case.
The term "connectionType" in the properties is a bit misleading since in the
JNDI case, it is just the means of finding the JDBC DataSource. I hope this
won't confuse people.
--a.
----- Original Message -----
From: "Anil Gangolli" <an...@busybuddha.org>
To: <de...@roller.apache.org>
Cc: <Ma...@Sun.COM>
Sent: Wednesday, June 13, 2007 7:55 AM
Subject: Re: Roller Easy Install - ready to commit
>
> Will we ask people to disable "auto-install" after an upgrade or
> installation to avoid performance issues? Is this something that will be
> done by the upgrade/installation process itself?
>
> --a.
>
>
> ----- Original Message -----
> From: "Dave" <sn...@gmail.com>
> To: <de...@roller.apache.org>
> Cc: <Ma...@Sun.COM>
> Sent: Wednesday, June 13, 2007 6:57 AM
> Subject: Roller Easy Install - ready to commit
>
>
>>I have already committed part of this work (e.g. DatabaseProvider and
>> MailProvider classes) and now I'm ready to commit the rest.
>>
>> I updated the proposal with latest design, status notes and screenshots:
>> http://cwiki.apache.org/confluence/display/ROLLER/Proposal+Easy+Install
>>
>> In particular, take a look at the section titled "Design for Option
>> #1: Property file based configuration" and anything marked READY TO
>> COMMIT.
>>
>> I'm not entirely happy with the changes in PersistenceSessionFilter
>> but I'd like to get other's opinions on the code so I'm ready to
>> commit. I believe this is safe because the auto-install logic off by
>> default.
>>
>> If there are no objections, I will commit the code later today.
>>
>> - Dave
>>
>
Re: Roller Easy Install - ready to commit
Posted by Dave <sn...@gmail.com>.
On 6/13/07, Anil Gangolli <an...@busybuddha.org> wrote:
> Will we ask people to disable "auto-install" after an upgrade or
> installation to avoid performance issues?
Yes. Folks should turn it off after install/upgrade.
> Is this something that will be done by the upgrade/installation process itself?
I guess we could, but I'd rather not have to modify the user's
roller-custom.properties file.
- Dave
Re: Roller Easy Install - ready to commit
Posted by Anil Gangolli <an...@busybuddha.org>.
Will we ask people to disable "auto-install" after an upgrade or
installation to avoid performance issues? Is this something that will be
done by the upgrade/installation process itself?
--a.
----- Original Message -----
From: "Dave" <sn...@gmail.com>
To: <de...@roller.apache.org>
Cc: <Ma...@Sun.COM>
Sent: Wednesday, June 13, 2007 6:57 AM
Subject: Roller Easy Install - ready to commit
>I have already committed part of this work (e.g. DatabaseProvider and
> MailProvider classes) and now I'm ready to commit the rest.
>
> I updated the proposal with latest design, status notes and screenshots:
> http://cwiki.apache.org/confluence/display/ROLLER/Proposal+Easy+Install
>
> In particular, take a look at the section titled "Design for Option
> #1: Property file based configuration" and anything marked READY TO
> COMMIT.
>
> I'm not entirely happy with the changes in PersistenceSessionFilter
> but I'd like to get other's opinions on the code so I'm ready to
> commit. I believe this is safe because the auto-install logic off by
> default.
>
> If there are no objections, I will commit the code later today.
>
> - Dave
>
Re: Roller Easy Install - ready to commit
Posted by Allen Gilliland <al...@sun.com>.
Dave wrote:
> On 6/13/07, Allen Gilliland <al...@sun.com> wrote:
>> I'm concerned about a couple things ...
>>
>> 1. You say you're not happy with the changes in PersistenceSessionFilter
>> which sounds ominous and to be honest I'm not even sure why you would
>> want to put any code in there. I much prefer the idea of putting the
>> code which checks if the application has been properly bootstrapped into
>> it's own filter. Call it the the BootstrapCheckFilter if you want. In
>> any case, i don't consider that logic to have anything to do with the
>> persistence session filter.
>
> I put it in PersistenceSessionFilter because it was easy and related,
> but I agree putting it in it's own filter is better. What I'm not so
> happy about is the way I'm forwarding to the Struts actions.
how are you forwarding to the struts actions? do you mean code wise or
just logically?
>
>
>> Also, in my mind this filter should be very simple, such as ...
>> if(RollerFactory.isBootstrapped()) {
>> chain.doFilter(req, res);
>> } else {
>> // app not bootstrapped yet, prompt for user involvement
>> response.sendRedirect(install/upgrade url);
>> }
>
> The logic is simple, but it is not as you describe. Currently, it is this:
> * If DB connection fails forward to DatabaseError action
> * Else if Roller tables not found direct to CreateDatabase action
> * Else if Roller tables need upgrade direct to UpgradeDatabase action
>
> I like your idea of having one central install action instead of
> routing logic in the filter.
Right, my thinking is that it would just be easier and nicer for users
if this all happens in one connected process.
>
>
>> 2. I don't know what you actually put in your SQLScriptRunner,
>> DatabaseCreator, and DatabaseUpgrader classes but I would have thought
>> this code could be part of the DatabaseProvider. It would be nice to
>> have more details on how these classes work.
>
> Then I should commit them.
>
>
>> 3. Why would we need users to restart or redeploy after doing the db
>> table work? In my mind that kind of defeats the purpose of making the
>> installation easy and really there should be no need for this.
>
> I wouldn't say it defeats the purpose, but yes, it is an additional
> step that should not be necessary if we have proper bootstrapping in
> place.
I definitely think we should find a way to make this work without
forcing a restart.
>
>
>> 4. I'm not sure exactly how your page flow is working. You have 3 new
>> actions defined but I don't understand how users get to each of them and
>> what happens after they take the action from each one.
>
> There really is no page flow, if you don't have tables you get
> directed to the CreateDatabase page, press the button, see results and
> that's it.
I think that grouping these things together makes more sense.
Basically the way the setup page works. It lists the things that need
to be done, provides indication of which of them has been done, and
gives you a way to take the next step.
This brings up another question, which is why wouldn't this be part of
the setup process? I mean, our current setup page is effectively part
of properly installing the app, the only difference is that the setup
stuff happens after bootstrapping, but the workflow should be integrated.
>
>
>> My expectation is that there would be more of a unified experience such
>> that if the application has not been bootstrapped yet then the bootstrap
>> checking filter forwards all requests to a url such as
>> /roller-ui/bootstrap.rol to handle the bootstrapping process. From that
>> url you can walk through all parts of the process as needed i.e.
>>
>> 1. check that db connection works
>> 2. do db table install/upgrade work
>> 3. check that mail session works
>> 4. everything is set, try bootstrapping application again
>> And these steps would basically be looped until the bootstrapping
>> process actually works. what i see so far doesn't seem to be as much of
>> a unified experience, but maybe i'm not understanding the flow?
>
> I like that, but I have not gone that far yet, in part because I've
> been separating installation and boostrapping in my mind and in my
> workspaces. One reason I'd like to commit is so I can merge changes
> into my roller_guice branch to experiment with DI based boostrapping
> options.
I agree that they are separate, but in my mind I just consider
installation/upgrading as a required precursor to bootstrapping, so it
makes sense that the logic is if(!bootstrapped) { send user to
install/upgrade action }
-- Allen
>
> I think I have a workable solution that is good enough to commit now
> for more feedback and improvements. Though I'd like to move the code
> from PersistenceSessionFilter first.
>
>
>> I do think that all the pages that you are showing there look great
>> though and I definitely think they will be a huge improvement over our
>> current installation process.
>
> Yep. The database error pages alone should reduce install problem list
> traffic significantly.
>
> - Dave
Re: Roller Easy Install - ready to commit
Posted by Allen Gilliland <al...@sun.com>.
Denis Balazuc wrote:
> How about having a separate "auto-install" Struts2 module that will do
> all that and adding a servlet filter as the boostrap to detect when the
> auto-install module should be triggered?
> You can then separate the Actions responsible for this, as well as avoid
> coupling in the current business layer and the roller webapp to address
> this specific feature.
That is effectively what I am doing. There is a struts2 module for the
install actions and it's only ever used when the BootstrapFilter
recognizes that the install type is 'auto' and the that the application
hasn't been bootstrapped yet.
>
> The fact that it is a feature that can be enabled/disabled makes me
> think that it should not be an integral part of the Roller monolith. I
> am not confortable with the idea of mixing this with the rest of the
> Roller business layer (adding methods to either static classes or
> interfaces just because the feature requires it - it sounds wrong to me).
> It would probably be good if the auto-installer had a business interface
> on its own in order to decouple things a bit more, which would allow
> people to implement a version of that feature, or modify its behaviour
> without necessarly be tied to a webapp or the rest of the Roller code.
> Also, having a business interface will help make a modular component out
> of it, which you can just plug in. And you guessed it, this opens the
> door to injecting various implementations.
I am not sure what you mean when you say "mixing this". What I layed
out below is effectively just proper application lifecycle management.
We are better defining the various states of the application and
providing code to better analyze what happens in each state and make
sure that the state transitions happen appropriately.
In the past the app did nothing to try and detect that the db connection
was available and working properly before trying to bootstrap, which
makes no sense because why startup Hibernate or JPA when you can't even
connect to the db? i.e. calling RollerFactory.getRoller() has no value
when the Roller instance hasn't been properly bootstrapped, so we should
handle that better, and that's basically what we are working on.
The actual auto-install feature is just a small slice of the overall
work that we are doing here. It represents only a single possible path
in the app startup sequence and it won't intrude on the code in any way.
-- Allen
>
> My 2 cents..
> Denis Balazuc
>
>
> Dave wrote:
>> On 6/19/07, Allen Gilliland <al...@sun.com> wrote:
>>> So, I found a couple things that weren't quite working right for me
>>> which I plan to modify a little bit, but as I thought about this more
>>> I'd like to suggest that we consider these 5 phases of the app lifecycle
>>> (at least these 5 for now) ...
>>>
>>> 1. preparation - app being prepared to be bootstrapped (install/ugprade
>>> stuff happens here)
>>> 2. bootstrap - wiring up the backend so the app can function
>>> 3. initialization - initializing services, such as starting background
>>> tasks, etc
>>> 4. app running - pretty self explanatory
>>> 5. shutdown - again, pretty simple
>>>
>>> Breaking that down into actual code I see things working something like
>>> this ...
>>>
>>> 1. preparation - RollerStartup (or something) controls this phase. key
>>> requirement is that we can't rely on the app being bootstrapped here.
>>> most work will be just with raw sql and services outside of the
>>> Weblogger business tier such as the database provider and config.
>>>
>>> 2. bootstrap - RollerFactory controls this phase. key requirement is
>>> that preparation is considered to have been done already.
>>>
>>> 3. initialization - the Roller instance should control this. key
>>> requirement is that bootstrapping has already been done.
>>>
>>> 4. app running - work is done by Roller instance. technically only
>>> bootstrapping is required for this, not initialization.
>>>
>>> 5. shutdown - Roller instance controls this. bootstrapping has to have
>>> happened for this to work.
>>>
>>> So, to accomplish this I want to make a couple simple tweaks to what we
>>> have now ...
>>>
>>> 1. create the RollerStartup class so that preparation work can be
>>> managed there. this is the only piece we are really missing right now
>>> and we have all of Dave's new EZ install code, it just needs to be
>>> managed through this class.
>>>
>>> 2. tidy up RollerFactory a little bit to make bootstrapping process
>>> simple and clear. i already have this in my workspace.
>>>
>>> 3. setup initialization process through Roller instance. this is really
>>> just the complement to the Roller.shutdown() method, so I plan to add a
>>> Roller.init() method and add that method to our existing manager methods
>>> so that this happens in the most logical fashion. again, this is just a
>>> small amount of code shuffling, not much new code.
>>>
>>> That's pretty much it. The only other thing is some restructuring of
>>> the RollerContext which is the normal place where we run through these
>>> phases.
>>>
>>> sound good?
>>
>> +1 all sounds good. - Dave
>>
Re: Roller Easy Install - ready to commit
Posted by Denis Balazuc <de...@balazuc.net>.
How about having a separate "auto-install" Struts2 module that will do
all that and adding a servlet filter as the boostrap to detect when the
auto-install module should be triggered?
You can then separate the Actions responsible for this, as well as avoid
coupling in the current business layer and the roller webapp to address
this specific feature.
The fact that it is a feature that can be enabled/disabled makes me
think that it should not be an integral part of the Roller monolith. I
am not confortable with the idea of mixing this with the rest of the
Roller business layer (adding methods to either static classes or
interfaces just because the feature requires it - it sounds wrong to me).
It would probably be good if the auto-installer had a business interface
on its own in order to decouple things a bit more, which would allow
people to implement a version of that feature, or modify its behaviour
without necessarly be tied to a webapp or the rest of the Roller code.
Also, having a business interface will help make a modular component out
of it, which you can just plug in. And you guessed it, this opens the
door to injecting various implementations.
My 2 cents..
Denis Balazuc
Dave wrote:
> On 6/19/07, Allen Gilliland <al...@sun.com> wrote:
>> So, I found a couple things that weren't quite working right for me
>> which I plan to modify a little bit, but as I thought about this more
>> I'd like to suggest that we consider these 5 phases of the app lifecycle
>> (at least these 5 for now) ...
>>
>> 1. preparation - app being prepared to be bootstrapped (install/ugprade
>> stuff happens here)
>> 2. bootstrap - wiring up the backend so the app can function
>> 3. initialization - initializing services, such as starting background
>> tasks, etc
>> 4. app running - pretty self explanatory
>> 5. shutdown - again, pretty simple
>>
>> Breaking that down into actual code I see things working something like
>> this ...
>>
>> 1. preparation - RollerStartup (or something) controls this phase. key
>> requirement is that we can't rely on the app being bootstrapped here.
>> most work will be just with raw sql and services outside of the
>> Weblogger business tier such as the database provider and config.
>>
>> 2. bootstrap - RollerFactory controls this phase. key requirement is
>> that preparation is considered to have been done already.
>>
>> 3. initialization - the Roller instance should control this. key
>> requirement is that bootstrapping has already been done.
>>
>> 4. app running - work is done by Roller instance. technically only
>> bootstrapping is required for this, not initialization.
>>
>> 5. shutdown - Roller instance controls this. bootstrapping has to have
>> happened for this to work.
>>
>> So, to accomplish this I want to make a couple simple tweaks to what we
>> have now ...
>>
>> 1. create the RollerStartup class so that preparation work can be
>> managed there. this is the only piece we are really missing right now
>> and we have all of Dave's new EZ install code, it just needs to be
>> managed through this class.
>>
>> 2. tidy up RollerFactory a little bit to make bootstrapping process
>> simple and clear. i already have this in my workspace.
>>
>> 3. setup initialization process through Roller instance. this is really
>> just the complement to the Roller.shutdown() method, so I plan to add a
>> Roller.init() method and add that method to our existing manager methods
>> so that this happens in the most logical fashion. again, this is just a
>> small amount of code shuffling, not much new code.
>>
>> That's pretty much it. The only other thing is some restructuring of
>> the RollerContext which is the normal place where we run through these
>> phases.
>>
>> sound good?
>
> +1 all sounds good. - Dave
>
Re: Roller Easy Install - ready to commit
Posted by Dave <sn...@gmail.com>.
On 6/19/07, Allen Gilliland <al...@sun.com> wrote:
> So, I found a couple things that weren't quite working right for me
> which I plan to modify a little bit, but as I thought about this more
> I'd like to suggest that we consider these 5 phases of the app lifecycle
> (at least these 5 for now) ...
>
> 1. preparation - app being prepared to be bootstrapped (install/ugprade
> stuff happens here)
> 2. bootstrap - wiring up the backend so the app can function
> 3. initialization - initializing services, such as starting background
> tasks, etc
> 4. app running - pretty self explanatory
> 5. shutdown - again, pretty simple
>
> Breaking that down into actual code I see things working something like
> this ...
>
> 1. preparation - RollerStartup (or something) controls this phase. key
> requirement is that we can't rely on the app being bootstrapped here.
> most work will be just with raw sql and services outside of the
> Weblogger business tier such as the database provider and config.
>
> 2. bootstrap - RollerFactory controls this phase. key requirement is
> that preparation is considered to have been done already.
>
> 3. initialization - the Roller instance should control this. key
> requirement is that bootstrapping has already been done.
>
> 4. app running - work is done by Roller instance. technically only
> bootstrapping is required for this, not initialization.
>
> 5. shutdown - Roller instance controls this. bootstrapping has to have
> happened for this to work.
>
> So, to accomplish this I want to make a couple simple tweaks to what we
> have now ...
>
> 1. create the RollerStartup class so that preparation work can be
> managed there. this is the only piece we are really missing right now
> and we have all of Dave's new EZ install code, it just needs to be
> managed through this class.
>
> 2. tidy up RollerFactory a little bit to make bootstrapping process
> simple and clear. i already have this in my workspace.
>
> 3. setup initialization process through Roller instance. this is really
> just the complement to the Roller.shutdown() method, so I plan to add a
> Roller.init() method and add that method to our existing manager methods
> so that this happens in the most logical fashion. again, this is just a
> small amount of code shuffling, not much new code.
>
> That's pretty much it. The only other thing is some restructuring of
> the RollerContext which is the normal place where we run through these
> phases.
>
> sound good?
+1 all sounds good. - Dave
Re: Roller Easy Install - ready to commit
Posted by Allen Gilliland <al...@sun.com>.
So, I found a couple things that weren't quite working right for me
which I plan to modify a little bit, but as I thought about this more
I'd like to suggest that we consider these 5 phases of the app lifecycle
(at least these 5 for now) ...
1. preparation - app being prepared to be bootstrapped (install/ugprade
stuff happens here)
2. bootstrap - wiring up the backend so the app can function
3. initialization - initializing services, such as starting background
tasks, etc
4. app running - pretty self explanatory
5. shutdown - again, pretty simple
Breaking that down into actual code I see things working something like
this ...
1. preparation - RollerStartup (or something) controls this phase. key
requirement is that we can't rely on the app being bootstrapped here.
most work will be just with raw sql and services outside of the
Weblogger business tier such as the database provider and config.
2. bootstrap - RollerFactory controls this phase. key requirement is
that preparation is considered to have been done already.
3. initialization - the Roller instance should control this. key
requirement is that bootstrapping has already been done.
4. app running - work is done by Roller instance. technically only
bootstrapping is required for this, not initialization.
5. shutdown - Roller instance controls this. bootstrapping has to have
happened for this to work.
So, to accomplish this I want to make a couple simple tweaks to what we
have now ...
1. create the RollerStartup class so that preparation work can be
managed there. this is the only piece we are really missing right now
and we have all of Dave's new EZ install code, it just needs to be
managed through this class.
2. tidy up RollerFactory a little bit to make bootstrapping process
simple and clear. i already have this in my workspace.
3. setup initialization process through Roller instance. this is really
just the complement to the Roller.shutdown() method, so I plan to add a
Roller.init() method and add that method to our existing manager methods
so that this happens in the most logical fashion. again, this is just a
small amount of code shuffling, not much new code.
That's pretty much it. The only other thing is some restructuring of
the RollerContext which is the normal place where we run through these
phases.
sound good?
-- Allen
Dave wrote:
> Comments below...
>
>
> On 6/19/07, Allen Gilliland <al...@sun.com> wrote:
>> One thing I am a little unsure of from looking at the code is what the
>> expected behavior is meant to be for the 'manual' vs. 'auto' install
>> types. This is what I would expect, let me know if this matches what
>> you are thinking ...
>>
>> if( install == 'auto' ) {
>>
>> if( install/upgrade required ) {
>> // skip bootstrapping/initialization and wait for user involvement
>> } else {
>> // app is up to date, so startup as usual
>> }
>>
>> } else {
>> // startup app as usual
>> }
>
> Yes, that is correct.
>
> The "manual" mode is supposed to work exactly as Roller did before.
> For example, in manual mode the database upgrade code does not run the
> database upgrade scripts.
>
>
>> So a couple things that I am expecting in particular are that if the
>> install type is set to 'manual' then we don't do any special
>> install/upgrade logic. Also, if the install type is 'auto' and we
>> recognize that there is work still needed, then we are requiring user
>> involvement, namely for the user to actually click a button indicating
>> that they want to do the install/upgrade.
>>
>> Assuming that's correct then I have a few things to commit around this.
>
> I'm working on the Guice branch today so you won't be stepping on my
> toes if you want to make improvements/fixes.
>
> - Dave
Re: Roller Easy Install - ready to commit
Posted by Dave <sn...@gmail.com>.
Comments below...
On 6/19/07, Allen Gilliland <al...@sun.com> wrote:
> One thing I am a little unsure of from looking at the code is what the
> expected behavior is meant to be for the 'manual' vs. 'auto' install
> types. This is what I would expect, let me know if this matches what
> you are thinking ...
>
> if( install == 'auto' ) {
>
> if( install/upgrade required ) {
> // skip bootstrapping/initialization and wait for user involvement
> } else {
> // app is up to date, so startup as usual
> }
>
> } else {
> // startup app as usual
> }
Yes, that is correct.
The "manual" mode is supposed to work exactly as Roller did before.
For example, in manual mode the database upgrade code does not run the
database upgrade scripts.
> So a couple things that I am expecting in particular are that if the
> install type is set to 'manual' then we don't do any special
> install/upgrade logic. Also, if the install type is 'auto' and we
> recognize that there is work still needed, then we are requiring user
> involvement, namely for the user to actually click a button indicating
> that they want to do the install/upgrade.
>
> Assuming that's correct then I have a few things to commit around this.
I'm working on the Guice branch today so you won't be stepping on my
toes if you want to make improvements/fixes.
- Dave
Re: Roller Easy Install - ready to commit
Posted by Allen Gilliland <al...@sun.com>.
Dave,
One thing I am a little unsure of from looking at the code is what the
expected behavior is meant to be for the 'manual' vs. 'auto' install
types. This is what I would expect, let me know if this matches what
you are thinking ...
if( install == 'auto' ) {
if( install/upgrade required ) {
// skip bootstrapping/initialization and wait for user involvement
} else {
// app is up to date, so startup as usual
}
} else {
// startup app as usual
}
So a couple things that I am expecting in particular are that if the
install type is set to 'manual' then we don't do any special
install/upgrade logic. Also, if the install type is 'auto' and we
recognize that there is work still needed, then we are requiring user
involvement, namely for the user to actually click a button indicating
that they want to do the install/upgrade.
Assuming that's correct then I have a few things to commit around this.
-- Allen
Dave wrote:
> On 6/18/07, Allen Gilliland <al...@sun.com> wrote:
>> Some comments after looking at the latest code on this ...
>>
>> 1. I'm not really liking that the RollerContext object has become a
>> DatabaseScriptProvider. I think that functionality should be separated
>> out into its own class or something. I think we were guilty of putting
>> way too much logic in the RollerContext before and we should try and
>> avoid that.
>>
>> 2. I still think the BootstrapFilter should be simplified even further.
>> We want to do as little work there as possible.
>>
>> 3. Your struts2 actions are ApplicationAware and are pulling things out
>> of the servlet context, to me that's a very confusing way of managing
>> access to that database script provider object. Why does that class
>> need to be a singleton anyways? Why wouldn't the struts2 action just
>> instantiate that class when it's actually going to use it?
>>
>> 4. Unless you have a good reason, I'd really like to keep all the
>> struts2 actions extending the UIAction class rather than directly using
>> ActionSupport.
>>
>> Overall I'm still feeling like this code is lacking an overall
>> cohesiveness and the code doesn't really match the workflow of doing an
>> installation and bootstrapping. So I think this still needs some work.
>
>
> Thanks for the comments and I've got no objection to any of your
> suggestions. I'm working removing pretty much all logic from the
> BootstrapFilter and moving it to a controlling Struts action so we can
> have some sort of cohesive page flow instead of the dead-enders we
> have now.
>
> - Dave
Re: Roller Easy Install - ready to commit
Posted by Dave <sn...@gmail.com>.
On 6/18/07, Allen Gilliland <al...@sun.com> wrote:
> Some comments after looking at the latest code on this ...
>
> 1. I'm not really liking that the RollerContext object has become a
> DatabaseScriptProvider. I think that functionality should be separated
> out into its own class or something. I think we were guilty of putting
> way too much logic in the RollerContext before and we should try and
> avoid that.
>
> 2. I still think the BootstrapFilter should be simplified even further.
> We want to do as little work there as possible.
>
> 3. Your struts2 actions are ApplicationAware and are pulling things out
> of the servlet context, to me that's a very confusing way of managing
> access to that database script provider object. Why does that class
> need to be a singleton anyways? Why wouldn't the struts2 action just
> instantiate that class when it's actually going to use it?
>
> 4. Unless you have a good reason, I'd really like to keep all the
> struts2 actions extending the UIAction class rather than directly using
> ActionSupport.
>
> Overall I'm still feeling like this code is lacking an overall
> cohesiveness and the code doesn't really match the workflow of doing an
> installation and bootstrapping. So I think this still needs some work.
Thanks for the comments and I've got no objection to any of your
suggestions. I'm working removing pretty much all logic from the
BootstrapFilter and moving it to a controlling Struts action so we can
have some sort of cohesive page flow instead of the dead-enders we
have now.
- Dave
Re: Roller Easy Install - ready to commit
Posted by Allen Gilliland <al...@sun.com>.
Some comments after looking at the latest code on this ...
1. I'm not really liking that the RollerContext object has become a
DatabaseScriptProvider. I think that functionality should be separated
out into its own class or something. I think we were guilty of putting
way too much logic in the RollerContext before and we should try and
avoid that.
2. I still think the BootstrapFilter should be simplified even further.
We want to do as little work there as possible.
3. Your struts2 actions are ApplicationAware and are pulling things out
of the servlet context, to me that's a very confusing way of managing
access to that database script provider object. Why does that class
need to be a singleton anyways? Why wouldn't the struts2 action just
instantiate that class when it's actually going to use it?
4. Unless you have a good reason, I'd really like to keep all the
struts2 actions extending the UIAction class rather than directly using
ActionSupport.
Overall I'm still feeling like this code is lacking an overall
cohesiveness and the code doesn't really match the workflow of doing an
installation and bootstrapping. So I think this still needs some work.
-- Allen
Anil Gangolli wrote:
>
> ----- Original Message Excerpt ---
>> Dave wrote:
> ....
>
>> The logic is simple, but it is not as you describe. Currently, it is
>> this:
>> * If DB connection fails forward to DatabaseError action
>> * Else if Roller tables not found direct to CreateDatabase action
>> * Else if Roller tables need upgrade direct to UpgradeDatabase action
>>
>> I like your idea of having one central install action instead of
>> routing logic in the filter.
>
> Once all of these have passed at least once in a given instance of the
> process, we should be able to retain state to avoid repeating at least
> the latter two checks each time.
>
> I don't know whether actually testing the connection each time is
> worthwhile.
>
> --a.
>
Re: Roller Easy Install - ready to commit
Posted by Anil Gangolli <an...@busybuddha.org>.
----- Original Message Excerpt ---
> Dave wrote:
....
> The logic is simple, but it is not as you describe. Currently, it is this:
> * If DB connection fails forward to DatabaseError action
> * Else if Roller tables not found direct to CreateDatabase action
> * Else if Roller tables need upgrade direct to UpgradeDatabase action
>
> I like your idea of having one central install action instead of
> routing logic in the filter.
Once all of these have passed at least once in a given instance of the
process, we should be able to retain state to avoid repeating at least the
latter two checks each time.
I don't know whether actually testing the connection each time is
worthwhile.
--a.
Re: Roller Easy Install - ready to commit
Posted by Dave <sn...@gmail.com>.
On 6/13/07, Allen Gilliland <al...@sun.com> wrote:
> I'm concerned about a couple things ...
>
> 1. You say you're not happy with the changes in PersistenceSessionFilter
> which sounds ominous and to be honest I'm not even sure why you would
> want to put any code in there. I much prefer the idea of putting the
> code which checks if the application has been properly bootstrapped into
> it's own filter. Call it the the BootstrapCheckFilter if you want. In
> any case, i don't consider that logic to have anything to do with the
> persistence session filter.
I put it in PersistenceSessionFilter because it was easy and related,
but I agree putting it in it's own filter is better. What I'm not so
happy about is the way I'm forwarding to the Struts actions.
> Also, in my mind this filter should be very simple, such as ...
> if(RollerFactory.isBootstrapped()) {
> chain.doFilter(req, res);
> } else {
> // app not bootstrapped yet, prompt for user involvement
> response.sendRedirect(install/upgrade url);
> }
The logic is simple, but it is not as you describe. Currently, it is this:
* If DB connection fails forward to DatabaseError action
* Else if Roller tables not found direct to CreateDatabase action
* Else if Roller tables need upgrade direct to UpgradeDatabase action
I like your idea of having one central install action instead of
routing logic in the filter.
> 2. I don't know what you actually put in your SQLScriptRunner,
> DatabaseCreator, and DatabaseUpgrader classes but I would have thought
> this code could be part of the DatabaseProvider. It would be nice to
> have more details on how these classes work.
Then I should commit them.
> 3. Why would we need users to restart or redeploy after doing the db
> table work? In my mind that kind of defeats the purpose of making the
> installation easy and really there should be no need for this.
I wouldn't say it defeats the purpose, but yes, it is an additional
step that should not be necessary if we have proper bootstrapping in
place.
> 4. I'm not sure exactly how your page flow is working. You have 3 new
> actions defined but I don't understand how users get to each of them and
> what happens after they take the action from each one.
There really is no page flow, if you don't have tables you get
directed to the CreateDatabase page, press the button, see results and
that's it.
> My expectation is that there would be more of a unified experience such
> that if the application has not been bootstrapped yet then the bootstrap
> checking filter forwards all requests to a url such as
> /roller-ui/bootstrap.rol to handle the bootstrapping process. From that
> url you can walk through all parts of the process as needed i.e.
>
> 1. check that db connection works
> 2. do db table install/upgrade work
> 3. check that mail session works
> 4. everything is set, try bootstrapping application again
> And these steps would basically be looped until the bootstrapping
> process actually works. what i see so far doesn't seem to be as much of
> a unified experience, but maybe i'm not understanding the flow?
I like that, but I have not gone that far yet, in part because I've
been separating installation and boostrapping in my mind and in my
workspaces. One reason I'd like to commit is so I can merge changes
into my roller_guice branch to experiment with DI based boostrapping
options.
I think I have a workable solution that is good enough to commit now
for more feedback and improvements. Though I'd like to move the code
from PersistenceSessionFilter first.
> I do think that all the pages that you are showing there look great
> though and I definitely think they will be a huge improvement over our
> current installation process.
Yep. The database error pages alone should reduce install problem list
traffic significantly.
- Dave
Re: Roller Easy Install - ready to commit
Posted by Allen Gilliland <al...@sun.com>.
I'm concerned about a couple things ...
1. You say you're not happy with the changes in PersistenceSessionFilter
which sounds ominous and to be honest I'm not even sure why you would
want to put any code in there. I much prefer the idea of putting the
code which checks if the application has been properly bootstrapped into
it's own filter. Call it the the BootstrapCheckFilter if you want. In
any case, i don't consider that logic to have anything to do with the
persistence session filter.
Also, in my mind this filter should be very simple, such as ...
if(RollerFactory.isBootstrapped()) {
chain.doFilter(req, res);
} else {
// app not bootstrapped yet, prompt for user involvement
response.sendRedirect(install/upgrade url);
}
2. I don't know what you actually put in your SQLScriptRunner,
DatabaseCreator, and DatabaseUpgrader classes but I would have thought
this code could be part of the DatabaseProvider. It would be nice to
have more details on how these classes work.
3. Why would we need users to restart or redeploy after doing the db
table work? In my mind that kind of defeats the purpose of making the
installation easy and really there should be no need for this.
4. I'm not sure exactly how your page flow is working. You have 3 new
actions defined but I don't understand how users get to each of them and
what happens after they take the action from each one.
My expectation is that there would be more of a unified experience such
that if the application has not been bootstrapped yet then the bootstrap
checking filter forwards all requests to a url such as
/roller-ui/bootstrap.rol to handle the bootstrapping process. From that
url you can walk through all parts of the process as needed i.e.
1. check that db connection works
2. do db table install/upgrade work
3. check that mail session works
4. everything is set, try bootstrapping application again
And these steps would basically be looped until the bootstrapping
process actually works. what i see so far doesn't seem to be as much of
a unified experience, but maybe i'm not understanding the flow?
I do think that all the pages that you are showing there look great
though and I definitely think they will be a huge improvement over our
current installation process.
-- Allen
Dave wrote:
> I have already committed part of this work (e.g. DatabaseProvider and
> MailProvider classes) and now I'm ready to commit the rest.
>
> I updated the proposal with latest design, status notes and screenshots:
> http://cwiki.apache.org/confluence/display/ROLLER/Proposal+Easy+Install
>
> In particular, take a look at the section titled "Design for Option
> #1: Property file based configuration" and anything marked READY TO
> COMMIT.
>
> I'm not entirely happy with the changes in PersistenceSessionFilter
> but I'd like to get other's opinions on the code so I'm ready to
> commit. I believe this is safe because the auto-install logic off by
> default.
>
> If there are no objections, I will commit the code later today.
>
> - Dave