So what would be the next steps.
Thank you,
Vladimir
On Tue, Apr 24, 2012 at 9:52 PM, Vladimir Berezniker <vmpn_at_hitechman.com>wrote:
>
>
> On Mon, Apr 23, 2012 at 10:03 AM, Hyrum K Wright <
> hyrum.wright_at_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
>> <vmpn_at_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/
>>
>
>
Received on 2012-05-04 05:24:11 CEST