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

On Thu, Jun 14, 2012 at 10:16 AM, Mat Booth <mat.booth_at_wandisco.com> wrote:
> On 12 June 2012 03:11, Vladimir Berezniker <vmpn_at_hitechman.com> wrote:
>>
>>
>> On Mon, Jun 11, 2012 at 8:39 AM, 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?
>>>
>>>>
>>>>
>>>> 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
>>>
>>>  Relatedly, do you have a
>>>>
>>>> high-level plan for this series of patches?
>>>
>>>
>>
>> More detailed plan, full version is in the BRANCH-README on javahl-ra
>> branch:
>>
>>    * Prepare existing code for merging of SVNRa editor       [IN PROGRESS]
>>
>>       01. JavaHL: Changed return value from the
>>           java svn_stream_t read function to be compatible
>>           with the txdelta_next_window function              [trunk_at_1342720]
>>
>>       02. JavaHL: Explicitly pass jobject jthis when
>>           processing dispose() call rather  than stashing a
>>           reference in the SVNBase class where it can be
>>           misused later                                      [trunk_at_1342810]
>>
>>       03. JavaHL: Add SVN_JNI_STRING macro to reduce amount
>>           of code necessary to declare JNIStringHolder and
>>           check for exceptions                               [IN REVIEW]
>>
>>       04. JavaHL: New method for creating java objects
>>           linked to their C++ counterpart                    [IN REVIEW]
>>
>>       05. JavaHL: Factored out common context for later use
>>           by SVNRa class                                     [IN REVIEW]
>>
>>       06. JavaHL: Minimal implementation of SVNRa            [TODO]
>>
>>       07. JavaHL: Merge existing SVNRepoAccess into SVNRA    [TODO]
>>
>>
>>    * Implement other ra functions not requiring addition code
>>      refactoring                                             [TODO]
>>
>>
>>    * Apply editor support patches                            [TODO]
>>       xx. JavaHL: Support returning non const, empty rather
>>           than NULL hash as required by
>>           (svn_ra_get_commit_editor3)
>>           apr_hash_t *revprop_table parameter                [br._at_1343456
>> TBR]
>>
>>       xx. JavaHL: Support keeping global reference to the
>>           callback java object as required by the RA API due
>>           to callback being used across method calls         [TODO]
>>
>>       xx. JavaHL: Added support for creating of svn_string_t
>>           from JNIByteArray                                  [TODO]
>>
>>       xx. JavaHL: Added SVN_JNI_BYTE_ARRAY macro to reduce
>>           amount of duplicate code dealing with jbyteArray
>>           wrapper and checking for exceptions                [TODO]
>>
>>       xx. JavaHL: Factor out common java string map
>>           processing into StringsTable class from
>>           svn_string_t specific processing in the
>>           RevpropTable class                                 [TODO]
>>
>>
>
> Hi Vladimir,
>
> When you start implementing JavaHL RA, what would be the best way for
> me to try it out? Will the work be on this branch first:
> http://svn.apache.org/repos/asf/subversion/branches/javahl-ra/ ?

I think that's the goal. We're encouraging him to put as much "common
functionality" type stuff on trunk, though.

-Hyrum

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