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 > 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://issues.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/category?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 >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
