[
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)
