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-04-25 03:53:17 CEST