[
https://issues.apache.org/jira/browse/HDFS-3606?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13412427#comment-13412427
]
Andy Isaacson commented on HDFS-3606:
-------------------------------------
Structure comment -- I wasn't expecting to find a whole bespoke testing
framework in expect.h when I opened this patch. That deserves a mention in the
description and/or upload comments for the diff.
(The framework is fine, I was just not expecting to see it here based on the
descriptions.)
Minor comment on the framework:
{code}
+#define EXPECT_LT(x, y) \
+ do { \
+ if ((x) >= (y)) { \
+ fprintf(stderr, "TEST_ERROR: failed on line %d: %s\n",\
+ __LINE__, #x); \
{code}
we might as well print #y too, even though this is pretty reasonable as it
stands.
Similarly for all the other {{EXPECT_foo(x,y)}} macros.
{code}
+ cobj = constructNewObjectOfClass(env, NULL, HADOOP_CONF, "()V");
...
+ bld = constructNewObjectOfClass(env, NULL, MINIDFS_CLUSTER_BUILDER,
+ "(L"HADOOP_CONF";)V", cobj);
+ if (!bld) {
+ fprintf(stderr, "nmdCreate: unable to construct "
+ "NativeMiniDfsCluster#Builder\n");
+ goto error_free_cl;
{code}
I could be missing something, but ... don't we leak {{cobj}} here? I think
this error exit should be {{goto error_dlr_cobj}}.
{code}
+ if (invokeMethod(env, &val, NULL, INSTANCE, bld,
+ MINIDFS_CLUSTER_BUILDER, "format",
...
+ if (invokeMethod(env, &val, NULL, INSTANCE, bld,
+ MINIDFS_CLUSTER_BUILDER, "build",
...
+ (*env)->DeleteLocalRef(env, val.l);
{code}
In the success code path, do we need to {{DeleteLocalRef}} the object returned
from the first call, to {{invokeMethod(..., "format", ...)}}?
The various {{nmdFoo}} functions return a mix of {{EIO}} and {{-EIO}}. I feel
like {{EIO}} makes a little more sense since the only answers are either
"success" or "failure". Except for {{nmdGetNameNodePort}} which returns a
small positive integer on success ... so maybe standardizing on {{-EIO}} makes
more sense. Your call.
{code}
+ eret = strlen(prefix);
+ ret = hdfsWrite(fs, file, prefix, eret);
{code}
Could we name this {{expected}} or {{buflen}} rather than {{eret}}? Sorry for
the nitpick but it really threw me off reading the code.
> libhdfs: create self-contained unit test
> ----------------------------------------
>
> Key: HDFS-3606
> URL: https://issues.apache.org/jira/browse/HDFS-3606
> Project: Hadoop HDFS
> Issue Type: Test
> Components: libhdfs
> Affects Versions: 2.0.1-alpha
> Reporter: Colin Patrick McCabe
> Assignee: Colin Patrick McCabe
> Priority: Minor
> Attachments: HDFS-3606.001.patch, HDFS-3606.003.patch
>
>
> We should have a self-contained unit test for libhdfs and also for FUSE.
> We do have hdfs_test, but it is not self-contained (it requires a cluster to
> already be running before it can be used.)
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
