On 10/30/12 2:00 PM, "Chris Geer" <[email protected]> wrote:
>On Tue, Oct 30, 2012 at 10:55 AM, Chris Geer <[email protected]> >wrote: > >> On Tue, Oct 30, 2012 at 9:48 AM, Ate Douma <[email protected]> wrote: >> >>> Another issue I also experienced but just now properly verified is that >>> rendering a page (like canonical home page) since the merge has become >>> excruciating slow. >>> >>> I haven't spend any time yet investigating why this is, but I just >>> compared rendering canonical page 1 with the current trunk against an >>>svn >>> snapshot of just before the merge. Result: ~ 40 sec. vs. 1 sec. >>> >> >> I'm not having this problem. The main screen does take 3-4 seconds to >>load >> which isn't great but it's not performance like you are seeing. The >>first >> time you log into the app after a DB wipe does take a little while but >>I've >> always seen that. >> >> I think I know what's going on but I need some more time to track it >>down. >> For some reason RegionWidgetTag.doStartTag() is being called several >> hundred times on each page load, which now calls WidgetRepository.get() >> each time, which is relatively slow (instead of just pulling it from the >> regionwidget object). Let me debug a little and see what I can do. >> >>> >>> To clarify, doStartTag call DefaultRenderService.render which now calls >WidgetRepository.get(). End result is still way too many calls to >doStartTag/Render Is this a merge issue? I don't seem to remember having these issues in the branchŠ > > >> It seems to me something must be completely wrong here and right now the >>> portal isn't practically usable anymore. >>> >>> This is worrisome because I think the model-split is an important step >>> and amounts to a lot of work by Chris and Matt. I truly hope this is >>>just a >>> simple configuration issue and nothing fundamental. >>> Anyone else having a similar experience? >>> >>> Note: scheduled for tomorrow is the (start of the) release Rave 0.17, >>> and next week is ApacheCon EU, where both Matt and myself will have a >>> Rave presentation and demo. So I'd like to have a reliable release at >>>least >>> by then. >>> >>> Ate >>> >>> >>> On 10/30/2012 05:11 PM, Ate Douma wrote: >>> >>>> On 10/29/2012 05:11 AM, Chris Geer wrote: >>>> >>>>> Branch is now merged into trunk. The only change I had to make was to >>>>> the OmdlOutputAdapter class where I had to comment out the @Component >>>>> annotation. This was because that class now needed a constructor that >>>>> took >>>>> several services to function and @Autowire wasn't working due to >>>>>order >>>>> of >>>>> creation issues I believe. The only place I could find it was used in >>>>> the >>>>> code was a place it was manually constructed so I think we are fine >>>>>but >>>>> I >>>>> may have missed something. I ran all the unit tests, integration >>>>>tests >>>>> and >>>>> also spot checked a lot of the capabilities. >>>>> >>>> >>>> Hi Chris, >>>> >>>> While trying to reconcile these model-split changes against trunk with >>>> the >>>> content-services sandbox, I encountered a few issues, all related to >>>>the >>>> RegionWidget - Widget separation. >>>> One minor issue is I think a simple merge error but others aspects are >>>> more >>>> structural. >>>> >>>> First of all, currently Rave breaks when accessed from a mobile >>>>device: >>>> mobile >>>> rendering now throws a JSP rendering exception because the >>>> <rave:render-widget/> >>>> tag has an extra and required widget parameter which hasn't been set >>>> (mobile.jsp >>>> #78). >>>> >>>> Another issue is that the Profile page no longer renders (static) >>>>widgets >>>> because the ProfileController doesn't derive and saves the list of >>>> widgets in >>>> the page to the model (as PageController does). I think you did have >>>> this in the >>>> branch but it got lost during the merge back into trunk. >>>> >>>> This functionality, retrieving and deriving the widgets from the page >>>> region(s) >>>> RegionWidgets and storing them as a list on the model so they are >>>> available >>>> during rendering, I'm not very happy with. Especially not when it is >>>> stored as a >>>> list. A map (WidgetId->Widget) might be better already so we don't >>>>need >>>> to >>>> iterate on the whole list to find back a matching single widget, as >>>>for >>>> example >>>> now done in region.tag. >>>> But maybe an even cleaner alternative would be retrieving a Widget 'on >>>> demand' >>>> through a custom tag itself, similar to the PersonTag does? >>>> And tags as RegionWidgetTag then can do this internally and then >>>> wouldn't need >>>> the extra widget parameter. >>>> >>>> Thoughts? >>>> >>>> >>>>> After integration I did find one bug >>>>> >>>>>RAVE-837<https://issues.**apache.org/jira/browse/RAVE-**837<https://is >>>>>sues.apache.org/jira/browse/RAVE-837>> >>>>> that >>>>> I will try and fix in the next day or so. Please file tickets for any >>>>> other >>>>> bugs and we'll squash them. >>>>> >>>>> Just remember now, all the IDs in the interfaces are Strings now, not >>>>> Longs. Longs are still used in the JPA implementation and converted >>>>>to >>>>> strings at the interface level. >>>>> >>>>> @Jasha - I fixed the category problem prior to merge. >>>>> >>>>> Chris >>>>> >>>>> On Fri, Oct 26, 2012 at 4:30 AM, Jasha Joachimsthal <[email protected] >>>>> >wrote: >>>>> >>>>> Hi Chris, >>>>>> >>>>>> I did a few tests and found the following things: >>>>>> >>>>>> Widget Store shows a log entry about tags: >>>>>> [INFO] [talledLocalContainer] 465793 ravePersistenceUnit WARN >>>>>> [http-8080-1] openjpa.Runtime - Supplied user parameters "[widgetId, >>>>>> tagId]" do not match expected parameters "[widgetId]" for the >>>>>>prepared >>>>>> query "PreparedQuery: [select t from JpaWidgetTag t where >>>>>> t.widgetId=:widgetId and t.tagId=:tagId] --> [SELECT t0.entity_id, >>>>>> t0.created_date, t0.tag_id, t0.user_id, t0.widget_id FROM >>>>>>widget_tag t0 >>>>>> WHERE (t0.widget_id = ? AND t0.tag_id IS NULL)]". >>>>>> >>>>>> In the Widget store if you select a category and then select the >>>>>>empty >>>>>> category you get an error page: >>>>>> URL: >>>>>> >>>>>> http://localhost:8080/portal/**app/store/category?categoryId=** >>>>>> >>>>>>0&referringPageId=1(logged<http://localhost:8080/portal/app/store/cat >>>>>>egory?categoryId=0&referringPageId=1(logged> >>>>>> >>>>>> in as canonical) >>>>>> [WARNING] [talledLocalContainer] java.lang.NullPointerException >>>>>> [WARNING] [talledLocalContainer] at >>>>>> >>>>>> org.apache.rave.portal.**service.impl.**DefaultWidgetService.** >>>>>> getWidgetsByCategory(**DefaultWidgetService.java:181) >>>>>> >>>>>> >>>>>> The integration tests passed except the last one (OpenID login), but >>>>>> that >>>>>> one has been fixed in the trunk. >>>>>> No real blockers IMO. >>>>>> >>>>>> Jasha >>>>>> >>>>>> >>>>>> On 25 October 2012 19:12, Chris Geer <[email protected]> wrote: >>>>>> >>>>>> All, I will be merging this over the weekend if there are no >>>>>>> objections. >>>>>>> (Sunday should be 72 hours). At that time I will do the final >>>>>>>reverse >>>>>>> >>>>>> merge >>>>>> >>>>>>> into the branch and merge into trunk. >>>>>>> >>>>>>> Chris >>>>>>> >>>>>>> On Fri, Oct 12, 2012 at 5:29 PM, Chris Geer <[email protected]> >>>>>>> >>>>>> wrote: >>>>>> >>>>>>> >>>>>>> I think we are at a point where the model-split work is far enough >>>>>>>> >>>>>>> along >>>>>> >>>>>>> that we should consider merging it soon. I need to finish up >>>>>>>> >>>>>>> refactoring >>>>>> >>>>>>> WidgetRating but after that there will be enough critical mass to >>>>>>>> merge >>>>>>>> >>>>>>> it >>>>>>> >>>>>>>> into the baseline IMHO. Prior to that happening it would be good >>>>>>>>if >>>>>>>> we >>>>>>>> >>>>>>> can >>>>>>> >>>>>>>> get several people to spend a little time and review the changes >>>>>>>>that >>>>>>>> >>>>>>> have >>>>>>> >>>>>>>> been made. Any volunteers? >>>>>>>> >>>>>>>> The major changes are: >>>>>>>> 1) All IDs in the interfaces have been changed from int to String. >>>>>>>> This >>>>>>>> >>>>>>> is >>>>>>> >>>>>>>> to support a more flexible ID structure in the future with >>>>>>>>backends >>>>>>>> >>>>>>> other >>>>>> >>>>>>> than JPA >>>>>>>> 2) The model has been split into several related chunks. The major >>>>>>>> >>>>>>> chunks >>>>>> >>>>>>> are Users/People, Widget and other right now. Object relationships >>>>>>>> >>>>>>> between >>>>>>> >>>>>>>> those groups have been broken and replaced with IDs. This is to >>>>>>>> >>>>>>> support a >>>>>> >>>>>>> more modular backend eventually. Right now they are all still part >>>>>>>of >>>>>>>> >>>>>>> the >>>>>> >>>>>>> same JPA persistance unit but that is short term. >>>>>>>> 3) The widget model has been changed to make Widget the top level >>>>>>>> >>>>>>> object >>>>>> >>>>>>> and WidgetComment, WidgetTag & WidgetRating are subordinate >>>>>>>objects. >>>>>>>> >>>>>>> These >>>>>>> >>>>>>>> changes include removing the widgetID attribute from the >>>>>>>>subordinate >>>>>>>> >>>>>>> object >>>>>>> >>>>>>>> so that they are associated with the Widget they are attached to. >>>>>>>>We >>>>>>>> >>>>>>> also >>>>>> >>>>>>> consolidated the various services and repositories into the >>>>>>>> WidgetService/Repository since acting on the subordinate >>>>>>>> object independent of the widget isn't ideal. >>>>>>>> 4) The link between RegionWidget and Widget has been replaced with >>>>>>>> WidgetID. This has required that we populate the JSP attributes >>>>>>>>with >>>>>>>> >>>>>>> the >>>>>> >>>>>>> list of widgets on a page (in the controller). >>>>>>>> >>>>>>>> There are plenty of other changes we could make so if there are >>>>>>>>any >>>>>>>> you >>>>>>>> think are crucial before the merge please point those out but I'm >>>>>>>> >>>>>>> hoping >>>>>> >>>>>>> we >>>>>>> >>>>>>>> can get the major changes into trunk and then iterate through the >>>>>>>> rest. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Chris >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >>
