On Tue, Oct 30, 2012 at 11:21 AM, Franklin, Matthew B. <[email protected]>wrote:
> 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Š > I don't know...I don't remember having issues in the branch either but the merges were really nasty since so many changes were made to trunk so it's possible. I'll know more tonight. > > > > > > > >> 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 > >>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > >
