Re: [PATCH] JavaHL: New method for creating java objects linked to their C++ counterpart

On Mon, Jun 11, 2012 at 2:39 PM, Vladimir Berezniker <vmpn_at_hitechman.com> wrote:
>
>
> On Mon, Jun 11, 2012 at 6:20 AM, Hyrum K Wright <hyrum.wright_at_wandisco.com>
> wrote:
>>
>> On Thu, Jun 7, 2012 at 4:03 AM, Vladimir Berezniker <vmpn_at_hitechman.com>
>> wrote:
>> > Hi All,
>> >
>> > The intention if this patch is to introduce reusable logic for creating
>> > java
>> > objects from within C++.  This keeps object creation logic fully within
>> > C++
>> > while leaving to java the decision as to when they will be destroyed. It
>> > will be used by RA code to allocate container object for items like
>> > SVNRa,
>> > Editor, Directory and File.
>> >
>> > Thank you,
>> >
>> > Vladimir
>> >
>> > [[[
>> > JavaHL: New method for creating java objects linked to their C++
>> > counterpart
>> >
>> > [ in subversion/bindings/javahl/native ]
>> >
>> > * SVNBase.cpp, SVNBase.h
>> >   (createCppBoundObject): New method for creating java objects linked to
>> > their
>> >     C++ counterpart
>> >
>> > [ in subversion/bindings/javahl/src/org/tigris/subversion/javahl/ ]
>> >
>> > * JNIObject.java: Base class for JNI linked java objects
>> > ]]]
>>
>> Looks good.  Is there a way to mark the new Java class as private?
>
>
> The original plan is to put RA java classes in a ra/ sub package, so package
> visible
> would not work in such case.  It is an abstract class, even if someone
> instantiates
> it will be harmless, as it will not create any C++ instances.  Would putting
> a good
> javadoc explaining it is not part of the public API address your concerns?

Yes, just documenting that it is private should work. My primary
concern is backward compat, rather than technical issues.

>>
>>
>> How do you plan to use this functionality?
>
>
> As compared to what SVNClient class does, where caller creates the java
> object which
> in turn calls native method call to create a matching native object. With RA
> layer,
> consumer calls a factory method:
>
> /**
> * Crates RA session for a given url with provided context
> * @param url to connect to
> * @param uuid of the remote repository, can be null if uuid check is not
> desired
> * @param config configuration to use for the session.
> * @return RA session
> */
> public static native ISVNRa createRaSession(String url, String uuid,
> ISVNRaConfig config);
> }
>
> that method is responsible for instantiation of the related C++ and Java
> classes.
> I think it is cleaner because:
>
> It better encapsulates implementation class: User only sees factory
> and ISVNRa interface
> Instantiation logic resides in a single language
> It avoids chicken and the egg with not year having cppAddr when java object
> is constructed: Allows enforcement that parameter is supplied via
> constructor argument

If you think that's cleaning, I'm happy to support it. It's been a
while since I've been in that part of the code, and I don't think many
folks around here have spent much time reasoning about it lately.
You're probably the domain expert. :)

>  Relatedly, do you have a
>>
>> high-level plan for this series of patches?
>
>
> Let me get the up to date version published. But high level summary for now:
>
> 1. Have the following 3 patches reviewed, so that minimal version of SVNRa
> implementation
> can be committed to the javahl-ra branch and merged with the existing code
> base.
>
> [PATCH] JavaHL: Add SVN_JNI_STRING macro to reduce amount of code necessary
> to declare JNIStringHolder and check for exceptions
> [PATCH] JavaHL: New method for creating java objects linked to their C++
> counterpart
> [PATCH] JavaHL: Factor out common context to be shared between SVNClient and
> SVNRa classes
>
> 2. Commit additional RA function calls that do not require additional
> enhancements to the
> common code to the branch. These are mostly pass-through calls to the C
> functions.
>
> 3. Submit another ~6 patches for review om shared code that will support RA
> Editor code
>
> 4. Commit revised RA Editor implementation to the branch
>
> 5. Present proposal on making RA layer use Runtime exceptions rather than
> checked ones

Thanks for that, and updating the branch.

A couple more things, now that I'm thinking about it:
* There is a nascent project on google code called SVNJ
(http://code.google.com/p/svnj/) which had some kind of editor
implementation. That code is Apache licensed, so we should be able to
use it. You may want to see if anything there is useful.
* We're in the middle of a rather large effort to move from the
current delta editor to something called Ev2. Ev2 should be simpler
from a bindings perspective, but it won't be fully implemented for a
while. When it is implemented, it's something you'll want to support,
but we don't have a concrete timeline for it, yet.

Thanks for being patient with my somewhat sporadic responses. :)

-Hyrum

-- 
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/