incubator-adffaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam Winer" <awi...@gmail.com>
Subject Re: [PORTAL] Changes and API review?
Date Sat, 23 Dec 2006 22:43:54 GMT
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 <darkarena@gmail.com> 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 <darkarena@gmail.com> 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 <darkarena@gmail.com> 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
> >> >
> >>
> >>
> >
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message