So what would be the next steps. Thank you,
Vladimir On Tue, Apr 24, 2012 at 9:52 PM, Vladimir Berezniker <v...@hitechman.com>wrote: > > > On Mon, Apr 23, 2012 at 10:03 AM, Hyrum K Wright < > hyrum.wri...@wandisco.com> wrote: > >> I haven't reviewed the patched, but some comments about the general ideas >> below. >> >> On Sat, Apr 21, 2012 at 10:51 PM, Vladimir Berezniker >> <v...@hitechman.com> wrote: >> > Hi All, >> > >> > I am sending patch series that adds support for subset of SVN Ra >> layer to >> > the JavaHL API. Initial goal was to expose commit editor APIs necessary >> to >> > commit to the remote repositories without local working copy and then >> seek >> > feedback before going further. >> >> Firstly, I applaud you work in this area, and thank you for submitting >> it back upstream. >> >> That being said, a large number of patches submitted in rapid >> succession can sometimes be difficult to digest. While you are >> certainly welcome to take whatever approach you desire for your own >> work, the Subversion community generally appreciates collaborative >> design and development, which is easier when submissions come in small >> chunks. > > > The patches are broken up, so that each contains one logical change. Which > I was hoping would make it easier to review. The first 17 > patches re-factor and add minor enhancements to the existing code. 2 add > actual RA implementation, one updates the build config and the other two > update the test suit. They could have been done as 3 patches but I thought > it would have been harder to review. If there is something I can do to help > people review please let me know. > > >> It also reduces the amount of code which may need to be >> rewritten. >> > > I always expected that based on feedback this might go as far as tossing > out or rewriting majority of what I have written so far. I have no issue > with that. For me it was RA learning exercise and gave me something > concrete to present as a starting point. The reason I chose editor API, is > that it required dealing with life cycle of multiple objects across method > calls the issue that other methods did not present. > > >> >> If it looks like this is going to be a long-term effort to get ready >> for a potential release, we could certainly look at giving you a >> branch in the repository, so that others can comment on your progress >> as it happens. > > > I am flexible, if that would make it the easiest for everyone, that would > work for me. > > I think it would be good idea if we establish milestones to be reached. > From my perspective they are: > * Learn RA API through implementing RA editor driver APIs (done) > * Submit the above for feedback (done) > * Receive and Incorporate feedback (next) > * Implement APIs necessary for reading state and content of repository > * <other milestones based on agreed additional requirements> > * <milestones necessary for release> > > As it turns out, there is already a javahl-ra branch, >> but it doesn't have a very wide coverage of the RA APIs. If it works >> better for you, I'm happy to remove that branch, and let you start on >> a fresh one anew. >> > > I have reviewed the branch that you mention and I am happy to continue > work on the existing branch incorporating my changes into it. I do have > couple of points for discussion: > > 1. Naming. I have used variations SVNRa for the C++ and java classes > related to the svn_ra_session_t. I realize that A should have been > capital too, but SVNRA looks too much like a java constant. In the > existing branch SVNReposAccess is used. If you have any preference for any > name please let me know, and I will use that, otherwise I will stick to > SVNRa. > > 2. In my approach object creation is done in the C++ code rather than > having it split between Java and C++. It felt to me as cleaner to have it > in one place rather than split between java and C++. You can see the code > for that in SVNRaFactory.java in patch 18 and > org_apache_subversion_javahl_ra_SVNRaFactory.cpp in patch 20 > > 3. I will change GetDateRevision to take longs instead of Date object > for the native call and have an additional wrapper method in java that > takes the date. Otherwise I think this method would be affected by issue > 2359 <http://subversion.tigris.org/issues/show_bug.cgi?id=2359> > (truncated time stamps) > > 4. I strongly dislike checked exceptions in code paths where there is > no expected recovery logic that could be applied. This just forces people > to either write a lot of try catch blocks that don't have any useful logic, > propagate the exception on all their methods, or catch and wrap the > exception in a RuntimeException derived class. Once again, if checked > exceptions is what is desired, that is what I will use. But I would be > curious to know the reasoning behind the decision. > > Looking forward to hearing from you. > > Vladimir > > >> >> -Hyrum >> >> > While developing the prototype I tried to make minimal changes to the >> > existing code, while reusing as much as possible for the new RA >> > implementation. As a result the new code takes advantage of new >> additions >> > that old code does not. Also not being an expert on Subversion API, >> where it >> > conflicted with JNI code I assumed JNI code was wrong and changed it. >> > >> > During the course of implementation I made couple of choices for the >> > reasons listed below: >> > >> > Hierarchical nature of editor baton's required life cycle management >> of >> > the wrapper C++ and java objects for cases when a parent object is >> disposed >> > before its children. I though of two possibilities: >> > >> > * Maintain weak JNI references in SVNBase then a parent can zero out >> > cppAddr for the child java objects if they are still around >> > * Internally release all resources maintained by the wrapper object >> and >> > set a flag that object has been disposed that is checked at the >> beginning of >> > each call. The wrapper object is deleted when dispose() or finalize() >> > method is explicitly called on the java object >> > >> > I chose the later because it did not require use of extra resources >> from >> > the JVM, freed majority of the used memory, and only had overhead when >> the >> > caller did not properly dispose of the objects. >> > >> > I also ran into an issue where I was not sure of what was the proper >> fix. >> > When passing "BAD" as a parameter to reparent() function, assert was >> > encountered that killed the JVM. Would it be ok to add uri parsing >> check to >> > svn_ra_reparent in ra_loader.c like the one done in svn_ra_open4? >> > >> > java: subversion/libsvn_subr/dirent_uri.c:1483: uri_skip_ancestor: >> Assertion >> > `svn_uri_is_canonical(child_uri, ((void *)0))' failed. >> > >> > >> > >> > Please see the remaining emails in this thread for the actual patches. >> > >> > * 01 - 02: C++ -- Enhancements to the existing JavaHL APIs >> independent >> > from the new Ra APIs >> > * 03 - 03: Java -- Factor out common code for use by the JavaHL Ra >> APIs >> > * 04 - 17: C++ -- Factor out common code for use by the JavaHL Ra >> APIs >> > * 18 - 18: Java -- The new code for the JavaHL Ra APIs >> > * 19 - 19: Build -- Include new java code in the build process >> > * 20 - 20: C++ -- The new code for the JavaHL Ra APIs >> > * 21 - 22: Java -- The new unit test code for the JavaHL Ra APIs >> > >> > In order to for the patches to apply please run the following copy >> commands >> > before applying patches 03, 12 and 13: >> > >> > * 03: svn cp >> > >> subversion/bindings/javahl/src/org/apache/subversion/javahl/SVNClient.java >> > >> subversion/bindings/javahl/src/org/apache/subversion/javahl/CommonContext.java >> > >> > * 12: svn cp subversion/bindings/javahl/native/RevpropTable.h >> > subversion/bindings/javahl/native/StringsTable.h >> > * 12: svn cp subversion/bindings/javahl/native/RevpropTable.cpp >> > subversion/bindings/javahl/native/StringsTable.cpp >> > >> > * 13: svn cp subversion/bindings/javahl/native/ClientContext.h >> > subversion/bindings/javahl/native/CommonContext.h >> > * 13: svn cp subversion/bindings/javahl/native/ClientContext.cpp >> > subversion/bindings/javahl/native/CommonContext.cpp >> > >> > Thank you, >> > >> > Vladimir >> > >> >> >> >> -- >> >> uberSVN: Apache Subversion Made Easy >> http://www.uberSVN.com/ >> > >
