Hi 2012/11/15 Mark Struberg <[email protected]>: > 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 <[email protected]> >> To: MyFaces Development <[email protected]> >> 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 <[email protected]> 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 <[email protected]> >>>> To: MyFaces Development <[email protected]>; Mark Struberg >> <[email protected]> >>>> Cc: >>>> Sent: Friday, November 16, 2012 12:11 AM >>>> Subject: Re: StateSaving review >>>> >>>> Hi >>>> >>>> 2012/11/15 Mark Struberg <[email protected]>: >>>>> 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 <[email protected]> >>>>>> To: MyFaces Development <[email protected]>; Mark >> Struberg >>>> <[email protected]> >>>>>> Cc: >>>>>> Sent: Thursday, November 15, 2012 11:06 PM >>>>>> Subject: Re: StateSaving review >>>>>> >>>>>> Hi >>>>>> >>>>>> 2012/11/15 Mark Struberg <[email protected]>: >>>>>>> >>>>>>>> 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 <[email protected]> >>>>>>>> To: MyFaces Development >> <[email protected]>; Mark >>>> Struberg >>>>>> <[email protected]> >>>>>>>> Cc: >>>>>>>> Sent: Thursday, November 15, 2012 10:20 PM >>>>>>>> Subject: Re: StateSaving review >>>>>>>> >>>>>>>> Hi >>>>>>>> >>>>>>>> 2012/11/15 Mark Struberg <[email protected]>: >>>>>>>>> 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 >> <[email protected]> >>>>>>>>>> To: MyFaces Development >>>> <[email protected]>; Mark >>>>>> Struberg >>>>>>>> <[email protected]> >>>>>>>>>> Cc: >>>>>>>>>> Sent: Thursday, November 15, 2012 9:30 PM >>>>>>>>>> Subject: Re: StateSaving review >>>>>>>>>> >>>>>>>>>> Hi >>>>>>>>>> >>>>>>>>>> 2012/11/15 Mark Struberg >> <[email protected]>: >>>>>>>>>>> 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 >>>> <[email protected]> >>>>>>>>>>>> To: MyFaces Development >>>>>> <[email protected]>; Mark >>>>>>>> Struberg >>>>>>>>>> <[email protected]> >>>>>>>>>>>> Cc: >>>>>>>>>>>> Sent: Thursday, November 15, 2012 >> 8:44 PM >>>>>>>>>>>> Subject: Re: StateSaving review >>>>>>>>>>>> >>>>>>>>>>>> Hi >>>>>>>>>>>> >>>>>>>>>>>> 2012/11/15 Mark Struberg >>>> <[email protected]>: >>>>>>>>>>>>> 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 >>>>>> <[email protected]> >>>>>>>>>>>>>> To: MyFaces Development >>>>>>>> <[email protected]> >>>>>>>>>>>>>> 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 >>>>>> <[email protected]> >>>>>>>>>>>>>>> To: MyFaces >> Development >>>>>>>> <[email protected]>; >>>>>>>>>> Mark >>>>>>>>>>>> Struberg >>>>>>>>>>>>>> <[email protected]> >>>>>>>>>>>>>>> Cc: >>>>>>>>>>>>>>> Sent: Thursday, >> November 15, >>>> 2012 6:20 >>>>>> PM >>>>>>>>>>>>>>> Subject: Re: >> StateSaving >>>> review >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Hi >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 2012/11/15 Mark >> Struberg >>>>>>>> <[email protected]>: >>>>>>>>>>>>>>>> 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 >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>>> >>>> >>
