From: "Jason T. Greene" <jason.greene(a)redhat.com&gt; To: "Stuart Douglas" <stuart.w.douglas(a)gmail.com&gt; Cc: "JBoss AS7 Development" <jboss-as7-dev(a)lists.jboss.org&gt; Sent: Wednesday, August 24, 2011 3:20:12 PM Subject: Re: [jboss-as7-dev] Moving EJB3 code into the AS7 source tree

On 8/24/11 4:08 PM, Stuart Douglas wrote: > Hi Guys, > > While looking at some EJB3 stuff I began to wonder just how much > stuff > from the JBoss EJB3 maven artefacts we actually use. Even though > there > is a lot of artefacts, it turns out that we use around 75 classes > from > EJB3, and more than half of them are timer service related (It > looks > like a lot of the stuff in the EJB3 package is either legacy stuff > that > is just hanging around). > > Given that the majority of the actual EJB logic / implementation is > in > the AS modules (in the EJB and EE modules), I was wondering if it > would > be worth merging the whole of EJB into AS7 and dumping the EJB3 > aggregator project altogether. > > I have a branch that does this here (although this branch has not > migrated any tests yet): > > https://github.com/stuartwdouglas/jboss-as/tree/remove > > Personally I think this would make it much easier to refactor, and > would > also allow us to remove some adaptor classes between AS7 and EJB3 > (e.g. > AS7 has to sub class the EJB3 interceptors, rather than EJB3 > providing > real jboss invocation interceptors).

I agree 1000% on this. In the past we had already decided to keep the ejb3 impl in the AS tree, so I think we should avoid splitting off components that are purely ejb3 specific and don't have use elsewhere.

I haven't reviewed all of this patch, but one thing that I had noticed earlier is that we have some code that was just copied over and not really in use (e.g. some broken pool implementations). IMO if we aren't using / supporting stuff in AS we should just not carry it over.

-- Jason T. Greene JBoss AS Lead / EAP Platform Architect JBoss, a division of Red Hat _______________________________________________ jboss-as7-dev mailing list jboss-as7-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/jboss-as7-dev

I have updated by branch, I have migrated the tests and moved the ejb classes into org.jboss.as.ejb3. As far as I can see it is ready to merge if we decide to go ahead with this. Stuart On 25/08/2011, at 8:26 AM, Andrig Miller wrote: > > > ------------------------------------------------------------------------ > > *From:*"Jason T. Greene" <jason.greene(a)redhat.com > <mailto:jason.greene@redhat.com>> > *To:*"Stuart Douglas" <stuart.w.douglas(a)gmail.com > <mailto:stuart.w.douglas@gmail.com>> > *Cc:*"JBoss AS7 Development" <jboss-as7-dev(a)lists.jboss.org > <mailto:jboss-as7-dev@lists.jboss.org>> > *Sent:*Wednesday, August 24, 2011 3:20:12 PM > *Subject:*Re: [jboss-as7-dev] Moving EJB3 code into the AS7 > source tree > > On 8/24/11 4:08 PM, Stuart Douglas wrote: > > Hi Guys, > > > > While looking at some EJB3 stuff I began to wonder just how much > stuff > > from the JBoss EJB3 maven artefacts we actually use. Even though > there > > is a lot of artefacts, it turns out that we use around 75 > classes from > > EJB3, and more than half of them are timer service related (It looks > > like a lot of the stuff in the EJB3 package is either legacy > stuff that > > is just hanging around). > > > > Given that the majority of the actual EJB logic / implementation > is in > > the AS modules (in the EJB and EE modules), I was wondering if > it would > > be worth merging the whole of EJB into AS7 and dumping the EJB3 > > aggregator project altogether. > > > > I have a branch that does this here (although this branch has not > > migrated any tests yet): > > > > https://github.com/stuartwdouglas/jboss-as/tree/remove > > > > Personally I think this would make it much easier to refactor, > and would > > also allow us to remove some adaptor classes between AS7 and > EJB3 (e.g. > > AS7 has to sub class the EJB3 interceptors, rather than EJB3 > providing > > real jboss invocation interceptors). > > I agree 1000% on this. In the past we had already decided to keep the > ejb3 impl in the AS tree, so I think we should avoid splitting off > components that are purely ejb3 specific and don't have use > elsewhere. > > I haven't reviewed all of this patch, but one thing that I had > noticed > earlier is that we have some code that was just copied over and not > really in use (e.g. some broken pool implementations). IMO if we > aren't > using / supporting stuff in AS we should just not carry it over. > > +1 to both of these. It would make my life easier on the performance > team too. > > > -- > Jason T. Greene > JBoss AS Lead / EAP Platform Architect > JBoss, a division of Red Hat > _______________________________________________ > jboss-as7-dev mailing list > jboss-as7-dev(a)lists.jboss.org <mailto:jboss-as7-dev@lists.jboss.org> > https://lists.jboss.org/mailman/listinfo/jboss-as7-dev > _______________________________________________ jboss-as7-dev mailing list jboss-as7-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/jboss-as7-dev

