Thanks Gurkan. I don't know that we have stuff that is deprecated pending removal at the moment. In terms of removing the CMP/BMP stuff... well, people are using it, which is why I'm working on it :-). I would be ok with marking it as deprecated, as long as we print out an explicit warning if your application is using it, so you know to migrate. In terms of the gain... I don't know. There'd be less code, but I suspect still the same dependencies, so we'd be removing a small part of openejb-core effectively. I think its a good discussion, but I'd prefer to see graceful deprecation with clear warnings before removal.
I'm sure adding comments to the code will be helpful, particularly for new contributors - thank you. We're currently on a CTR policy. It is perfectly ok to commit, and then review. I personally prefer to commit stuff to my fork and discuss a diff here before its merged. Even though I haven't had many replies on this thread, just writing about what I'm running into helps. For folks that aren't committers, the usual contribution methods are fine - GitHub PRs, diffs attached to JIRAs, whatever works. The nice thing about PRs is that your name is still attached to the commit when its merged, so you forever get credit in the git log. Applying diffs from tickets works, and we'll give credit of course, but the author in log will be whoever applies the diff. In other news, I think I have fix for my test. The bad news is that my Mac chose that moment to blow fuses in 4 different chargers and needs to be repaired, so I'm mid-swapping-over machines. I should get that pushed tomorrow morning all being well. Jon On Tue, Dec 4, 2018 at 7:14 PM Gurkan Erdogdu <cgerdo...@gmail.com> wrote: > Great job John. In the mean time, > What do you think to trim old parts of TomEE which includes > > - Removing ORB support > - Removing CMP/BMP support > - Some other non-common or deprecated parts? > > If we want to have a maintainable TomEE codebase, instead of adding more > features, we need to have more reliable/readable/lightweight code base. I > will start to add comments/explanations to the methods, remove duplicated > parts etc. > > Also, is it possible to return back to commit and then review policy? We > can have some strict rules to correlate the commit with the JIRA issue. If > the commit will have a big impact in the current codebase, before working > on the feature, we can discuss it first and starts to work if the community > all agree. Otherwise, it includes lots of commits within one shot that hard > to understand the content. I think that community consensus is necessary > before adding big changes/updates to the current codebase. > > WDYT? Comments/advises are so welcome! > > Regards. > Gurkan > > > On Tue, Dec 4, 2018 at 8:46 PM Jonathan Gallimore < > jonathan.gallim...@gmail.com> wrote: > > > My issue seems more basic than CMP actually - a straight session bean > with > > a business method that does a no-op is also failing using a Local and > > LocalHome interface throws this exception. I stripped the test right down > > to the basics, and also tried it in a sample. Not sure what I'm not > seeing > > at this point. I'll keep you posted if I find anything. > > > > Jon > > > > On Tue, Dec 4, 2018 at 4:41 PM Jonathan Gallimore < > > jonathan.gallim...@gmail.com> wrote: > > > > > I'm kinda enjoying the challenge. And yes, I'll definitely be kicking > out > > > some docs and better examples for this. :) > > > > > > Jon > > > > > > On Tue, Dec 4, 2018 at 4:38 PM David Blevins <david.blev...@gmail.com> > > > wrote: > > > > > >> In the bike shed vs nuclear reactor analogy of open source, you're > > >> working on a nuclear reactor and therefore not getting much > > participation. > > >> This particular code is super hard and the guy who wrote it was Dain > > >> Sundstrom, who went on to create Presto a 300 pedabyte data warehouse > > >> Facebook runs on. This is also the only CMP implementation that runs > on > > >> JPA. > > >> > > >> Thank you for working on it. > > >> > > >> We should document it incredibly, because it *is* a "nuclear reactor" > > and > > >> few people can work on it. I'm aware of the high level design, but > this > > one > > >> should point at actual code and say "look here and here for the > critical > > >> things. If you have issues, do this and do that." > > >> > > >> > > >> -- > > >> David Blevins > > >> http://twitter.com/dblevins > > >> http://www.tomitribe.com > > >> > > >> > On Dec 4, 2018, at 3:48 AM, Jonathan Gallimore < > > >> jonathan.gallim...@gmail.com> wrote: > > >> > > > >> > I hacked together a little JVMTI agent to help debug this, and I > > think I > > >> > have tracked down where the NPE is - looks to be field is null in > the > > >> > metadata that is handed over to OpenJPA. Looks like I now have > enough > > of > > >> > stacktrace to debug it. When I track down my mistake, I'll let you > > know > > >> > (and no doubt kick myself as well :-) ) > > >> > > > >> > Jon > > >> > > > >> > On Mon, Dec 3, 2018 at 4:55 PM Jonathan Gallimore < > > >> > jonathan.gallim...@gmail.com> wrote: > > >> > > > >> >> I have pushed some further work on this, but I'm now stuck. I have > > >> tried > > >> >> to ensure that this is working ok with CMP beans with a 1-m > > >> relationship, > > >> >> but I am getting a very weird NPE from here: > > >> >> > > >> > > > https://github.com/apache/tomee/pull/222/commits/5d3efd692c4ee3c635d76e5e53b0ff583d692be3#diff-59a18d263fb512ee53c08513f51d2172R58 > > >> >> > > >> >> Weirdly, the business method on MovieBusinessBean works ok: > > >> >> > > >> > > > https://github.com/apache/tomee/pull/222/commits/5d3efd692c4ee3c635d76e5e53b0ff583d692be3#diff-d3d03f1bc557e1eca2bac8afe2a7c86bR56 > > >> . > > >> >> All I can see in terms of the stack trace is an NPE thrown inside > the > > >> >> proxy, called from line 58 of MoviesServlet. Only started happening > > >> when I > > >> >> added the addActor method. > > >> >> > > >> >> I'll kick hacking away on it, but any review of the code, any > samples > > >> of > > >> >> CMP code with relationships working, or general debugging tips are > > much > > >> >> appreciated. > > >> >> > > >> >> Jon > > >> >> > > >> >> On Thu, Nov 29, 2018 at 10:23 AM Jonathan Gallimore < > > >> >> jonathan.gallim...@gmail.com> wrote: > > >> >> > > >> >>> This is my work in progress so far: > > >> >>> https://github.com/apache/tomee/pull/222. > > >> >>> > > >> >>> I'd like to incorporate some Arquillian tests today, and ensure > that > > >> this > > >> >>> works with things like relationships between entities. > > >> >>> > > >> >>> The change here is fairly straightforward though; we pick up a > > >> >>> persistence unit called "cmp", if one has been defined, and read > all > > >> the > > >> >>> elements on it. If an entity has been defined in one of those > > mappings > > >> >>> files, we add the entity class to a set, and the CMP/JPA > processing > > >> will > > >> >>> simply ignore it. The persistence provider should then use the > > >> mapping that > > >> >>> has been defined in the mapping file and not generate its own. > > >> >>> > > >> >>> I'll post further updates here, but feedback is welcome. > > >> >>> > > >> >>> Jon > > >> >>> > > >> >>> On Wed, Nov 28, 2018 at 11:50 AM Jonathan Gallimore < > > >> >>> jonathan.gallim...@gmail.com> wrote: > > >> >>> > > >> >>>> Hi > > >> >>>> > > >> >>>> This continues on from the work that Otavio did on this thread: > > >> "Creates > > >> >>>> an unmarshal that does not filter to JavaEE namespace". > > >> >>>> > > >> >>>> For those of you who aren't aware, TomEE supports CMP entity > beans > > >> >>>> (which is quite an old way of doing persistence - it dates back > to > > >> the EJB > > >> >>>> 2.1 era - and possibly earlier, I started using EJB around 2.1), > > and > > >> it > > >> >>>> does this by converting them to JPA entities on the fly and > > creating > > >> an ORM > > >> >>>> mapping file in XML for the persistence provider to use. It uses > a > > >> special > > >> >>>> persistence unit called "cmp". > > >> >>>> > > >> >>>> One of the nice things you can do is use > > >> >>>> the openejb.descriptors.output=true system property, and you'll > get > > >> the > > >> >>>> persistence.xml and orm.xml file that was generated at deploy > time. > > >> >>>> Unfortunately, if you try and customize the ORM xml, and provide > > >> both the > > >> >>>> persistence xml and ORM xml files in your app, using the > > >> <mapping-file> tag > > >> >>>> in persistence.xml (something like the example below), it doesn't > > >> work, and > > >> >>>> the auto generated mappings are still used instead. > > >> >>>> > > >> >>>> <?xml version="1.0" encoding="UTF-8" standalone="yes"?> > > >> >>>> <persistence xmlns="http://java.sun.com/xml/ns/persistence"; > > >> >>>> version="2.0"> > > >> >>>> <persistence-unit name="cmp" transaction-type="JTA"> > > >> >>>> <jta-data-source>jta-ds</jta-data-source> > > >> >>>> <non-jta-data-source>non-jta-ds</non-jta-data-source> > > >> >>>> > > >> >>>> > <mapping-file>META-INF/openejb-cmp-generated-orm.xml</mapping-file> > > >> >>>> <class>...</class> > > >> >>>> ... > > >> >>>> <exclude-unlisted-classes>true</exclude-unlisted-classes> > > >> >>>> </persistence-unit> > > >> >>>> </persistence> > > >> >>>> > > >> >>>> I've reproduced this in a test, and am working on a fix. Doesn't > > look > > >> >>>> like it'll be too hard. > > >> https://issues.apache.org/jira/browse/TOMEE-2295 > > >> >>>> is the JIRA, and I'll post a PR here for review and discussion > when > > >> I'm > > >> >>>> done. > > >> >>>> > > >> >>>> Cheers > > >> >>>> > > >> >>>> Jon > > >> >>>> > > >> >>> > > >> > > >> > > >
