myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Struberg <strub...@yahoo.de>
Subject Re: StateSaving reviews
Date Fri, 16 Nov 2012 00:41:21 GMT
The usual release cycle is  2 to 3 months. The next expected release is early/mid November and not next week. If you are too stressed I can take over doing the release. 


> Let's make a deal, let me do it myself. I just take a snapshot of your 
> changes,
> revert, and apply only the refactoring step by step, then I compare it. In that
> way, I'll be 100% sure the change is correct. Sounds good for you?
Leo, please learn to read foreign peoples code! It's really not that complicated. It's the same others have to do over and over again with your code as well. You can perfectly achieve the same by just reading through the diffs. That's the reason we send them to the committers list. Those diffs contain all the changes.


Going back to the old source is also not really an option in my personal opinion as it 

a.) contained a bug (NPE). There is a unit test for it now btw. 

b.) worked _completely_ different in ProjectStage.Production and all others ProjectStages thus made it tremendously hard to debug/reproduce for developers. 

And I'm not yet talking about stylistics like ode duplications, unused fields and generic types and endless inner classes. 


And also we had this kind of discussion quite a few times already and you always ended up reverting without giving any technical reasoning. That sucks to be honest.


For gods sake, a very last time I let you revert something. 

Please first rethink (and then probably do if you are still convinced about it's necessity) the following: 
* revert to the old state
* remove the ProjectStage.Production and always use the same key strategy. This is really crucial for stability!
* apply the unit test I created and fix the NPE.
* move the viewId.hashCode() to the key strategy constructor and you will clearly see the code duplication all over the place

* you can also spare the byte[] variant as this can be perfectly done inside the key strategy. There goes another code duplication. That will btw remove a broken unchecked upcast as well.

* remove the bloody src/main/old
* I bet I forgot some other cleanup I did 


I will drop/cleanup the rest do the windowId handling after this release if you get it out of the door this week. 


Btw you should either copy over Tom and Manfreds @author from the JspViewStateManager or remove your own @author from the copied class as it is still 80% the old code.


LieGrue,
strub


----- Original Message -----
> From: Leonardo Uribe <lu4242@gmail.com>
> To: MyFaces Development <dev@myfaces.apache.org>; Mark Struberg <struberg@yahoo.de>
> Cc: 
> Sent: Friday, November 16, 2012 1:11 AM
> Subject: Re: StateSaving reviews
> 
> Hi
> 
> 2012/11/15 Mark Struberg <struberg@yahoo.de>:
>>  Kito, there was no single word inside the PMC that the release will be next 
> week. I just again scanned our internal mailboxes to verify that. Leo is just 
> adding another messy argument which is not funded.
>>  Of course there will be a release soon, but there was no agreed date yet.
>> 
> 
> There is an agreement of make a release each two months, or earlier
> if something happen. Check the dates, and you will see the systematic
> pattern. That was started some years ago, and people already trust that.
> 
>>  And again: there is no effective code change. Just making inner classes 
> visible to uncover the mess.
>> 
> 
> Let's make a deal, let me do it myself. I just take a snapshot of your 
> changes,
> revert, and apply only the refactoring step by step, then I compare it. In that
> way, I'll be 100% sure the change is correct. Sounds good for you?
> 
> regards,
> 
> Leonardo Uribe
> 
>>  LieGrue,
>>  strub
>> 
>> 
>> 
>> 
>>  ----- Original Message -----
>>>  From: Kito Mann <kito.mann@virtua.com>
>>>  To: MyFaces Development <dev@myfaces.apache.org>
>>>  Cc:
>>>  Sent: Friday, November 16, 2012 12:54 AM
>>>  Subject: Re: StateSaving reviews
>>> 
>>>  As an observer to this argument and someone whose clients depend on
>>>  the stability of the code base, such a  refactoring so close to
>>>  release seems like a bad idea. Why not do this after the release?
>>> 
>>>  Sent from my iPhone
>>> 
>>>  http://www.jsfcentral.com
>>>  http://www.Virtua.com
>>> 
>>> 
>>>  On Nov 15, 2012, at 6:36 PM, Mark Struberg <struberg@yahoo.de> 
> wrote:
>>> 
>>>> 
>>>>>   But you reverted the changes I did months ago. Didn't you? 
> oh yes,
>>>  you
>>>>>   "refactor" it and get rid what you don't like.
>>>> 
>>>>   That is OSS Leo. We take what we find and improve it. This is not 
> a revert
>>>  - this is evolution!
>>>> 
>>>>   There is no such thing like 'my code' in the ASF.
>>>> 
>>>> 
>>>>>   That's unfair. I don't think that's true.
>>>>   Look at what happened with the RelativeResourceHandler and where 
> it lives
>>>  now for example. Just to name one in the not so far past.
>>>>   I'm not going further with commenting any of your personal 
> attacks but
>>>  will further again only focus on technical arguments.
>>>> 
>>>> 
>>>>   As I said before, we can rollback the viewId to the hashCode if 
> you like.
>>>  I'm perfectly fine with that.
>>>> 
>>>>   I'm also fine with moving the classes to another package root. 
> But
>>>  I'm -1 on the original 15++ inner classes implementation because 
> this is
>>>  unmaintainable.
>>>> 
>>>> 
>>>>   Again: if we set the key back to hashCode then there is no 
> functional
>>>  change to the previous version! It's just own classes instead of 
> inner
>>>  classes + removing duplicated code.
>>>> 
>>>> 
>>>>   Actually you really can blame me for something (actually not only 
> me but
>>>  all other community members as well): to not have reviewed that part 
> earlier. I
>>>  confess for that part. This is a complicated area and we should have 
> given
>>>  feedback way earlier as this is hard to get right. But it doesn't 
> get easier
>>>  by having large changes developed locally and then committing it in one 
> big
>>>  step.
>>>> 
>>>> 
>>>>>   Since the plan is do a release next week, could
>>>>>   we revert the changes, and put them on hold
>>>> 
>>>>   I still don't see the reason for it. The code is fundamentally 
> the same
>>>  (after moving back to the hashCode) and most likely will pass the TCK.
>>>>   Please give it a try or provide fundamental feedback. The classes 
> are still
>>>  the same, they only got moved from inner classes to own classes.
>>>> 
>>>> 
>>>>>   https://jira.springsource.org/browse/SWF-1365
>>>>   This bug got fixed and released more than 2 years ago in SWF. This 
> was even
>>>  done before swf-2.0 went final. I see no reason to keep any old code in 
> MyFaces
>>>  only for that.
>>>> 
>>>>   LieGrue,
>>>>   strub
>>>> 
>>>> 
>>>> 
>>>>   ----- Original Message -----
>>>>>   From: Leonardo Uribe <lu4242@gmail.com>
>>>>>   To: MyFaces Development <dev@myfaces.apache.org>; Mark 
> Struberg
>>>  <struberg@yahoo.de>
>>>>>   Cc:
>>>>>   Sent: Friday, November 16, 2012 12:11 AM
>>>>>   Subject: Re: StateSaving review
>>>>> 
>>>>>   Hi
>>>>> 
>>>>>   2012/11/15 Mark Struberg <struberg@yahoo.de>:
>>>>>>   Leo, it would be great if you reflect a bit.
>>>>>> 
>>>>>>>   I have checked the code. I know what SerializedViewKey 
> does.
>>>>>> 
>>>>>>   good! no other arguments? Well, then all is fine it seems.
>>>>>> 
>>>>>> 
>>>>>>>   it is YOU the one who are reverting code done by other 
> people
>>>>>> 
>>>>>>   The only code I ever reverted in MyFaces was your revert 
> of my code
>>>  change.
>>>>>   That's it.
>>>>> 
>>>>>   But you reverted the changes I did months ago. Didn't you? 
> oh yes,
>>>  you
>>>>>   "refactor" it and
>>>>>   get rid what you don't like.
>>>>> 
>>>>>>   But actually I'm not the only one who got code 
> reverted by you,
>>>  and
>>>>>   those people are fed up as well and partly left the community 
> already
>>>  because of
>>>>>   that.
>>>>>> 
>>>>> 
>>>>>   That's unfair. I don't think that's true. I'm 
> just
>>>  exercising my
>>>>>   right
>>>>>   to participate, that's it. People can disagree from time 
> to time.
>>>>>   That's part of the life, if you can't deal with it, 
> it's
>>>  your
>>>>>   problem.
>>>>>   But I'm always willing to look for alternatives and 
> resolve
>>>>>   differences, like I'm doing right now, and that's what 
> makes
>>>  the
>>>>>   difference.
>>>>> 
>>>>>> 
>>>>>>>   I'm sorry to be so direct, but as a member of the
>>>  community, I
>>>>>>>   have the right to participate and I'm willing to 
> exercise
>>>  that
>>>>>   right.
>>>>>> 
>>>>>>   That's perfectly fine as well. So please start your 
> excercise
>>>  with
>>>>>   listening to other people sometimes.
>>>>>>   * I listed 5++ flaws in the design.
>>>>>>   * I removed a few classes which contained 99% copy & 
> paste.
>>>>>>   * I removed a few classes which are not used anymore since 
> over a
>>>  year
>>>>>>   * I removed an old resources folder which was just lying 
> around for
>>>  no
>>>>>   reason.
>>>>>>   * I moved the remaining inner classes to toplevel and an 
> own
>>>  package to
>>>>>   make it easier to maintain and overlook the whole complexity.
>>>>>>   * I added the viewId to _one_ of the dozen key 
> implementations.
>>>  Actually we
>>>>>   don't need it. We also don't need the viewId hashCode 
> which was
>>>  there
>>>>>   before. So I don't care, let's move this back to your 
> hashCode
>>>  solution
>>>>>   and fix it properly in the next release.
>>>>>>   * I cleaned up your generics because it always only used 
> the String
>>>  type
>>>>>   anyway. And this cannot change in the future because this fact 
> comes
>>>  directly
>>>>>   from the JSF spec.
>>>>>>   Otherwise there has been no fundamental change to your 
> code. The
>>>  mess you
>>>>>   see is all yours so to say (sorry for the sarcasm).
>>>>>> 
>>>>> 
>>>>>   That's your opinion, not mine. But I can see some advance. 
> At last,
>>>  there is
>>>>>   one list of the changes proposed. Since the plan is do a 
> release next
>>>>>   week, could
>>>>>   we revert the changes, and put them on hold while the 
> necessary steps
>>>  are done?
>>>>>   Otherwise a release will not be possible for a long time (In 
> fact, you
>>>>>   have kidnapped
>>>>>   the release). In that way, we can dedicate enough time and 
> resources to
>>>  get
>>>>>   it out once for all. Do you agree with that? Could you let me 
> complete
>>>>>   the planned
>>>>>   schedule?
>>>>> 
>>>>>   Anyway, what's the deal to put the changes in 2.1.x 
> branch? why you
>>>  are so
>>>>>   desperate for that? You have not given to me any reason why 
> you
>>>  can't work
>>>>>   with
>>>>>   2.1.x-client-window branch, which is best for what you are 
> trying to
>>>>>   do, or why you
>>>>>   can wait just 2 weeks to commit the changes.
>>>>> 
>>>>>   Come on Mark, is not a big deal, right?
>>>>> 
>>>>>> 
>>>>>>>   The concrete case is spring webflow does not support 
> PSS
>>>  algorithm, so
>>>>>>>   we need to switch back and enable compatibility mode 
> with JSF
>>>  1.2.
>>>>>> 
>>>>>>   Spring WebFlow uses it's own StateManager which 
> natively
>>>  extends
>>>>>   javax.faces.StateManager. There is no MyFaces code involved 
> anymore
>>>  since ages!
>>>>>> 
>>>>> 
>>> 
> http://static.springsource.org/spring-webflow/docs/2.0.x/javadoc-api/org/springframework/faces/webflow/FlowViewStateManager.html
>>>>>> 
>>>>>>   I also googled quite some time and fine nothing pointing 
> to any
>>>  problems
>>>>>   with the MyFaces StateManager. Not even on stackoverflow.
>>>>>> 
>>>>> 
>>>>>   Look this:
>>>>> 
>>>>>   https://jira.springsource.org/browse/SWF-1365
>>>>> 
>>>>>   Because that problem, I decided to preserve 
> JspStateManagerImpl long
>>>  time ago,
>>>>>   specifically when MYFACES-3117 was solved, because the change 
> over
>>>>>   ResponseStateManager imposed a change over all state 
> management in
>>>  MyFaces.
>>>>>   Is there a problem? don't care if exists or not, 
> what's
>>>  important is
>>>>>   provide
>>>>>   a way to go back to the old behavior. Be conservative, keep 
> code
>>>  stable.
>>>>> 
>>>>>   regards,
>>>>> 
>>>>>   Leonardo Uribe
>>>>> 
>>>>>> 
>>>>>> 
>>>>>>   LieGrue,
>>>>>>   strub
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>>   ----- Original Message -----
>>>>>>>   From: Leonardo Uribe <lu4242@gmail.com>
>>>>>>>   To: MyFaces Development 
> <dev@myfaces.apache.org>; Mark
>>>  Struberg
>>>>>   <struberg@yahoo.de>
>>>>>>>   Cc:
>>>>>>>   Sent: Thursday, November 15, 2012 11:06 PM
>>>>>>>   Subject: Re: StateSaving review
>>>>>>> 
>>>>>>>   Hi
>>>>>>> 
>>>>>>>   2012/11/15 Mark Struberg <struberg@yahoo.de>:
>>>>>>>> 
>>>>>>>>>     You did more than that. You removed the 
> following
>>>  classes:
>>>>>>>>> 
>>>>>>>>>     IntIntSerializedViewKey
>>>>>>>>>     IntByteArraySerializedViewKey
>>>>>>>>>     ReferenceSerializedViewKey
>>>>>>>> 
>>>>>>>>     Those have been almost 1:1 clones of other 
> classes. Go
>>>  check
>>>>>>>   SerializedViewKey, it contains all the merged 
> functionality.
>>>  Without
>>>>>   any
>>>>>>>   performance overhead.
>>>>>>>>     Look at the code first and then you can 
> complain. You
>>>  revert code
>>>>>   done by
>>>>>>>   other people without even looking at the changes it 
> seems.
>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>>>>   I have checked the code. I know what SerializedViewKey 
> does.
>>>>>>>   I'm pretty aware of what it does and its effects. 
> I have to
>>>  say it,
>>>>>>>   it is YOU the one who are reverting code done by other 
> people
>>>>>>>   without even looking at the changes, just because you 
> have
>>>>>>>   the perception that the changes are wrong, but nothing 
> more.
>>>>>>>   Without any proof or any deep consideration.
>>>>>>> 
>>>>>>>   The changes I have done over the time has follow the 
> same
>>>>>>>   protocol, create an issue, provide a patch, wait for 
> feedback
>>>  of
>>>>>>>   the community, if no objections after a prudent time 
> commit
>>>  them
>>>>>>>   and close the issue. In this moment the changes done 
> by me has
>>>>>>>   the tacit aval of the community. Your changes do not 
> have that,
>>>>>>>   because they have objections done 72 hours after they 
> were
>>>>>>>   proposed. This is a meritocracy, and the rules of the 
> community
>>>>>>>   in this case are on my side.
>>>>>>> 
>>>>>>>   I'm sorry to be so direct, but as a member of the
>>>  community, I
>>>>>>>   have the right to participate and I'm willing to 
> exercise
>>>  that
>>>>>   right.
>>>>>>>   I will not look to other side and let some untested 
> code go
>>>  into
>>>>>>>   myfaces core 2.1.x. I appreciate your efforts to make 
> this part
>>>>>>>   better, but I encourage you to respect and play with 
> the rules
>>>>>>>   of the community.
>>>>>>> 
>>>>>>>   Look the dates of the improvements:
>>>>>>> 
>>>>>>>   MYFACES-3563
>>>>>>>   Created: 10/Jun/12 11:33
>>>>>>>   Resolved: 25/Jun/12 09:05
>>>>>>> 
>>>>>>>   MYFACES-3117
>>>>>>>   Created: 26/Apr/11 19:08
>>>>>>>   Resolved: 14/May/11 02:27
>>>>>>> 
>>>>>>>   That code has been there for years!!!!!! , the latest 
> changes
>>>  were done
>>>>>>>   5 months ago !!!. Nobody complained at that time. Now 
> you came
>>>  here,
>>>>>>>   complaining about the code and you want to fix it 
> without any
>>>  interest
>>>>>>>   of what others think about that. That's not cool.
>>>>>>> 
>>>>>>>> 
>>>>>>>>>     and on top of that you removed 
> JspStateManagerImpl,
>>>  which is a
>>>>>   class
>>>>>>>>>     that is there for compatibility with old 
> state
>>>  managers that
>>>>>   relies on
>>>>>>>>>     MyFaces 1.2.x.
>>>>>>>> 
>>>>>>>>     This is not JSF-1.x since a long time. APIs in 
> the
>>>  StateManager
>>>>>   got changed
>>>>>>>   for AJAX support, etc.
>>>>>>>> 
>>>>>>>>     But I'm all fine with going back to the
>>>  JspStateManagerImpl
>>>>>   again as I
>>>>>>>   already said.
>>>>>>>> 
>>>>>>>>     Btw, may I quote your answer to my question if I 
> can
>>>  delete it?
>>>>>>>>     LU> "Remove that code is a valid 
> option."
>>>>>>>> 
>>>>>>> 
>>>>>>>   The concrete case is spring webflow does not support 
> PSS
>>>  algorithm, so
>>>>>>>   we need to switch back and enable compatibility mode 
> with JSF
>>>  1.2.
>>>>>>>   JspStateManagerImpl is there because the state manager 
> in that
>>>  library
>>>>>>>   relies on the old behavior. I think there are other 
> cases too.
>>>  But I
>>>>>   hope to
>>>>>>>   remove it in the future (JSF 2.2).
>>>>>>> 
>>>>>>>   regards,
>>>>>>> 
>>>>>>>   Leonardo Uribe
>>>>>>> 
>>>>>>>>     LieGrue,
>>>>>>>>     strub
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>>     ----- Original Message -----
>>>>>>>>>     From: Leonardo Uribe 
> <lu4242@gmail.com>
>>>>>>>>>     To: MyFaces Development
>>>  <dev@myfaces.apache.org>; Mark
>>>>>   Struberg
>>>>>>>   <struberg@yahoo.de>
>>>>>>>>>     Cc:
>>>>>>>>>     Sent: Thursday, November 15, 2012 10:20 PM
>>>>>>>>>     Subject: Re: StateSaving review
>>>>>>>>> 
>>>>>>>>>     Hi
>>>>>>>>> 
>>>>>>>>>     2012/11/15 Mark Struberg 
> <struberg@yahoo.de>:
>>>>>>>>>>      Leo, the stuff which currently got 
> changed is
>>>  nothing
>>>>>   more than
>>>>>>>   just your
>>>>>>>>>     code. I only refactored it into an own 
> package and
>>>  moved the
>>>>>   inner
>>>>>>>   classes to
>>>>>>>>>     toplevel to show up the complexity of the 
> solution.
>>>  There was
>>>>>   no code
>>>>>>>   change
>>>>>>>>>     _yet_ other than the viewId. We can change 
> back the
>>>  viewId to
>>>>>   the hash
>>>>>>>   if you
>>>>>>>>>     feel better. It makes no difference anyway.
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>     You did more than that. You removed the 
> following
>>>  classes:
>>>>>>>>> 
>>>>>>>>>     IntIntSerializedViewKey
>>>>>>>>>     IntByteArraySerializedViewKey
>>>>>>>>>     ReferenceSerializedViewKey
>>>>>>>>> 
>>>>>>>>>     also you created this one (it was an 
> abstract class
>>>  before)
>>>>>>>>> 
>>>>>>>>>     SerializedViewKey
>>>>>>>>> 
>>>>>>>>>     and on top of that you removed 
> JspStateManagerImpl,
>>>  which is a
>>>>>   class
>>>>>>>>>     that is there for compatibility with old 
> state
>>>  managers that
>>>>>   relies on
>>>>>>>>>     MyFaces 1.2.x. Obviously you have not 
> considered
>>>  those cases,
>>>>>   but I
>>>>>>>   have
>>>>>>>>>     done that long time ago.
>>>>>>>>> 
>>>>>>>>>     Without ask, without propose a patch, 
> without let the
>>>  people
>>>>>   who care
>>>>>>>   about
>>>>>>>>>     to participate. Just do it and let others do 
> the
>>>  clean up.
>>>>>>>>> 
>>>>>>>>>     Mark, I have came here and proposed to you 
> many
>>>  possible
>>>>>   alternatives
>>>>>>>>>     and you're letting me without choices. I 
> beg you
>>>  to move
>>>>>   your
>>>>>>>   changes to
>>>>>>>>>     2.1.x-client-window branch, and when we have 
> certain
>>>  about
>>>>>   which
>>>>>>>   changes
>>>>>>>>>     needs to be done, backport them to 2.1.x. In 
> just one
>>>  step,
>>>>>   without
>>>>>>>   this
>>>>>>>>>     pain and stress you are causing me. Please. 
> The
>>>  reason why it
>>>>>   was
>>>>>>>   created
>>>>>>>>>     2.1.x-client-window was precisely to try 
> changes for
>>>  client
>>>>>   window /
>>>>>>>   state
>>>>>>>>>     manager and flash scope.
>>>>>>>>> 
>>>>>>>>>     In this moment I can't do any release. 
> You have
>>>  tied my
>>>>>   hands.
>>>>>>>   I'm
>>>>>>>>>     trying to be
>>>>>>>>>     kind, please.
>>>>>>>>> 
>>>>>>>>>>      In the long run this stuff needs 
> _heavy_
>>>  cleanup.
>>>>>>>>>>      Again: Extracting out an abstraction 
> over Server
>>>  vs
>>>>>   Client
>>>>>>>   stateSaving is
>>>>>>>>>     perfectly fine, but the code is now way more
>>>  complicated than
>>>>>   it ever
>>>>>>>   has been.
>>>>>>>>>     Premature abstraction is way worse than 
> premature
>>>>>   optimisation.
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>     It is a work in progress, I already told you 
> many
>>>  times. I
>>>>>   know the
>>>>>>>   abstraction
>>>>>>>>>     is not the best, but it works, and has 
> helped a lot
>>>  to me to
>>>>>   understand
>>>>>>>   how
>>>>>>>>>     this algorithm should work. It is like a 
> worm that
>>>  became
>>>>>   chrysalis and
>>>>>>>   in the
>>>>>>>>>     future will become a butterfly. "... 
> the
>>>  morphing step is
>>>>>   not nice
>>>>>>>>>     ..."
>>>>>>>>> 
>>>>>>>>>     regards,
>>>>>>>>> 
>>>>>>>>>     Leonardo
>>>>>>>>> 
>>>>>>>>>>      LieGrue,
>>>>>>>>>>      strub
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>>      ----- Original Message -----
>>>>>>>>>>>      From: Leonardo Uribe
>>>  <lu4242@gmail.com>
>>>>>>>>>>>      To: MyFaces Development
>>>>>   <dev@myfaces.apache.org>; Mark
>>>>>>>   Struberg
>>>>>>>>>     <struberg@yahoo.de>
>>>>>>>>>>>      Cc:
>>>>>>>>>>>      Sent: Thursday, November 15, 2012 
> 9:30 PM
>>>>>>>>>>>      Subject: Re: StateSaving review
>>>>>>>>>>> 
>>>>>>>>>>>      Hi
>>>>>>>>>>> 
>>>>>>>>>>>      2012/11/15 Mark Struberg
>>>  <struberg@yahoo.de>:
>>>>>>>>>>>>       Leo, I'm ok with a revert. 
> But not
>>>  to your
>>>>>   version
>>>>>>>   but to the
>>>>>>>>>     original
>>>>>>>>>>>      JspStateManagerImpl!
>>>>>>>>>>>> 
>>>>>>>>>>>>       Really, this currently feels 
> so
>>>  overcomplicated
>>>>>   without
>>>>>>>   giving
>>>>>>>>>     much
>>>>>>>>>>>      benefit. It would have been 
> perfectly enough
>>>  to
>>>>>   remove the
>>>>>>>   viewId
>>>>>>>>>     String and
>>>>>>>>>>>      replace it with a XORShift random 
> value if
>>>  you
>>>>>   don't trust
>>>>>>>   the
>>>>>>>>>     sequencer.
>>>>>>>>>>>> 
>>>>>>>>>>>>       I know one goal was to 
> abstract out the
>>>  state
>>>>>   between
>>>>>>>   client and
>>>>>>>>>     server.
>>>>>>>>>>>      But that doesn't mean that we 
> end up
>>>  with Object,
>>>>>   Object
>>>>>>>   and do
>>>>>>>>>     hardcoded
>>>>>>>>>>>      upcasts as done right now in the 
> code. This
>>>  is no
>>>>>   improvement
>>>>>>>   over the
>>>>>>>>>     original
>>>>>>>>>>>      code. The windowId change would 
> have been 30
>>>  LOC btw.
>>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>>      It is fine if you want to change 
> the
>>>  implementation
>>>>>   to make
>>>>>>>   something
>>>>>>>>>>>      better. What's not fine, is 
> that you
>>>  want to work
>>>>>   in 2.1.x
>>>>>>>   branch
>>>>>>>>>>>      directly, that is right now in 
> maintenance
>>>  stage. So,
>>>>>   you are
>>>>>>>   doing
>>>>>>>>>>>      all kind of refactorings, and that 
> makes me
>>>  crazy.
>>>>>   The last
>>>>>>>   release
>>>>>>>>>>>      was done on 23/Sep/12 and I'm 
> doing 1
>>>  release
>>>>>   each 2
>>>>>>>   months. Your
>>>>>>>>>>>      changes will take longer and I need 
> time to
>>>  review
>>>>>   them,
>>>>>>>   because I
>>>>>>>>>>>      will not do a release over changes 
> that I
>>>  have not
>>>>>   seen.
>>>>>>>>>>> 
>>>>>>>>>>>      My idea is start a release process 
> next
>>>  week. Why
>>>>>   don't
>>>>>>>   you take
>>>>>>>>>     your
>>>>>>>>>>>      time?, instead you try to do 
> changes without
>>>  any
>>>>>   clear
>>>>>>>   direction.
>>>>>>>>>>> 
>>>>>>>>>>>      In this moments, we are starting 
> the
>>>  necessary steps
>>>>>   to work
>>>>>>>   on 2.2.x.
>>>>>>>>>>>      You should be working in 
> 2.1.x-client-window
>>>  branch
>>>>>   !!!!!
>>>>>>>>>>> 
>>>>>>>>>>>      regards,
>>>>>>>>>>> 
>>>>>>>>>>>      Leonardo Uribe
>>>>>>>>>>> 
>>>>>>>>>>>>       LieGrue,
>>>>>>>>>>>>       strub
>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>>       ----- Original Message -----
>>>>>>>>>>>>>       From: Leonardo Uribe
>>>>>   <lu4242@gmail.com>
>>>>>>>>>>>>>       To: MyFaces Development
>>>>>>>   <dev@myfaces.apache.org>; Mark
>>>>>>>>>     Struberg
>>>>>>>>>>>      <struberg@yahoo.de>
>>>>>>>>>>>>>       Cc:
>>>>>>>>>>>>>       Sent: Thursday, November 
> 15, 2012
>>>  8:44 PM
>>>>>>>>>>>>>       Subject: Re: StateSaving 
> review
>>>>>>>>>>>>> 
>>>>>>>>>>>>>       Hi
>>>>>>>>>>>>> 
>>>>>>>>>>>>>       2012/11/15 Mark Struberg
>>>>>   <struberg@yahoo.de>:
>>>>>>>>>>>>>>        Btw, I still do not 
> see where
>>>  the trick
>>>>>   for
>>>>>>>   storing a
>>>>>>>>>     separate
>>>>>>>>>>>      state list
>>>>>>>>>>>>>       for each browser tab is 
> hidden.
>>>>>>>>>>>>>>        That was the goal of 
> all the
>>>>>   refactoring
>>>>>>>   initially. Where
>>>>>>>>>     is that
>>>>>>>>>>>      done?
>>>>>>>>>>>>>> 
>>>>>>>>>>>>> 
>>>>>>>>>>>>>       Mark, months ago we 
> created
>>>>>   2.1.x-client-window
>>>>>>>   branch:
>>>>>>>>>>>>> 
>>>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>> 
>>>>>>> 
>>>>> 
>>> 
> http://svn.apache.org/repos/asf/myfaces/core/branches/2.1.x-client-window/
>>>>>>>>>>>>> 
>>>>>>>>>>>>>       This branch is the same as 
> 2.1.x,
>>>  but only
>>>>>   with the
>>>>>>>   changes
>>>>>>>>>     proposed
>>>>>>>>>>>>>       long time ago for JSF 2.2 
> client
>>>  window.
>>>>>>>>>>>>> 
>>>>>>>>>>>>>       It has what you need to 
> have a
>>>  state list
>>>>>   for each
>>>>>>>   browser
>>>>>>>>>     tab.
>>>>>>>>>>>>>       The only thing you need to 
> provide
>>>  is a
>>>>>   ClientWindow
>>>>>>>>>     implementation
>>>>>>>>>>>>>       and that's it. The 
> state saving
>>>  code
>>>>>   already has
>>>>>>>   included
>>>>>>>>>     the
>>>>>>>>>>>      client-window
>>>>>>>>>>>>>       concept. I also fixed 
> Flash scope
>>>  to use
>>>>>   client
>>>>>>>   window.
>>>>>>>>>>>>>       It is also available in a 
> maver
>>>  repo:
>>>>>>>>>>>>> 
>>>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>> 
>>>>>>> 
>>>>> 
>>> 
> https://repository.apache.org/content/repositories/snapshots/org/apache/myfaces/core/myfaces-bundle/
>>>>>>>>>>>>> 
>>>>>>>>>>>>>       Let's make a deal. 
> Move your
>>>  changes to
>>>>>   that
>>>>>>>   branch, let
>>>>>>>>>     me apply
>>>>>>>>>>>      my fix
>>>>>>>>>>>>>       in trunk and do a release. 
> When
>>>  your changes
>>>>>   are
>>>>>>>   ready, we can
>>>>>>>>>     discuss
>>>>>>>>>>>>>       them and backport from
>>>  2.1.x-client-window
>>>>>   to trunk.
>>>>>>>   Does that
>>>>>>>>>     sound
>>>>>>>>>>>>>       good for you?
>>>>>>>>>>>>> 
>>>>>>>>>>>>>       regards,
>>>>>>>>>>>>> 
>>>>>>>>>>>>>       Leonardo Uribe
>>>>>>>>>>>>> 
>>>>>>>>>>>>> 
>>>>>>>>>>>>>>        LieGrue,
>>>>>>>>>>>>>>        strub
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>        ----- Original 
> Message -----
>>>>>>>>>>>>>>>        From: Mark 
> Struberg
>>>>>>>   <struberg@yahoo.de>
>>>>>>>>>>>>>>>        To: MyFaces 
> Development
>>>>>>>>>     <dev@myfaces.apache.org>
>>>>>>>>>>>>>>>        Cc:
>>>>>>>>>>>>>>>        Sent: Thursday, 
> November
>>>  15, 2012
>>>>>   6:32 PM
>>>>>>>>>>>>>>>        Subject: Re: 
> StateSaving
>>>  review
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>        I did further 
> reviews:
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>        Currently the
>>>  CounterKeyFactory
>>>>>   needs some
>>>>>>>   random to
>>>>>>>>>     be unique
>>>>>>>>>>>>>       (according to
>>>>>>>>>>>>>>>        Leo) and the
>>>  RandomKeyFactory needs
>>>>>   a
>>>>>>>   counter to be
>>>>>>>>>     unique.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>        Does that ring a 
> bell?
>>>  That stuff
>>>>>   is
>>>>>>>   completely
>>>>>>>>>     useless!
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>        Foooooolks wake 
> up,
>>>  let's
>>>>>   provide ONE
>>>>>>>   factory
>>>>>>>>>     which is
>>>>>>>>>>>      waterproof.
>>>>>>>>>>>>>       That
>>>>>>>>>>>>>>>        would be much 
> better than
>>>  having
>>>>>   15++
>>>>>>>   classes full of
>>>>>>>>>     half
>>>>>>>>>>>      baken/broken
>>>>>>>>>>>>>>>        algorithms.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>        Proposal: Get rid 
> of all
>>>  that
>>>>>   KeyFactory
>>>>>>>   stuff again
>>>>>>>>>     and have
>>>>>>>>>>>      one GOOD
>>>>>>>>>>>>>>>        algorithm: 
> Counter +
>>>  XORShift
>>>>>   Random.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>        The KeyFactory 
> stuff got
>>>  only
>>>>>   introduced in
>>>>>>>   2.1.9 and
>>>>>>>>>     is
>>>>>>>>>>>      crashing in
>>>>>>>>>>>>>       production.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>        LieGrue,
>>>>>>>>>>>>>>>        strub
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>        PS: don't 
> take it
>>>  personal, but
>>>>>   from
>>>>>>>   looking at
>>>>>>>>>     the code I
>>>>>>>>>>>      see
>>>>>>>>>>>>>       clearly what
>>>>>>>>>>>>>>>        happens if 
> someone works
>>>  on a stuff
>>>>>   alone
>>>>>>>   without
>>>>>>>>>     reviews.
>>>>>>>>>>>      This always
>>>>>>>>>>>>>       leads to
>>>>>>>>>>>>>>>        deficits, 
> regardless how
>>>  good one
>>>>>   is.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>        ----- Original 
> Message
>>>  -----
>>>>>>>>>>>>>>>>         From: 
> Leonardo Uribe
>>>>>>>   <lu4242@gmail.com>
>>>>>>>>>>>>>>>>         To: MyFaces
>>>  Development
>>>>>>>>>     <dev@myfaces.apache.org>;
>>>>>>>>>>>      Mark
>>>>>>>>>>>>>       Struberg
>>>>>>>>>>>>>>>       
> <struberg@yahoo.de>
>>>>>>>>>>>>>>>>         Cc:
>>>>>>>>>>>>>>>>         Sent: 
> Thursday,
>>>  November 15,
>>>>>   2012 6:20
>>>>>>>   PM
>>>>>>>>>>>>>>>>         Subject: Re:
>>>  StateSaving
>>>>>   review
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>         Hi
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>         2012/11/15 
> Mark
>>>  Struberg
>>>>>>>>>     <struberg@yahoo.de>:
>>>>>>>>>>>>>>>>>          I'm
>>>  currently
>>>>>   reviewing all
>>>>>>>   this area.
>>>>>>>>>     It seems
>>>>>>>>>>>      that we
>>>>>>>>>>>>>       have quite
>>>>>>>>>>>>>>>        some
>>>>>>>>>>>>>>>>         stuff to 
> improve.
>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>>          a.) 
> just a gut
>>>  feeling
>>>>>   yet, but my
>>>>>>>   tummy
>>>>>>>>>     tells me
>>>>>>>>>>>      that we
>>>>>>>>>>>>>       have to
>>>>>>>>>>>>>>>        review
>>>>>>>>>>>>>>>>         our key
>>>  generator/storage
>>>>>   strategies.
>>>>>>>   Too
>>>>>>>>>     complicated or
>>>>>>>>>>>      too badly
>>>>>>>>>>>>>>>        documented.
>>>>>>>>>>>>>>>>         At least 
> they are not
>>>  self
>>>>>   describing.
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>         It's a 
> complex
>>>  algorithm.
>>>>>   No
>>>>>>>   documentation
>>>>>>>>>     so far,
>>>>>>>>>>>      because it
>>>>>>>>>>>>>       is still
>>>>>>>>>>>>>>>>         work in 
> progress.
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>>          b.) 
> candidate 1:
>>>>>>>   CounterKeyFactory. If we
>>>>>>>>>     like to
>>>>>>>>>>>      prevent
>>>>>>>>>>>>>       reboot
>>>>>>>>>>>>>>>        clashes
>>>>>>>>>>>>>>>>         then we 
> might add
>>>  another int
>>>>>   which
>>>>>>>   contains a
>>>>>>>>>     random
>>>>>>>>>>>      value. Think
>>>>>>>>>>>>>       about
>>>>>>>>>>>>>>>        having
>>>>>>>>>>>>>>>>         a Server 
> with a
>>>  single page
>>>>>   right now.
>>>>>>>   Click on
>>>>>>>>>     it a few
>>>>>>>>>>>      times,
>>>>>>>>>>>>>       then
>>>>>>>>>>>>>>>        restart
>>>>>>>>>>>>>>>>         myfaces and 
> issue a
>>>  few
>>>>>   requests to the
>>>>>>>   same
>>>>>>>>>     page and go
>>>>>>>>>>>      back in
>>>>>>>>>>>>>       your
>>>>>>>>>>>>>>>        browser
>>>>>>>>>>>>>>>>         history. 
> Proposal:
>>>  instead of
>>>>>   the
>>>>>>>   viewId we
>>>>>>>>>     should add a
>>>>>>>>>>>      random
>>>>>>>>>>>>>       number.
>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>         Since the 
> counter is
>>>  also
>>>>>   stored into
>>>>>>>   session,
>>>>>>>>>     the last
>>>>>>>>>>>      counter
>>>>>>>>>>>>>       values
>>>>>>>>>>>>>>>>         is not lost.
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>>          c.) a 
> general
>>>  one. We
>>>>>   might
>>>>>>>   introduce an
>>>>>>>>>     own Random
>>>>>>>>>>>      which
>>>>>>>>>>>>>       either uses
>>>>>>>>>>>>>>>> 
>>>>>   java.util.concurrent.ThreadLocalRandom
>>>>>>>   for java
>>>>>>>>>     7++ or
>>>>>>>>>>>      the old
>>>>>>>>>>>>>       Random impl.
>>>>>>>>>>>>>>>>> 
>>>  ThreadLocalRandom has a
>>>>>   _much_
>>>>>>>   better
>>>>>>>>>     performance
>>>>>>>>>>>      on
>>>>>>>>>>>>>       servers! Or we
>>>>>>>>>>>>>>>        just
>>>>>>>>>>>>>>>>         use a simple 
> XORShift
>>>  which is
>>>>>   surely
>>>>>>>   good
>>>>>>>>>     enough for
>>>>>>>>>>>      most cases
>>>>>>>>>>>>>       and
>>>>>>>>>>>>>>>        performs
>>>>>>>>>>>>>>>>         like hell. 
> The
>>>  spreading of
>>>>>   XORShift is
>>>>>>>   better
>>>>>>>>>     than the
>>>>>>>>>>>      standard
>>>>>>>>>>>>>       Java
>>>>>>>>>>>>>>>        algorithm
>>>>>>>>>>>>>>>>         even ...
>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>         It could 
> work.
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>>          d.) 
> KeyFactory
>>>  looks a
>>>>>   bit
>>>>>>>   overengineered.
>>>>>>>>>     The
>>>>>>>>>>>      return type is
>>>>>>>>>>>>>       either
>>>>>>>>>>>>>>>>         Integer or 
> byte []
>>>  but the
>>>>>   encoded
>>>>>>>   value is
>>>>>>>>>     always
>>>>>>>>>>>      represented as
>>>>>>>>>>>>>       String.
>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>         The key is 
> encrypted
>>>  and
>>>>>   base64
>>>>>>>   encoded, but
>>>>>>>>>     that's
>>>>>>>>>>>      done in
>>>>>>>>>>>>>>>> 
>>>>>>>   HtmlResponseStateManager.writeViewStateField.
>>>>>>>>>     The change
>>>>>>>>>>>      only has
>>>>>>>>>>>>>>>>         sense if we 
> move the
>>>>>   responsibility of
>>>>>>>   encrypt
>>>>>>>>>     to
>>>>>>>>>>>>>>>> 
>>>>>>>>>     
> ServerSideStateCacheImpl/ClientSideStateCacheImpl. My
>>>>>>>>>>>      opinion is
>>>>>>>>>>>>>       it is
>>>>>>>>>>>>>>>>         better to 
> keep it as
>>>  is.
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>>          e.) 
> Instead of
>>>  trashing
>>>>>   the
>>>>>>>   Session with
>>>>>>>>>>>      setAttribute and
>>>>>>>>>>>>>       synchronized
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>         blocks we 
> should
>>>  rather store
>>>>>   an
>>>>>>>   AtomicInteger.
>>>>>>>>>     This is
>>>>>>>>>>>      perfectly
>>>>>>>>>>>>>       fine now
>>>>>>>>>>>>>>>        as we
>>>>>>>>>>>>>>>>         do not 
> support java
>>>  1.4 any
>>>>>   longer,
>>>>>>>   right?
>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>         The 
> synchronized
>>>  blocks are
>>>>>   over the
>>>>>>>>>>>      SerializedViewCollection,
>>>>>>>>>>>>>       which
>>>>>>>>>>>>>>>>         is unique 
> per
>>>  session, so it
>>>>>   does not
>>>>>>>   suppose
>>>>>>>>>     any
>>>>>>>>>>>      overhead (tested
>>>>>>>>>>>>>>>>         with YourKit
>>>  profiler). But
>>>>>   performance
>>>>>>>   tests
>>>>>>>>>     shows that
>>>>>>>>>>>      the
>>>>>>>>>>>>>>>>         additional 
> calls to
>>>  session
>>>>>   map impose
>>>>>>>   an small
>>>>>>>>>     overhead
>>>>>>>>>>>      (the
>>>>>>>>>>>>>       ideal is
>>>>>>>>>>>>>>>>         minimize 
> those
>>>  calls). Since
>>>>>   the
>>>>>>>   counter is
>>>>>>>>>     stored per
>>>>>>>>>>>      session,
>>>>>>>>>>>>>       there
>>>>>>>>>>>>>>>>         is no chance 
> of key
>>>  clashing.
>>>>>   One
>>>>>>>   option could
>>>>>>>>>     be
>>>>>>>>>>>      something like
>>>>>>>>>>>>>       the
>>>>>>>>>>>>>>>>         trick used 
> in Flash
>>>  Scope. An
>>>>>>>   AtomicLong from a
>>>>>>>>>     random
>>>>>>>>>>>      seed, but
>>>>>>>>>>>>>>>>         anyway it is
>>>  necessary to
>>>>>   change the
>>>>>>>   algorithm
>>>>>>>>>     for check
>>>>>>>>>>>      if there
>>>>>>>>>>>>>       is a
>>>>>>>>>>>>>>>>         key 
> clashing,
>>>  generates
>>>>>   another key.
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>>          Just a 
> few
>>>  random
>>>>>   ideas...
>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>         MS>> 
> another
>>>  one:
>>>>>>>>>>>>>>>>         MS>> 
> All that
>>>  stuff has
>>>>>   nothing
>>>>>>>   to do with
>>>>>>>>>     the
>>>>>>>>>>>      RenderKit and
>>>>>>>>>>>>>       should
>>>>>>>>>>>>>>>        go
>>>>>>>>>>>>>>>>         into
>>>>>>>   org.apache.myfaces.application.viewstate.
>>>>>>>>>>>>>>>>         MS>> 
> wdyt?
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>         That stuff 
> is used by
>>>>>>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>   org.apache.myfaces.renderkit.html.HtmlResponseStateManager, so
>>>>>>>   in
>>>>>>>>>>>>>>>>         theory is 
> related to
>>>  the
>>>>>   renderkit
>>>>>>>   (because
>>>>>>>>>>>      ResponseStateManager
>>>>>>>>>>>>>       is
>>>>>>>>>>>>>>>>         renderkit 
> dependant),
>>>  but it
>>>>>   is an
>>>>>>>   application
>>>>>>>>>     scope
>>>>>>>>>>>      concept.
>>>>>>>>>>>>>       Maybe
>>>>>>>>>>>>>>>> 
>>>>>>>>>     
> org.apache.myfaces.renderkit.application.viewstate.
>>>  has
>>>>>>>>>>>      more sense
>>>>>>>>>>>>>       to
>>>>>>>>>>>>>>>>         me.
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>         MS>> 
> Also the
>>>  following
>>>>>   classes
>>>>>>>   are
>>>>>>>>>     related and
>>>>>>>>>>>      should imo
>>>>>>>>>>>>>       get moved
>>>>>>>>>>>>>>>>         to this new 
> package:
>>>>>>>>>>>>>>>>         MS>> *
>>>  StateCache
>>>>>>>>>>>>>>>>         MS>> *
>>>  StateCacheFactory
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>         Yes, a 
> better name
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>         MS>> *
>>>  StateManagerImpl
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>         No, 
> StateManagerImpl
>>>  deals
>>>>>   with the
>>>>>>>   logic
>>>>>>>>>     related to
>>>>>>>>>>>      calculate the
>>>>>>>>>>>>>>>>         state to a 
> view,
>>>  StateCache
>>>>>   deals with
>>>>>>>   the
>>>>>>>>>     storage of the
>>>>>>>>>>>      view
>>>>>>>>>>>>>       (maybe
>>>>>>>>>>>>>>>>         a better 
> name is
>>>  StateStorage,
>>>>>   for
>>>>>>>   example
>>>>>>>>>>>>>>>> 
>>>>>>>>> 
>>>  ServerSideStateStorageImpl/ClientSideStateStorageImpl?
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>         regards,
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>         Leonardo
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>>         
> LieGrue,
>>>>>>>>>>>>>>>>>          strub
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>> 
>>>>>>> 
>>>>> 
>>> 
> 

Mime
View raw message