On 8/25/11 3:09 AM, Carlo de Wolf wrote: > http://community.jboss.org/people/wolfc/blog/2010/11/26/strategies-for-se... > > The only objection I have is putting everything in a single module and > just separate concerns on a package level. There is also some logical separation in that ejb3 is still a separate subsystem, and therefore a separate module. The code is just in the same place and versioned with all other subsystems. I think it's fine to break things into separately versioned external components when we have reuse (or great potential of reuse). The ejb3 timer implementation though is unlikely to be useful to anyone other than our impl. So using package separation for that case seems more than adequate (at least now) > > As I've pointed out a couple of times, in the AS 7 code base we're not > vigilant enough nor does the review process catch design issues. For the > review process to do a proper design check would form an unwanted choke > point. So we still need to find ways to guard the code against design > regression. We certainly could improve here in various areas. We need to make sure we capture designs in wikis or in javadoc or in code comments. This is done in many cases but not others. Ideally we start with an agreed upon requirements doc, then a set of docs detailing the abstract design. Then when code is created we try to reflect the doc information in comments and javadoc. Any significant refactor then can be reviewed against that information. Right now we certainly discuss all major EE refactors, and we validate they introduce no test regressions and no TCK regressions before we merge these kind of patches. As to preventing choke points, I think this has to do with what the patch touches. The burden is ultimately on the person sending in the pull request / patch to break up changes in such a way that they are applied in reasonable time frames. For example simple bug fixes require much less review than a complete re-architecture of EE proxies. -- Jason T. Greene JBoss AS Lead / EAP Platform Architect JBoss, a division of Red Hat

On 08/26/2011 06:58 AM, Stuart Douglas wrote: > I have moved the code and tests over. > > Stuart > > On 26/08/2011, at 3:42 AM, Jason T. Greene wrote: > >> On 8/25/11 3:09 AM, Carlo de Wolf wrote: >>> http://community.jboss.org/people/wolfc/blog/2010/11/26/strategies-for-se... >>> >>> >>> The only objection I have is putting everything in a single module and >>> just separate concerns on a package level. >> There is also some logical separation in that ejb3 is still a >> separate subsystem, and therefore a separate module. The code is >> just in the same place and versioned with all other subsystems. I >> think it's fine to break things into separately versioned external >> components when we have reuse (or great potential of reuse). The >> ejb3 timer implementation though is unlikely to be useful to anyone >> other than our impl. So using package separation for that case seems >> more than adequate (at least now) I wasn't talking about an external component. So based on that false assumption the commit has been approved. I also see that we've lost the history of the source code, while git is capable of (and designed for) slicing and dicing code bases including history. I would rather go for an open minded approach which gives the freedom for re-use, then such a close minded one. But I agree with you that we are incapable of fostering such an environment. >>> As I've pointed out a couple of times, in the AS 7 code base we're not >>> vigilant enough nor does the review process catch design issues. >>> For the >>> review process to do a proper design check would form an unwanted >>> choke >>> point. So we still need to find ways to guard the code against design >>> regression. >> We certainly could improve here in various areas. We need to make >> sure we capture designs in wikis or in javadoc or in code comments. >> This is done in many cases but not others. Ideally we start with an >> agreed upon requirements doc, then a set of docs detailing the >> abstract design. Then when code is created we try to reflect the doc >> information in comments and javadoc. Any significant refactor then >> can be reviewed against that information. >> >> Right now we certainly discuss all major EE refactors, and we >> validate they introduce no test regressions and no TCK regressions >> before we merge these kind of patches. That works on the assumption that tests get written (and get written well). Something I recently encountered showed that we're still lacking coverage: https://github.com/wolfc/jboss-as/commit/658a23d43711e5b1f2dc8a3f7c550636... Note that I'm a guilty party here as well as I did not write a test-case to guard my initial MDB implementation (just a demo). >> As to preventing choke points, I think this has to do with what the >> patch touches. The burden is ultimately on the person sending in the >> pull request / patch to break up changes in such a way that they are >> applied in reasonable time frames. For example simple bug fixes >> require much less review than a complete re-architecture of EE proxies. Yes, pull requests creating a burden on the reviewer. That's the nature of our current review process. Whether a contribution is applied in a timely manner is up to the pushers. To go by example I've got a contribution that is a first stage of on-going work, yet it has been sitting idly for some time now: https://github.com/jbossas/jboss-as/pull/90 Carlo >> -- >> Jason T. Greene >> JBoss AS Lead / EAP Platform Architect >> JBoss, a division of Red Hat

Carlo and I had a chat about this yesterday. For the benefit of those who are following this thread, EJB3 team is *not* opposed to moving that code from a separate repo/project into the AS7 codebase. That's fine. Some of the points that we discussed included the loss of history over that code. I don't know if it's possible to somehow get back the history on those files (now that the upstream already contains these files). But it would be good to have the history since those files (like the tx interceptors) have been around since AS 4.x days and have some good amount of commits linked against JIRAs.

