I think the next steps would be do to this work on a branch. -Hyrum
On Thu, May 3, 2012 at 10:23 PM, Vladimir Berezniker <v...@hitechman.com> wrote: > 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 >> (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/ >> >> > -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com/