You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Vladimir Berezniker <vm...@hitechman.com> on 2012/05/04 05:23:30 UTC
Re: [RFC][PATCH 00/22] JavaHL Ra API Implementation
So what would be the next steps.
Thank you,
Vladimir
On Tue, Apr 24, 2012 at 9:52 PM, Vladimir Berezniker <vm...@hitechman.com>wrote:
>
>
> On Mon, Apr 23, 2012 at 10:03 AM, Hyrum K Wright <
> hyrum.wright@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
>> <vm...@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/
>>
>
>
Re: [RFC][PATCH 00/22] JavaHL Ra API Implementation
Posted by Hyrum K Wright <hy...@wandisco.com>.
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 <vm...@hitechman.com> wrote:
> So what would be the next steps.
>
> Thank you,
>
> Vladimir
>
>
> On Tue, Apr 24, 2012 at 9:52 PM, Vladimir Berezniker <vm...@hitechman.com>
> wrote:
>>
>>
>>
>> On Mon, Apr 23, 2012 at 10:03 AM, Hyrum K Wright
>> <hy...@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
>>> <vm...@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/