You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hawq.apache.org by superbobry <gi...@git.apache.org> on 2018/06/12 18:35:23 UTC
[GitHub] incubator-hawq pull request #1376: [RFC] Changed libhdfs3 to be ABI compatib...
GitHub user superbobry opened a pull request:
https://github.com/apache/incubator-hawq/pull/1376
[RFC] Changed libhdfs3 to be ABI compatible with libhdfs
I have been working on this in the context of tensorflow/tensorflow#16919, and the three discrepancies I've identified are:
* libhdfs3 does not define `hdfsPread` which nonetheless can be implemented in terms of `hdfsTell`, `hdfsSeek` and `hdfsRead`;
* it also uses the name `hdfsSync` for `hdfsHSync`;
* `hdfsFlush`, which flushes the internal buffer of the InputStream in libhdfs, in libhdfs3 always flushes to HDFS, i.e. it does what `hdfsHFlush` should do. One way to address this is to make `hdfsFlush` a noop, but I guess it's already too late to change the implementation, wdyt?
Do you want me to add a test for `hdfsPread`? It does not do much on its own, but I can add one nonetheless.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/superbobry/incubator-hawq libhdfs-abi-compat
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-hawq/pull/1376.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1376
----
commit c048cd3345235ba41bfeb961cfeb83f62b595b24
Author: Sergei Lebedev <s....@...>
Date: 2018-06-12T17:55:02Z
Deprecated hdfsSync in favour of hdfsHSync
It was probably misnamed at some point, because the implementation seems
to be doing something very close to its JVM counterpart.
commit 9650b1644c7becf6ca4d97ed84a995d18210a189
Author: Sergei Lebedev <s....@...>
Date: 2018-06-12T18:17:54Z
Implemented hdfsPread
Closes Pivotal-Data-Attic/attic-c-hdfs-client#51. AFAIK there is no
corresponding ticket in HAWQ JIRA.
----
---
[GitHub] incubator-hawq issue #1376: HAWQ-1624. Change libhdfs3 to be ABI compatible ...
Posted by interma <gi...@git.apache.org>.
Github user interma commented on the issue:
https://github.com/apache/incubator-hawq/pull/1376
@superbobry
Merged, please close this PR, thanks.
---
[GitHub] incubator-hawq issue #1376: HAWQ-1624. Change libhdfs3 to be ABI compatible ...
Posted by interma <gi...@git.apache.org>.
Github user interma commented on the issue:
https://github.com/apache/incubator-hawq/pull/1376
@superbobry
Code review is ok, I need to do more tests to verify your modification.
I will @ you if all tests pass (or provide failure massages so that you can fix them), I plan to finish all tests and reply to you in the first half of next week.
Wait some moments, Thanks.
---
[GitHub] incubator-hawq issue #1376: HAWQ-1624. Change libhdfs3 to be ABI compatible ...
Posted by superbobry <gi...@git.apache.org>.
Github user superbobry commented on the issue:
https://github.com/apache/incubator-hawq/pull/1376
@amyrazz44, @wangzw thanks for the feedback! Could you kindly comment on my concern regarding the behaviour of `hdfsSync`?
---
[GitHub] incubator-hawq issue #1376: HAWQ-1624. Change libhdfs3 to be ABI compatible ...
Posted by interma <gi...@git.apache.org>.
Github user interma commented on the issue:
https://github.com/apache/incubator-hawq/pull/1376
@superbobry Thanks for your PR and sorry for response late.
* hdfsPread, LGTM, thanks for your tests.
* hdfsSync, LGTM
Looks you just added a new `hdfsHSync()` function and didn't change original `hdfsSync()` behavior, so it works with HAWQ.
* hdfsFlush
Do you mean you want to modify the `hdfsFlush()` behavior? I greped code, HAWQ just uses `hdfsHFlush()` by now. So you can feel free to modify `hdfsFlush()`, but make sure pass all tests (run all tests is a hard thing for new committer, if your modification is simple, I can run this test for you).
---
[GitHub] incubator-hawq pull request #1376: HAWQ-1624. Change libhdfs3 to be ABI comp...
Posted by superbobry <gi...@git.apache.org>.
Github user superbobry closed the pull request at:
https://github.com/apache/incubator-hawq/pull/1376
---
[GitHub] incubator-hawq pull request #1376: [RFC] Changed libhdfs3 to be ABI compatib...
Posted by wangzw <gi...@git.apache.org>.
Github user wangzw commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1376#discussion_r195622167
--- Diff: depends/libhdfs3/src/client/Hdfs.cpp ---
@@ -35,6 +35,7 @@
#include "Thread.h"
#include "XmlConfig.h"
+#include <boost/scope_exit.hpp>
--- End diff --
Do not make boost as hard requirement. libhdfs3 only needs boost if you are using an old version compiler such as GCC 4.4.
If the function provided by boost is not in the standard c++ library, do not use such function.
---
[GitHub] incubator-hawq issue #1376: HAWQ-1624. Change libhdfs3 to be ABI compatible ...
Posted by interma <gi...@git.apache.org>.
Github user interma commented on the issue:
https://github.com/apache/incubator-hawq/pull/1376
@wangzw thanks!
---
[GitHub] incubator-hawq issue #1376: [RFC] Changed libhdfs3 to be ABI compatible with...
Posted by amyrazz44 <gi...@git.apache.org>.
Github user amyrazz44 commented on the issue:
https://github.com/apache/incubator-hawq/pull/1376
Please create [Apache HAWQ JIRA](https://issues.apache.org/jira/projects/HAWQ/summary) for your commit and add the corresponding test for your code changes. Thanks.
---
[GitHub] incubator-hawq issue #1376: HAWQ-1624. Change libhdfs3 to be ABI compatible ...
Posted by interma <gi...@git.apache.org>.
Github user interma commented on the issue:
https://github.com/apache/incubator-hawq/pull/1376
@superbobry
As I known, no/not many person declare that they depend on libhdfs3. In the past, they just make a copy into their repo if need it (not a good things, but understandable, maybe making all hawq tests passed is a big burden for them).
So I think breaking changes are not a big deal in libhdfs3, if guys found something broken, hope them to back to HAWQ community.
---
[GitHub] incubator-hawq issue #1376: HAWQ-1624. Change libhdfs3 to be ABI compatible ...
Posted by interma <gi...@git.apache.org>.
Github user interma commented on the issue:
https://github.com/apache/incubator-hawq/pull/1376
@superbobry all tests passed, cool!
@amyrazz44 @wengyanqing for another +1.
Thanks.
---
[GitHub] incubator-hawq issue #1376: HAWQ-1624. Change libhdfs3 to be ABI compatible ...
Posted by superbobry <gi...@git.apache.org>.
Github user superbobry commented on the issue:
https://github.com/apache/incubator-hawq/pull/1376
I'd appreciate if you guys could also have a look at my question in the JIRA ticket on the inefficiency of hdfsPread.
---
[GitHub] incubator-hawq pull request #1376: HAWQ-1624. Change libhdfs3 to be ABI comp...
Posted by wangzw <gi...@git.apache.org>.
Github user wangzw commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1376#discussion_r195728970
--- Diff: depends/libhdfs3/src/client/Hdfs.cpp ---
@@ -35,6 +35,7 @@
#include "Thread.h"
#include "XmlConfig.h"
+#include <boost/scope_exit.hpp>
--- End diff --
I did not means the compiler version constrain, just use c++ standard library first, and use boost as an alternative.
---
[GitHub] incubator-hawq issue #1376: HAWQ-1624. Change libhdfs3 to be ABI compatible ...
Posted by superbobry <gi...@git.apache.org>.
Github user superbobry commented on the issue:
https://github.com/apache/incubator-hawq/pull/1376
@interma this is OK with me. Do you think the PR is good to merge?
---
[GitHub] incubator-hawq issue #1376: HAWQ-1624. Change libhdfs3 to be ABI compatible ...
Posted by superbobry <gi...@git.apache.org>.
Github user superbobry commented on the issue:
https://github.com/apache/incubator-hawq/pull/1376
Hi @interma, thank you for the review.
Regarding hdfsFlush: what I meant was how to proceed with making its behaviour compatible with libhdfs, i.e. removing the H part from the implementation. Even though HAWQ does not rely on hdfsFlush, other users of the library could, in theory, assume that hdfsFlush is hdfsHFlush. What is the usual policy for dealing with such potentially breaking changes?
---
[GitHub] incubator-hawq pull request #1376: HAWQ-1624. Change libhdfs3 to be ABI comp...
Posted by superbobry <gi...@git.apache.org>.
Github user superbobry commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1376#discussion_r195726512
--- Diff: depends/libhdfs3/src/client/Hdfs.cpp ---
@@ -35,6 +35,7 @@
#include "Thread.h"
#include "XmlConfig.h"
+#include <boost/scope_exit.hpp>
--- End diff --
Removed. Which C++ version do you target for libhdfs3?
Also, just out of curiosity where does the compiler version constraint come from? CentOS 6?
---