hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Anu Engineer (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (HDFS-12340) Ozone: C/C++ implementation of ozone client using curl
Date Thu, 07 Sep 2017 06:05:00 GMT

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

Anu Engineer edited comment on HDFS-12340 at 9/7/17 6:04 AM:
-------------------------------------------------------------

Thank you for updating the patch. Some more minor comments.


1. /hadoop-common/src/CMakeLists.txt:  Can we please handle the case where there is no curl
installed on the build machine? if not found case.

2.nit: Can we please format the curl finding part consistent with the code around it? 

3. src/main/native/libozoneclient/CMakeLists.txt: nit: I find it unusable to name the extension
with a capital letter as in ".C", if there is such a convention can you please share? 

4. LIBOZONECLIENT_VERSION "0.0.0", can we please set to this to 0.0.1 ?

5. Same comment for other CMake files where we use a ".C" file convention.

6. In main.c -- Can we use a proper command parser instead of case 1: 2: etc. I am looking
for something like 
{noformat}
> createVolume volume1
> help 
{noformat}
etc. and correspondingly change the printUsage ? 

7. In main.c -- since we have a bunch of isValidStr calls in the code, does it make sense
to put them in the same place and have one printUsage call ? also would you like to init rc
to -1 or 1 as the case may be and just return the rc always? 

8. In main.c: main function -- can you please init all the variables. 
{noformat}
  char user[64] = {0};
  char serverIp[64] = {0};
  char port[64] = {0};
  char version[16] = {0};
  char vol[256] = {0};
  char bucket[256] = {0};
  char infile[256] = {0};
  char key[256] = {0};
{noformat}

9. ozone_client.C: nit: can we please rename this as ozone_client.c -- small "c" as the name
of the file.

10. we have isValidStr twice, I agree the scope is restricted, but I think we can define it
once and can be used with 0 to cover the first use case.

11. nit: int executeCurlDownloadOpearation(ozoneInfo* info, char *url, FILE *fd); can we please
have consistent placement for the * operator ? always with the variable and away from the
type ? 

12: Again a nit: you don't have to fix this. When we write a library, it is very rare that
we will print to streams, even error stream. Generally, we will return different error codes
so that calling application can decide how to print it, or we can provide a function that
returns error string if needed. It makes it easier to use the library under c/c++ code. Please
see how curl itself does this.

13: nit : you don't need these lines. 
{noformat}
 /* initialize the ozone  Info structure */
 info->curl = NULL;
 info->chunk = NULL;
 info->url = NULL;
 info->user = NULL;
 info->version = NULL;
 {noformat}
 calloc already takes care of this.

 14: in the error path of initOzoneClient, don't we have to call the opposite function of
curl_easy_init, that is curl_easy_cleanup ? 


15: In function initOzoneClient, the variable rc is not needed. You can jump to exit only
if you get a failure and return info just before that. That way you can eliminate the if /
else condition too.

16: In function resetOzoneInfo: nit: line 148, double semi-colon at the end.

17: Before we dereference info, check it is not null. Line 152

18: resetOzoneInfo: The error handling of this function seems wrong. Here is a case where
there could be a resource leak.  Let us say the function fails at line 158, where is trying
to do curl_easy_init(). The function fails with returning \-1. Now, what do I do with the
ozoneInfo object? I cannot call the resetOzoneInfo function safely, since curl_easy_cleanup
will fail if I try to call this function again, and I have no idea how to release this object.
I would suggest that we re-write it, possibly with better error handling strategy. I cannot
call destroyOzoneClient since it would suffer from the same issue that curl_easy_cleanup would
happen on the info->curl, which has not been allocated.

19: In resetOzoneInfo: Is it possible that chunk will just accumulate the same tags again
and again ? 

20: In createBucket: 
{noformat}
  strcpy(url, info->url);
  strcat(url, volumeName);
  strcat(url, "/");
  strcat(url, bucketName);
{noformat}
You can replace this with a single call to sprintf, same comment every other function.

21: In putKey: if we fail the if test in line 341/346/351/356 the program will crash since
we are closing a NULL file in the exit path.


was (Author: anu):
Thank you for updating the patch. Some more minor comments.


1. /hadoop-common/src/CMakeLists.txt:  Can we please handle the case where there is not curl
installed on the build machine? if not found case.

2.nit: Can we please format the curl finding part consistent with the code around it? 

3. src/main/native/libozoneclient/CMakeLists.txt: nit: I find it unusable to name the extension
with a capital letter as in ".C", if there is such a convention can you please share? 

4. LIBOZONECLIENT_VERSION "0.0.0", can we please set to this to 0.0.1 ?

5. Same comment for other CMake files where we use a ".C" file convention.

6. In main.c -- Can we use a proper command parser instead of case 1: 2: etc. I am looking
for something like 
{noformat}
> createVolume volume1
> help 
{noformat}
etc. and correspondingly change the printUsage ? 

7. In main.c -- since we have a bunch of isValidStr calls in the code, does it make sense
to put them in the same place and have one printUsage call ? also would you like to init rc
to -1 or 1 as the case may be and just return the rc always? 

8. In main.c: main function -- can you please init all the variables. 
{noformat}
  char user[64] = {0};
  char serverIp[64] = {0};
  char port[64] = {0};
  char version[16] = {0};
  char vol[256] = {0};
  char bucket[256] = {0};
  char infile[256] = {0};
  char key[256] = {0};
{noformat}

9. ozone_client.C: nit: can we please rename this as ozone_client.c -- small "c" as the name
of the file.

10. we have isValidStr twice, I agree the scope is restricted, but I think we can define it
once and can be used with 0 to cover the first use case.

11. nit: int executeCurlDownloadOpearation(ozoneInfo* info, char *url, FILE *fd); can we please
have consistent placement for the * operator ? always with the variable and away from the
type ? 

12: Again a nit: you don't have to fix this. When we write a library, it is very rare that
we will print to streams, even error stream. Generally, we will return different error codes
so that calling application can decide how to print it, or we can provide a function that
returns error string if needed. It makes it easier to use the library under c/c++ code. Please
see how curl itself does this.

13: nit : you don't need these lines. 
{noformat}
 /* initialize the ozone  Info structure */
 info->curl = NULL;
 info->chunk = NULL;
 info->url = NULL;
 info->user = NULL;
 info->version = NULL;
 {noformat}
 calloc already takes care of this.

 14: in the error path of initOzoneClient, don't we have to call the opposite function of
curl_easy_init, that is curl_easy_cleanup ? 


15: In function initOzoneClient, the variable rc is not needed. You can jump to exit only
if you get a failure and return info just before that. That way you can eliminate the if /
else condition too.

16: In function resetOzoneInfo: nit: line 148, double semi-colon at the end.

17: Before we dereference info, check it is not null. Line 152

18: resetOzoneInfo: The error handling of this function seems wrong. Here is a case where
there could be a resource leak.  Let us say the function fails at line 158, where is trying
to do curl_easy_init(). The function fails with returning \-1. Now, what do I do with the
ozoneInfo object? I cannot call the resetOzoneInfo function safely, since curl_easy_cleanup
will fail if I try to call this function again, and I have no idea how to release this object.
I would suggest that we re-write it, possibly with better error handling strategy. I cannot
call destroyOzoneClient since it would suffer from the same issue that curl_easy_cleanup would
happen on the info->curl, which has not been allocated.

19: In resetOzoneInfo: Is it possible that chunk will just accumulate the same tags again
and again ? 

20: In createBucket: 
{noformat}
  strcpy(url, info->url);
  strcat(url, volumeName);
  strcat(url, "/");
  strcat(url, bucketName);
{noformat}
You can replace this with a single call to sprintf, same comment every other function.

21: In putKey: if we fail the if test in line 341/346/351/356 the program will crash since
we are closing a NULL file in the exit path.

> Ozone: C/C++ implementation of ozone client using curl
> ------------------------------------------------------
>
>                 Key: HDFS-12340
>                 URL: https://issues.apache.org/jira/browse/HDFS-12340
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>          Components: ozone
>    Affects Versions: HDFS-7240
>            Reporter: Shashikant Banerjee
>            Assignee: Shashikant Banerjee
>              Labels: OzonePostMerge
>             Fix For: HDFS-7240
>
>         Attachments: HDFS-12340-HDFS-7240.001.patch, HDFS-12340-HDFS-7240.002.patch,
main.C, ozoneClient.C, ozoneClient.h
>
>
> This Jira is introduced for implementation of ozone client in C/C++ using curl library.
> All these calls will make use of HTTP protocol and would require libcurl. The libcurl
API are referenced from here:
> https://curl.haxx.se/libcurl/
> Additional details would be posted along with the patches.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-help@hadoop.apache.org


Mime
View raw message