[jira] [Commented] (HDFS-573) Porting libhdfs to Windows

["Colin Patrick McCabe (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HDFS-573?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14081336#comment-14081336 ] 

Colin Patrick McCabe commented on HDFS-573:
-------------------------------------------

Thanks for looking at this, Chris.  Looks pretty good.

{code}
/* Use gcc type-checked format arguments.  This is not supported on Windows. */
#ifdef _WIN32
#define TYPE_CHECKED_PRINTF_FORMAT(formatArg, varArgs)
#else
#define TYPE_CHECKED_PRINTF_FORMAT(formatArg, varArgs) \
  __attribute__((format(printf, formatArg, varArgs)))
#endif
{code}
Let's put this in {{platform.h}} rather than {{exception.h}}.  It will be useful in a lot of different spots.

{code}
    const tTime NO_CHANGE = -1;
{code}
Let's make this {{static}} while we're moving it.

{code}
    const char *cPathName;
    const char* cUserName;
    const char* cGroupName;
{code}
nit: stars should be next to the variable name

{{jni_helper.c}}: This hash table stuff is not quite correct.  For instance, here:
{code}
static int hashTableInit(void)
{
    if (!gClassRefHTable) {
        LOCK_HASH_TABLE();
{code}

You're checking {{gClassRefHTable}} without the lock, so there's no guarantee that another thread's changes will be visible to you.

The code here needs some fixing (and needed some fixing even before your patch-- this isn't something you introduced.)  You can see that there's a potential double insert issue here:

{code}
jthrowable globalClassReference(const char *className, JNIEnv *env, jclass *out)
{
    jclass clsLocalRef;
    jclass cls = searchEntryFromTable(className);
    if (cls) {
        *out = cls;
        return NULL;
    }
    <===== POINT 1
    clsLocalRef = (*env)->FindClass(env,className);
    if (clsLocalRef == NULL) {
        return getPendingExceptionAndClear(env);
    }
    cls = (*env)->NewGlobalRef(env, clsLocalRef);
    if (cls == NULL) {
        (*env)->DeleteLocalRef(env, clsLocalRef);
        return getPendingExceptionAndClear(env);
    }
    (*env)->DeleteLocalRef(env, clsLocalRef);
    insertEntryIntoTable(className, cls);
    *out = cls;
    return NULL;
{code}

Because {{globalClassReference}} drops the lock after {{searchEntryFromTable}}, two threads could both get to POINT 1 at the same time, and end up creating two global class references to the same class.  Then the insert would fail for one and we'd have a memory leak.

It's better just to have the {{globalClassReference}} function hold the hash table lock the whole way through, and  atomically search + insert if not present.  We also don't need the silly LOCK_HASH_TABLE macros (a macro for one ordinary function call?), or really any of the hash table wrapper functions.  Check out the HADOOP-10388 branch, this is fixed there.

Again, I realize you didn't break this, but while we're in this code, let's fix it :)

{code}
    char *hadoopClassPathVMArg = "-Djava.class.path=";
{code}
If this doesn't change it should be {{const char * const}}.

{code}
    hdfsBuilderSetNameNodePort(bld, (tPort)port);
{code}
Can we avoid this typecast by making port be a variable of type {{tPort}}?  Same comment in {{hdfsSingleNameNodeConnect}}.

thanks Chris

> Porting libhdfs to Windows
> --------------------------
>
>                 Key: HDFS-573
>                 URL: https://issues.apache.org/jira/browse/HDFS-573
>             Project: Hadoop HDFS
>          Issue Type: Improvement
>          Components: libhdfs
>         Environment: Windows, Visual Studio 2008
>            Reporter: Ziliang Guo
>            Assignee: Chris Nauroth
>         Attachments: HDFS-573.1.patch
>
>   Original Estimate: 336h
>  Remaining Estimate: 336h
>
> The current C code in libhdfs is written using C99 conventions and also uses a few POSIX specific functions such as hcreate, hsearch, and pthread mutex locks.  To compile it using Visual Studio would require a conversion of the code in hdfsJniHelper.c and hdfs.c to C89 and replacement/reimplementation of the POSIX functions.  The code also uses the stdint.h header, which is not part of the original C89, but there exists what appears to be a BSD licensed reimplementation written to be compatible with MSVC floating around.  I have already done the other necessary conversions, as well as created a simplistic hash bucket for use with hcreate and hsearch and successfully built a DLL of libhdfs.  Further testing is needed to see if it is usable by other programs to actually access hdfs, which will likely happen in the next few weeks as the Condor Project continues with its file transfer work.
> In the process, I've removed a few what I believe are extraneous consts and also fixed an incorrect array initialization where someone was attempting to initialize with something like this: JavaVMOption options[noArgs]; where noArgs was being incremented in the code above.  This was in the hdfsJniHelper.c file, in the getJNIEnv function.



--
This message was sent by Atlassian JIRA
(v6.2#6252)