Re: [PORTAL] Changes and API review?

[Scott O'Bryan]

Adam,

I agree with many of your points about the reflection.  My comment was 
mainly on casting and casting was still a realitivly slow operation as 
of JDK 1.4 (I havn't tested 1.5).  Still, you have a valid point that 
it's done only once per class-loader per context.  My question remains, 
though, why do it at all when we don't, at this moment, need to cast or 
expose the getInstance() API.  I wholeheartedly disagree that using the 
service loader is the correct answer so for getting the instance we can 
avoid this alltogether by not exposing it.

As for the disable function, there are things that can certainly be done 
to make that more robust.  The first may be to expose some methods to 
the API (like isInRequest) but frankly I'm reluctant to do that without 
a GlobalConfigurator api exposed.  The other way, perhaps, is to expose 
a utility to the API to check to see if the base configurator is current 
inside of a request.  The impl of the utility could exist in the impl 
package and the global (base) configurator would not have to be exposed 
at all.

Both of the above, however, depend on being able to load classes which 
exist in the Impl package from the API and, for many of the reasons I 
stated in my last email, the Service Loader is completly the wrong 
thing.  This is a challenge that is shared by, not only, the 
RequestContext but also the TrinidadFilter.  And it's a challenge of 
every API factory that needs to refer to an Impl.  Faces has a 
FactoryFinder which solves many of these problems, but I frankly think a 
system like that is overkill.  Especially since we are expecting only 
one "global-type" configurator.  Frankly, I'm not sure I really care 
what the mechanism is that we use and making it a constant is an 
implementation choice and has no bearing on the API so long as the 
factory method returns the proper object.  I therefore don't really see 
the harm in using a constant for the short term and then, at a later 
date, look at replacing it with a more robust system that might handled 
this in a better fashion.  I really don't think the service loader is 
that mechanism, however.

Scott

Adam Winer wrote:
Scott,

A few points.

First, I said only that initializing with construction is more
robust then separating the two, and only that.  That's plainly true.
Why you want to turn that around to ad hominem generalizations
is beyond me and not productive.

Second, it certainly is a good point that you now might disable
the configurator "too late", which could easily lead to
inconsistencies  That could be caught in a couple of ways - a good
one would be warning the developer on the next request.

GlobalConfiguratorImpl could easily handle
this on its own, and both make sure that "if you start it, it
finishes", and warn (and ignore) on the next configurator
invocation if someone has disabled too late.  Disabling
the configurator is a rare enough requirement (in the number
of places that'll want to do it) that this is a sufficient improvement
to deal with that legitimate issue - all we need to do is catch
anyone that gets it wrong (and avoid misbehaviors when it does
happen).

Third, you really need to stop imagining introspection is
a horribly slow operation.  That was true in JDK 1.2,
but hardly the case at all now.  And introspection *once during
initialization*, not even once per request, isn't even remotely a
performance issue, and describing it as such is just plain
wrong.

Fourth, referring to GlobalConfiguratorImpl by name from the API
is awful.  (We do this for the RequestContextFactoryImpl, and
I've always disliked that decision).  It totally breaks API-IMPL
separation and dependency order.  We're talking lesser of two
evils here, and it's a judgement call.  That said, I'm happy with moving
Configurator.getInstance() down to impl (into GlobalConfiguratorImpl),
as you're right that there isn't an obvious reason why it would need 
to be
exposed at this point.  That gets rid of the need to use any 
introspection
API (Services or otherwise) to access GlobalConfiguratorImpl.

Finally, I had little choice *but* to check in code on that branch.  It's
version controlled, so if it breaks portlets, then svn revert is trivial.
But when you repeatedly misrepresent the technical aspects of what
I'm proposing, claiming everything I propose is horrific and slow and
ugly and will break some ill-defined API that is still to come,
how else can I show you what I want???  Now, let's work together to go
forwards.  My checkin isn't end-of-story, no more than yours was.

FWIW, I can't believe that JSR-301 is going to somehow make this
API unusable.  It will obviously be a different API, but I can't believe
that it'll be so different that it'll be completely impossible to use
that API such that it can invoke whatever it is that we come up with.

-- Adam



On 12/22/06, Scott O'Bryan <[EMAIL PROTECTED]> wrote:

Adam,

You I must have a different definition of robust.  Two of the MANY
examples that illustrate my point is as follows:



1. There is no error checking on disableConfiguratorServices method and,
the implementation as it stand now, could force us to break our contract
with the configurators.  Furthermore, it's not always obvious that
something wrong was done.  For instance, the following code will not
generate an error:

FacesContectFactory.getFacesContext(context, request, response,
lifecycle);
...
Configurator.disableConfiguratorServices();

This will not generate an error yet cleanup (endRequest) will not occur
even though beginRequest WAS called (when the FacesContextFactory was
created OR when the filter is run).  getExternalContext MAY or MAY NOT
be wrapped depending on whether or not an external context was retrieved
before the disable.  The basically leaves the status of externalContext
in an unknown state and will not execute the end Request.  You
eliminated the "reflection" in my latest patch by getting rid of this
check.  In a public "rhobust" API, this really needs to be enforced.  It
USED to call an IllegalStateException so it didn't necessarily have to
be caught because it should be fairly obvious at the time someone writes
their code.

2. You got rid of reflection in the public Configurator class by using
Trinidad's Service Loader.  This class not ONLY uses reflection, but it
also reads a bunch of files and returns an "array" of initialized
services.  Do we really need to open some file objects and initialize an
extra array for no reason?  Now it's an interesting idea to be able to
replace the global configurator, but first off that wasn't a requirement
that I was coding for and second off, we cannot guarantee the "order"
that the services files are loaded in.  Again this leaves us in an
unknown state if we are trying to override the default functionality and
is simply using a sledge hammer to drive a nail if we aren't.

You were able to eliminate a lot of the casting by not exposing the is
initialized method on the global configurator by moving the
initialization code inside of "getInstance" method.  I kept this
separate so that I could adjust to JSR-301 as I need to.  Butt the API
is now locked in to perform initialization at that stage.  This is
something that very well could become a problem.  And I haven't even
taken a look at how you handled the isInRequest functionality that I
need to maintain the proper flexibility in the portal case.

Still, I really don't have time to argue these points.  I'll just have
to generate Jira tickets as problems come up and we'll have to change
the API's if we need to change them.  All I really have to say about the
subject is allowing the GlobalConfigurator implementation handles all
these issues for free.  And using my latest checkins handles most of
them, just not as cleanly.  Certainly. though, a lot more cleanly then
using the "Service Loader".

You said that you didn't know if these changes worked in a portal
environment, and I'm not real sure I can confirm that within the next
week or so.  But checking in changes to a "portal compatibility" branch
without being able to check them in a Proof of Concept (POC) is really
damaging to the project being that Portal compatibility is what this
project is all about.  I was able to test my stuff as a POC using the
Oracle Portal and Bridge, and I'm also aware of the changes that need to
be made in the MyFaces Bridge to support these cases.  Many of these
issues ARE going to be addressed in JSR-301 but my hope it to have the
myfaces bridge up to par before that time.

My final, and I mean final, comment on this is that it is important to
me to have error checking on disableConfiguratorServices. That's from a
robust API perspective.  From a Portal standpoint, it is far more
important for me to have control over the initialization logic and to
error out of getInstance when an instance is not available (or return
null, I don't care which) then it is for me to have an exposed
"getInstance()" in the API and for the thing to return a dummy
configurator.  Exposing the getInstance made sense when we had the
ability to tack additional non-static API's onto the
GlobalConfigurator.  Since we don't, I can't see ANY reason why someone
would need this API outside of Trinidad.  The object is simply "not
useful" outside of this codebase anymore.  Plus it keeps us flexible to
add an API later if we need one whereas this current implementation
LOCKS us in to providing a plain run-of-the-mill configurator.  Lastly,
for GOD sake, don't use the service loading system to load this class.
The Service loading system is great for making extensions to Trinidad,
but it has limitations which stem from the fact that order within these
services is not guaranteed.  For now I say we simply have the
GlobalConfiguratorImpl (in the Impl package) be referenced directly if
we need to, and if we have the need to add additional
globalConfigurators in the future, we can come up with an API that makes
sense.  This one makes no sense at all.  Essentially I outlined for you
the things in my last checkin.  You're latest checkin' will lock us into
unwanted implementations, implementations that make NO sense what so
ever, and implementations that are not robust.

As I stated before, though, I don't really have time to argue with you
anymore.  I have some other projects that I need to work on at the
moment.  So if you're happy with the API, call a vote and let's get this
moved in so I can move forward and we can hopefully have trinidad
working inside of a MyFaces Portal environment sometime this century.

Scott O'Bryan

Adam Winer wrote:
> On 12/21/06, Scott O'Bryan <[EMAIL PROTECTED]> wrote:
>>
>> Adam,
>>
>> Well, you basically implemented one of the solutions I said I didn't
>> like earlier, but oh well.  And there are a number of places you need
to
>> cast.  So the concerns are still valid.
>
>
> Well, I don't get that claim, as I didn't add a single cast in my
> patch at all, and removed references to the impl class, so I can't
> imagine
> how I've made things worse.  And I didn't really see any casts that 
were
> already there.
>
> The one question I do have is why does getInstance take in an
>> ExternalContext?  I'm assuming it's because your executing the init
>> inside of the getInstance object and I'm not sure this is the correct
>> place for this.  My hope in all of this was to be able to explicitly
>> handle initialization of this object.  Plus, nine times out of 
ten, the
>> ExternalContext object is ignored in the call to this method.
>
>
> A create + initialize construct is more robust than a create-and-hope-
> it-gets-initialized-by-someone-else construct.
>
> -- Adam
>
>
>
> Either way, don't see as I really have the time to argue.  So is it
>> acceptable to everyone?
>>
>> Also, there is no
>>
>>
>> Adam Winer wrote:
>> > Scott,
>> >
>> > OK, well, I just went ahead and implemented what I was trying
>> > to say, to see if I'd run into the problems you're describing.  I
>> > didn't...
>> > (It's possible I've broken something in portlet land - I only 
tested
>> > the changes in a servlet environment.)
>> >
>> > On 12/21/06, Scott O'Bryan <[EMAIL PROTECTED]> wrote:
>> >> Adding getInstance() to the configurator will either force us to
cast
>> in
>> >> a bunch of different places
>> >
>> > No, it doesn't.  No casts were required at all, much
>> > less in a bunch of places, and most of the code now
>> > doesn't even have references to the impl class at all.
>> >
>> >> or to expose the GlobalConfiguratorImpl's
>> >> api to the rest of the world (which I don't want to do because
>> they are
>> >> applicable ONLY to global configurator.  And it won't lock us into
an
>> >> API we may have to expand later.
>> >>
>> >> As for simply putting the Boolean on the request map, either I'll
>> have
>> >> to make a protected constant on the Configurator interface (which
has
>> no
>> >> bearing on any of the configurators except the global configurator
so
>> it
>> >> shouldn't go into the ancestor) or I add a porotected (isDisabled)
>> >> method (which, again, is only applicable to the
>> GlobalConfiguratorImpl
>> >> and therfore shouldn't do into it's Ancestor).
>> >
>> > See the code checked in, which does not suffer these probems.
>> >
>> >> I've never been one to include a feature into an interface or a
class
>> >> that is applicable in only one instance of that class because it
>> >> violated basics OO design principals.  The only other option I see
>> here
>> >> is to define the constant used to store the boolean in BOTH
>> classes and
>> >> hope they remain in sync, but I've never been a big fan of that
>> either.
>> >
>> > Again, see the code checked in.
>> >
>> > -- Adam
>> >
>>
>>
>