I have created a JIRA issue for this now we have a working candidate. https://issues.apache.org/jira/browse/WICKET-1738
I have left your new synchronization in there, and removed thed session check Stefan Fußenegger wrote: > > I believe you can remove the sessionId check. It was just meant to prove > my expectation that a passed sessionId is always equal to the current > session's id (which I think it did). > > The extra synchronization still makes sense as it avoids that two > concurrent threads add a PageStore for the same pageMapName. This could > cause strange (and hard to reproduce/debug) errors. > > good luck with your testing! > > > > > richardwilko wrote: >> >> Ok, cool, >> >> Initial testing shows that no extra synchronisation is required. It >> works :) >> >> However I had to modify the getHttpSession method, the test to make sure >> the sessionid is the same, that doesn't work clustered as the session id >> contains jvm route (ie machine identifier) so changes from machine to >> machine. The first part of the id is the same (the part before the dot) >> so we could either implement some text splitting or remove it completely. >> >> I also forgot about the other stuff that gets stored in httpsession, ie >> the actual session object, so my simple 4 class config didnt work. I >> just copied the original wicket one into mine, there are some extra >> classes that are now distributed that dont need to be, but this was the >> quickest way to get it working. IClusterable is used everywhere so will >> be pain if we want to sort that out. >> >> After more testing i will post back a merged version of code, but all is >> looking good, >> >> cheers, >> >> Richard >> >> >> >> Stefan Fußenegger wrote: >>> >>> As I said, it's a matter of taste and not worth a discussion. I think I >>> can handle it, if you want to stick with your solution ;) >>> >>> With "complain" I meant: "All changes to clustered objects must happen >>> within the context of a Terracotta transaction. This means that a thread >>> must acquire a clustered lock prior to modifying the state of any >>> clustered objects. If a thread attempts to modify a clustered object >>> outside the context of a terracotta transaction, a runtime exception >>> will be thrown." ( >>> http://www.terracotta.org/confluence/display/docs1/Concept+and+Architecture+Guide#ConceptandArchitectureGuide-Transactions >>> Terracotta Product Documentation ). PageMapStore is clustered after >>> being stored in the session. Therefore, modifications to >>> PageMapStore._pageMaps (as in getPageMap(String)) will cause an >>> exception outside of a transaction. I can't see where the transaction >>> starts, but testing this in a clustered environment will quickly give an >>> answer whether we need further synchronization or not (maybe you already >>> did this, while I only tested without actually using TC clustering). I >>> added the further synchronization if needed >>> >>> btw: getPageStore(...) should probably be renamed as it in fact returns >>> a PageMapStore, not a PageStore, same for getPageMap(...) that returns a >>> PageStore. >>> >>> i think a nice side effect of this new implementation is that wicket >>> components need not be IClusterable anymore. we always test our app in >>> "pure wicket mode" and test it clustered prior to deployment. we always >>> find some objects that are Serializable but not IClusterable, hence >>> causing exceptions with TC. Using serialization of pages, both modes >>> would need Serializable objects only. >>> >>> regards >>> stefan >>> >>> http://www.nabble.com/file/p18335661/OurTerracottaPageStore.java >>> OurTerracottaPageStore.java >>> >>> >>> >>> >>> richardwilko wrote: >>>> >>>> Looks good, >>>> >>>> >>>> Stefan Fußenegger wrote: >>>>> >>>>> - I wouldn't use an extra class just to wrap a HashMap of PageStore. I >>>>> would just put them into the plain session. But finally, this is just >>>>> a matter of taste. I even think that this class lacks proper >>>>> synchronization. Doesn't Terracotta complain about modifying an >>>>> instance outside of a transaction?? >>>>> >>>> >>>> I disagree, I think it makes the code cleaner as all the stuff to do >>>> with creating PageStores (and debug information) is encapsulated in the >>>> class. I don't think that the synchronisation is an issue, im not sure >>>> what you mean about terracotta complaining, if Ari is still watching >>>> this thread then he can probably answer the question. >>>> >>>> It is a pain that you cant get the last element of the list, but your >>>> solution works well, I tweaked it slightly to remove a not needed if >>>> statement (you dont need to check the page id, the subset stuff takes >>>> care of it). >>>> >>>> I'm gonna put this slightly modified class through our test environment >>>> here where i work and throw loads of simulated users at it to see how >>>> it works. >>>> >>>> I have removed the wicket module from my terracotta config as this >>>> currently forces the use of httpsessionstore. Also with our solution >>>> we only need to instrument 4 classes so i'm not using the IClusterable >>>> thing in the config, i'm just declaring the classes manually. >>>> I would be good to find out how we go about modifying the wicket tim >>>> (probably another question for Ari there). I downloaded the terracotta >>>> source but couldn't get it to build. >>>> >>>> Richard >>>> >>>> >>>> >>>> >>>> >>>> >>> >>> >> >> > > -- View this message in context: http://www.nabble.com/Terracotta-integration-tp18168616p18338735.html Sent from the Wicket - Dev mailing list archive at Nabble.com.
