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/ >
