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/04/22 05:51:11 UTC
[RFC][PATCH 00/22] JavaHL Ra API Implementation
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.
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
[RFC][PATCH 05/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Support keeping global reference to the callback java object as
required by the RA API due to callback being used across method calls
[ in subversion/bindings/javahl/native ]
* CommitCallback.cpp, CommitCallback.h
(CommitCallback, ~CommitCallback): Add handling of additional
parameter and state when requesting a global reference to be kept
]]]
[RFC][PATCH 22/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Add unit tests for the Ra API
[ in subversion/bindings/javahl/test/org/tigris/subversion/javahl/ ]
* RaReadonlyTests.java: New readonly tests for the RA API
* RaTests.java: New read/write tests for the RA API
* RunTests.java
(suite): Add the RaReadonlyTests and RaTests to the test suit
]]]
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/
Re: [RFC][PATCH 00/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
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 Vladimir Berezniker <vm...@hitechman.com>.
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 <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 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. It also reduces the amount of code which may need to be
rewritten.
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. 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.
-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/
[RFC][PATCH 11/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Added SVN_JNI_INPUT_STREAM macro to reduce amount of duplicate
code dealing with InputStream wrapper and checking for exceptions
[ in subversion/bindings/javahl/native ]
* InputStream.h
(SVN_JNI_INPUT_STREAM): New macro to declare local variable of type
InputStream and return in case of exceptions
]]]
[RFC][PATCH 15/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Separated logic for clearing java object cppAddr into its own
function as needed by the commit editor C++ objects that require the child
to clear the reference and parent to perform deletion
[ in subversion/bindings/javahl/native ]
* SVNBase.cpp, SVNBase.h
(dispose, disconnectCppObject): Split logic for clearing cppAddr
value in the java object into its own function
]]]
[RFC][PATCH 14/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Added support for allocating SVNBase object from passed in
pools rather than global pool, as needed by the Ra JNI implementation
[ in subversion/bindings/javahl/native ]
* SVNBase.cpp, SVNBase.h
(SVNBase): Additional constructor taking a parent pool parameter
]]]
[RFC][PATCH 09/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Added support for creating of svn_string_t from JNIByteArray
[ in subversion/bindings/javahl/native ]
* JNIByteArray.cpp, JNIByteArray.h
(getLength): Mark as const as the function does not alter class data
and can be used by other const functions
(getSvnString): New function to convert JNI byte array to svn_string_t
]]]
[RFC][PATCH 19/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Include the new Ra java code in the build process
* build.conf
(options): Don't try to find new jni header files before they are
generated
(javahl-java): compile classes in the
src/org/apache/subversion/javahl/ra directory
(javahl-ra-javah): new section for generating jni header files for
the Ra classes
(libsvnjavahl): make the javahl library depend on the ra jni files
]]]
[RFC][PATCH 21/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Expose additional methods publicly so that readonly tests can
use them to setup a single shared readonly test repository to speed up tests
[ in subversion/bindings/javahl/test/org/tigris/subversion/javahl/ ]
* SVNTests.java:
(USERNAME, PASSWORD): Make test username and password publicly
available constants
(setUp): Make the method public
(DefaultPromptUserPassword): make the class public.
(prompt): Return true so that the getPassword() and getUsername()
methods are called by the JNI code
(getConf): New method to retreive test configuration directory
(getGreeksRepoUrl): New method to retreive URL to the shared test repo
]]]
[RFC][PATCH 13/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Factor out common progress and cancellation callbacks as well
as configuration and authentication context into CommonContext class from
SVNClient specific code in the ClientContext class so that it can be shared
with the Ra JNI code
[ in subversion/bindings/javahl/native ]
* CommonContext.cpp, CommonContext.h, ClientContext.cpp, ClientContext.h
(username, password, getConfigDirectory, setConfigDirectory,
setPrompt, cancelOperation, progress,): Move from ClientContext to
CommonContext
* CommonContext.cpp, CommonContext.h
(attachJavaObject): Create new function to hold common logic of
attaching to the java CommonContext class used for callbacks
(getConfigData, getAuthBaton): Split getContext into separate
configuration data setup and authentication data setup to better reflect
their different life cycles
(getClientName): New function to return client name that should be
used in callbacks
* ClientContext.cpp, ClientContext.h
(ClientContext, getContext): Use the factored out CommonContext
member variables and functions
]]]
[RFC][PATCH 04/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Support returning non const, empty rather than NULL hash as
required by (svn_ra_get_commit_editor3) apr_hash_t *revprop_table parameter
[ in subversion/bindings/javahl/native ]
* RevpropTable.cpp, RevpropTable.h
(hash): Removed const qualifier and added bool nullIfEmpty parameter
to specify whether empty hash or NULL should be returned
]]]
[RFC][PATCH 03/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Factored out common context for later use by SVNRa class
[ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]
* CommonContext.java, SVNClient.java
(ClientContext): Move to progress listener into CommonContext for
later use by the new SVNRa class
]]]
[RFC][PATCH 07/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Added CPPADDR_NULL_PTR macro to reduce amount of duplicate code
checking C++ pointer extracted from the java object
[ in subversion/bindings/javahl/native ]
* JNIUtil.h
(CPPADDR_NULL_PTR): New macro to test for NULL pointer and raise java
exception if necessary
]]]
[RFC][PATCH 01/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Fix return value from the java svn_stream_t read function to be
compatible with the txdelta_next_window function
[ in subversion/bindings/javahl/native ]
* InputStream.cpp
(read): Return 0 instead of -1 as expected by the txdelta_next_window
function
]]]
[RFC][PATCH 16/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Add new method for creating new java objects connected to a
matching C++ object
[ in subversion/bindings/javahl/native ]
* SVNBase.cpp, SVNBase.h
(createCppBoundObject): New method for creating new java objects
connected to a matching C++ object
]]]
[RFC][PATCH 02/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Explicitly pass jobject jthis when processing dispose() call
rather than stashing a reference in the SVNBase class where it can be
missused later
[ in subversion/bindings/javahl/native ]
* SVNBase.cpp, SVNBase.h
(dispose, jthis): Accept jobject jthis as explicit parameter to
dispose() and delete the member variable jthis
* SVNClient.cpp, SVNClient.h, SVNRepos.cpp, SVNRepos.h
(dispose): Accept object jthis as explicit parameter and pass it to
SVNBase::dispose
* org_apache_subversion_javahl_SVNClient.cpp,
org_apache_subversion_javahl_SVNRepos.cpp
(Java_org_apache_subversion_javahl_SVNClient_dispose,
Java_org_apache_subversion_javahl_SVNRepos_dispose): Pass object jthis as
explicit parameter and pass it to the C++ wrapper class
]]]
[RFC][PATCH 10/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Added SVN_JNI_BYTE_ARRAY macro to reduce amount of duplicate
code dealing with jbyteArray wrapper and checking for exceptions
[ in subversion/bindings/javahl/native ]
* JNIByteArray.h
(SVN_JNI_BYTE_ARRAY): New macro to declare local variable of type
JNIByteArray and return in case of exceptions
]]]
[RFC][PATCH 18/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: New Java classes exposing the Ra layer to java
[ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]
* JNIObject.java
[ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ra ]
* ISVNDirectory.java, ISVNEditor.java, ISVNFile.java, ISVNNode.java,
ISVNRa.java, ISVNRaConfig.java, RaContext.java,
SVNCommitEditorBuilder.java, SVNDirectory.java, SVNEditor.java,
SVNFile.java, SVNRa.java, SVNRaConfigDefault.java, SVNRaFactory.java
]]]
[RFC][PATCH 20/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: New C++ classes exposing the Ra layer to java
[ in subversion/bindings/javahl/native ]
* IDirectoryParent.h, org_apache_subversion_javahl_ra_SVNEditor.cpp,
org_apache_subversion_javahl_ra_SVNRa.cpp,
org_apache_subversion_javahl_ra_SVNRaFactory.cpp,
org_apache_subversion_javahl_types_ra_SVNDirectory.cpp,
org_apache_subversion_javahl_types_ra_SVNFile.cpp, RaContext.cpp,
RaContext.h, SVNDirectory.cpp, SVNDirectory.h, SVNEditor.cpp, SVNEditor.h,
SVNFile.cpp, SVNFile.h, SVNRa.cpp, SVNRa.h: New files
]]]
[RFC][PATCH 06/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Support logging of the static method calls
[ in subversion/bindings/javahl/native ]
* JNIStackElement.cpp
(JNIStackElement): Add logic to deal with NULL jthis, which happens
with static method calls
]]]
[RFC][PATCH 17/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Add support for marking object disposed without deleting it,
used by the new Ra JNI code
[ in subversion/bindings/javahl/native ]
* SVNBase.cpp, SVNBase.h
(markDisposed, assertNotDisposed, init): New function to support
tracking disposal state of the object
(ASSERT_NOT_DISPOSED): New macro for derived object to include in the
beginning of the java call to make sure object has not been disposed
]]]
[RFC][PATCH 08/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Added SVN_JNI_STRING macro to reduce amount of duplicate code
dealing with jstring wrapper and checking for exceptions
[ in subversion/bindings/javahl/native ]
* JNIStringHolder.h
(SVN_JNI_STRING): New macro to declare JNIStringHolder local variable
and return in case of exception
]]]
[RFC][PATCH 12/22] JavaHL Ra API Implementation
Posted by Vladimir Berezniker <vm...@hitechman.com>.
[[[
JavaHL: Factor out common java string map processing into StringsTable
class from svn_string_t specific processing in the RevpropTable class
[ in subversion/bindings/javahl/native ]
* StringsTable.cpp, StringsTable.h, RevpropTable.cpp, RevpropTable.h
(m_revprops): Move m_revprops to base class and rename to more
appropriate m_strings
(RevpropTable): Move constructor logic to base class StringsTable
(~RevpropTable): Move local reference release from destructor to the
end of the new common constructor as by the end of the constructor all data
has been copied and a reference to java object is no longer required
* StringsTable.cpp, StringsTable.h
(hash): New function to create (char *) to (char *) hash from java
Map<String, String> to be used for creating of lock tocken table for
svn_ra_open4 call
* RevpropTable.cpp, RevpropTable.h
(hash): Use the new base member variable m_strings instead of
m_revprops
]]]