...
[14:15:13 CST(-0600)] <jwennmacher> 
[15:23:51 CST(-0600)] <jwennmacher> EricDalquist: I'd like to get your feedback on something to make sure I'm going in the right direction.
[15:23:56 CST(-0600)] <EricDalquist> ok
[15:24:27 CST(-0600)] <jwennmacher> I'm making the modifications to allow for multiple tabs to be selected in the render tab report form.
[15:25:38 CST(-0600)] <jwennmacher> I think the current design doesn't allow for multiple tabs to be selected and included in the query.
[15:26:06 CST(-0600)] <EricDalquist> ah the search api?
[15:27:58 CST(-0600)] <jwennmacher> Yes. What I'm thinking is I need to modify the BaseStatsticsReportController method createAggregationsQueryKey to return a Set<K> instead of just a <K> and iterate through the sets, or I could modify TabRenderAggregationKey to have a Set<AggreatedTabMapping> instead of just AggregatedTabMapping.
[15:28:13 CST(-0600)] <jwennmacher> I think the former solution makes more sense. Thoughts?
[15:28:59 CST(-0600)] <jwennmacher> This is a bit out of context so let me know what doesn't make sense.
[15:29:02 CST(-0600)] <EricDalquist> hrm
[15:29:04 CST(-0600)] <EricDalquist> ok
[15:29:08 CST(-0600)] <EricDalquist> looking at this now ...
[15:30:09 CST(-0600)] <jwennmacher> But basically yes, we need to pass multiple tab names into baseAggregationDao.getAggregations. The key as is (a single tabname-group-interval) doesn't work for multiple tabnames (or portlet names or whatever)
[15:30:46 CST(-0600)] <EricDalquist> ah right
[15:30:57 CST(-0600)] <EricDalquist> and BaseStatisticsReportController has native support for multi-group queries
[15:30:59 CST(-0600)] <jwennmacher> JpaBaseAggregationDao.getAggretations has an invocation to bindAggregationSpecificKeyParameters(query, key) which is how that 'extra' dimension would be handled
[15:31:21 CST(-0600)] <jwennmacher> yes, but not multiple whatever-extra-thing queries
[15:31:24 CST(-0600)] <EricDalquist> yeah
[15:31:31 CST(-0600)] <EricDalquist> give me 5 minutes to poke around the API
[15:31:39 CST(-0600)] <jwennmacher> OK
[15:35:22 CST(-0600)] <EricDalquist> ok ... so my thoughts I think line up with yours
[15:35:35 CST(-0600)] <EricDalquist> change createAggregationsQueryKey to return a Set<K>
[15:35:53 CST(-0600)] <EricDalquist> and change BaseAggregationDao
[15:35:54 CST(-0600)] <EricDalquist> getAggregations(DateTime start, DateTime end, K key, AggregatedGroupMapping... aggregatedGroupMappings)
[15:35:57 CST(-0600)] <EricDalquist> to
[15:36:02 CST(-0600)] <EricDalquist> getAggregations(DateTime start, DateTime end, Set<K> key, AggregatedGroupMapping... aggregatedGroupMappings)
[15:36:03 CST(-0600)] <jwennmacher> Adjustment to my suggested solution. Have BaseStatsticsReportController method createAggregationsQueryKey return a Set<K>, and pass that Set<K> into a JpaBaseAggregationDao method that accepts a set<K> instead
[15:36:09 CST(-0600)] <jwennmacher> Yep!!!
[15:36:16 CST(-0600)] <EricDalquist> perhaps add that as an overloaded of the existing getAggregations
[15:36:41 CST(-0600)] <EricDalquist> then you'll have to tweak the various impls of BaseAggregationDao to collate that Set<K> into a single query
[15:36:44 CST(-0600)] <EricDalquist> which shouldn't be too bad
[15:37:21 CST(-0600)] <jwennmacher> Nope. Cool.
[15:37:32 CST(-0600)] <jwennmacher> Thanks for the collaboration.
[15:37:36 CST(-0600)] <EricDalquist> no problem
[15:37:41 CST(-0600)] <EricDalquist> always happy to talk about that stuff
[15:43:46 CST(-0600)] <jwennmacher> BTW one other question. I haven't thought through this all the way but I wanted to get your initial thoughts on aspectj vs. aop proxies. What brought this to mind is I had to make a minor change to JpaAggregatedTabLookupDao. The public method getMappedTabForLayoutId invoked by a controller did not have the @OpenEntityManager annotation but the internal public method it invoked did. The entity manager wasn't created because
[15:44:32 CST(-0600)] <jwennmacher> If using compile-time or run-time weaving, this type of issue of course wouldn't occur.
[15:45:11 CST(-0600)] <EricDalquist> not sure I follow
[15:45:25 CST(-0600)] <EricDalquist> JpaAggregatedTabLookupDao.getTabMapping has the @OpenEntityManager annotation
[15:45:40 CST(-0600)] <EricDalquist> ah right
[15:45:41 CST(-0600)] <EricDalquist> I see
[15:45:47 CST(-0600)] <EricDalquist> ignore my previous statement
[15:46:08 CST(-0600)] <EricDalquist> yeah, I have nothing against compile-time weaving if someone (hint) wants to look into it
[15:46:14 CST(-0600)] <jwennmacher> Yes. The controller was invoking getMappedTabForLayoutId (incorreclty mind you ... I thought the layoutNodeId was the id, but I found out it wasn't and had to implement a get by id method).
[15:46:23 CST(-0600)] <EricDalquist> load-time worries me a little more since then we're adding more complexity to the deployers
[15:46:27 CST(-0600)] <jwennmacher> I like that hint.
[15:46:59 CST(-0600)] <jwennmacher> Yeah I like compile-time, especially on big projects. You don't need anything else happening at startup.
[15:47:10 CST(-0600)] <jwennmacher> Slows things down even more
[15:48:07 CST(-0600)] <jwennmacher> I didn't know if there was discussion about that before or not. I've used it before and I generally find it better than aop proxies because you don't have to think about these types of things.
[15:48:46 CST(-0600)] <EricDalquist> no, its never really been brought up before
[15:48:53 CST(-0600)] <EricDalquist> I've thought abuot it
[15:49:01 CST(-0600)] <EricDalquist> but just never had the motivation to look into it
[15:49:08 CST(-0600)] <EricDalquist> I'd love to see something like that figured out though
[15:49:24 CST(-0600)] <EricDalquist> I suppose in theory runtime performance would be better with compile-time weaving too
[15:49:30 CST(-0600)] <EricDalquist> fewer wrapper layers
[15:49:44 CST(-0600)] <jwennmacher> If there are any downsides you can think of, I'm interested in hearing that. Otherwise, I'll add that as a todo item. I suspect it might be considered low on the priority list though (which is fine)
[15:50:02 CST(-0600)] <EricDalquist> yeah, I don't know enough about it to really know of down sides
[15:50:07 CST(-0600)] <EricDalquist> but I can't think of any right now
[15:50:16 CST(-0600)] <EricDalquist> just ping here when/if you work on it
[15:50:21 CST(-0600)] <EricDalquist> I'd love to see the possibilities
[15:50:31 CST(-0600)] <jwennmacher> Sounds good. Always love to get others insights.