One other point was having finer grained modules like ejb3-tx (within the AS codebase) instead of clubbing everything under the jboss-as/ejb3 module. That way each such module can have the right set of limited module dependencies. Ofcourse, just creating a module for the sake of it wouldn't make sense. So if there is a clear separation for creating a module, then perhaps that would be a good idea. Thoughts?

On 08/30/2011 06:31 PM, Jason T. Greene wrote: > On 8/30/11 3:08 AM, Jaikiran Pai wrote: >> Carlo and I had a chat about this yesterday. For the benefit of those >> who are following this thread, EJB3 team is *not* opposed to moving >> that >> code from a separate repo/project into the AS7 codebase. That's fine. >> >> Some of the points that we discussed included the loss of history over >> that code. I don't know if it's possible to somehow get back the >> history >> on those files (now that the upstream already contains these files). >> But >> it would be good to have the history since those files (like the tx >> interceptors) have been around since AS 4.x days and have some good >> amount of commits linked against JIRAs. > It should be possible using git filter-branch and a limited merge. We > would just replace the files. It's a pretty small number (70). Although > really old history may not be all that useful since the context has > completely changed twice over. The older the history the less chance it is really useful. Mostly it just a couple of commits of that single code block. But that ultimately can lead down to a long time ago. Especially pieces that haven't changed much. This is mostly used during support, to figure out why a piece of code is there. >> One other point was having finer grained modules like ejb3-tx (within >> the AS codebase) instead of clubbing everything under the jboss-as/ejb3 >> module. That way each such module can have the right set of limited >> module dependencies. Ofcourse, just creating a module for the sake >> of it >> wouldn't make sense. So if there is a clear separation for creating a >> module, then perhaps that would be a good idea. Thoughts? > Yeah I think separating JPA, CMP, and EJB3 into separate subsystems is a > good example of this. Anytime we want to make some particular component > optional it makes sense to break it into some kind of sub module. We > have to be careful about having too many subsystems though, or having > incompatible combinations. So for example I could see the ejb3 tx > integration being optional IF the tx subsystem is not active > (interceptors don't get installed or something like that). However, I'm > not sure if it needs to be it's own artifact, it's only a handful of > classes. Maybe it would help if there was a proposal for how the > dependency breakdown would look? > A module becoming it's own artifact is a side-effect of Maven. The real criteria for a separate runtime-artifact is like you said, whether the functionality is enabled in certain configurations or not. What I've seen in the past is that concerns get mixed. So instead of isolating complexity people would hack in a piece of code to make things work. By isolating tx we can make sure it doesn't do anything untoward with other API/SPIs for example. So we can make sure it doesn't abuse jboss-metadata to operate. A similar example would have been annotation via reflection scanning, but we can't isolate out the JDK API in favor of the Jandex API. Carlo

At first glance this test looks like a functional test, but it's so exhaustive that it acts like a stress test. It's ran against every timezone in the system, and in my system I have 608. It shouldn't be a big deal to modify this test to execute in a reasonable time frame (at least by default). During development I sometimes do one-off exhaustive tests like this, but it's not practical to put them in a test suite. For example, I tend to run anything that does bytecode analysis against every class file on my hard drive. There is, of course, nothing wrong with writing exhaustive tests that can be saved and executed manually. Maybe we should have a special testsuite module for this? [NOTE: I would support splitting off generic time math into a separate library. It's something that could be useful to other projects, and it's one of those things that once you get it right, you shouldn't need to update it much]

On 9/1/11 8:51 AM, Jaikiran Pai wrote: > On Thursday 01 September 2011 07:14 PM, Jason T. Greene wrote: > As for moving it to a separate library, it was indeed a separate library > in the timerservice project that we merged recently into AS codebase. I > think it probably makes sense to leave this library out of AS code base > and let it be independent library. I meant generic time manipulation logic, not the entire ejb3 timer service implementation, which encompasses much more than that.

On 9/1/11 9:03 AM, Jaikiran Pai wrote: > On Thursday 01 September 2011 07:29 PM, Jason T. Greene wrote: >> On 9/1/11 8:51 AM, Jaikiran Pai wrote: >>> On Thursday 01 September 2011 07:14 PM, Jason T. Greene wrote: >>> As for moving it to a separate library, it was indeed a separate >>> library >>> in the timerservice project that we merged recently into AS >>> codebase. I >>> think it probably makes sense to leave this library out of AS code >>> base >>> and let it be independent library. >> >> I meant generic time manipulation logic, not the entire ejb3 timer >> service implementation, which encompasses much more than that. >> > Yeah, I meant the same too :) The time manipulation library was a > separate maven project/library. I think it makes sense to let it stay > independent and be outside of AS and let the rest of the timerservice > logic stay in AS. You mean this https://github.com/jbossejb3/jboss-ejb3-timerservice/tree/master/ejb31-ca... ?

That's reasonable. Although I was more thinking of common routines that do date math with Calendar. I mean the hairy part of this is caused by the fact that Calendar is a bad API and so difficult to use, and easy to screw up. Such a module would have potential reusability across other projects.

Another possibility is exploring the idea of using JODA.