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 <vmpn_at_hitechman.com> wrote:
> 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
>> (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/
Received on 2012-05-15 17:55:38 CEST