Orc 17 #134

AnatoliShein · 2017-06-20T20:49:29Z

In this pull request I added LIBHDFS++ library for reading files from HDFS to ORC project.

Libhdfs++ is located in orc/c++/lib/libhdfspp and by default builds as a light-weight library without examples, tests, and tools (and by this avoids dependencies on JDK, valgrind and gmock). However, if the flag -DHDFSPP_LIBRARY_ONLY=FALSE is passed to cmake, then it will build the examples, tests, and tools as well.

Libhdfs++ depends on protobuf libraries in orc/c++/libs/protobuf-2.6.0 and is searching the system for packages Doxygen, OpenSSL, CyrusSASL, GSasl, and Threads dynamically (however only OpenSSL and Threads are required).

The folder libhdfspp also includes a script pull_hdfs.sh which pulls the latest changes from Libhdfs++ Hadoop branch to ORC, and generates file 'imported_timestamp' with the timestamp and the information about the latest commit.

I also updated all the ORC tools to automatically use Libhdfs++ to read ORC files on HDFS if their path begins with 'hdfs://'.

Please review.

xndai · 2017-06-22T23:46:56Z

c++/src/OrcHdfsFile.cc

+      char input_buffer[BUF_SIZE];
+      do{
+        //Reading file chunks
+        status = file->PositionRead(input_buffer, sizeof(input_buffer), offset, &last_bytes_read);


Why do we need an intermedia buffer here? Can we just pass in "buf" and "length" into PositionRead()? This way we don't need following memcpy() and can also simply the logic here.

I used intermediate buffer because it was used for PositionRead in a similar case. So I just tried without this buffer and it worked.

xndai · 2017-06-22T23:51:22Z

c++/src/OrcHdfsFile.cc

+      std::unique_ptr<hdfs::FileHandle> file_handle(file_raw);
+      file = std::move(file_handle);
+      //transferring the ownership of FileSystem to HdfsFileInputStream to avoid premature deallocation
+      file_system = fs;


Not quite understand why we need a local variable "fs". You could just initialize this->file_system. Also "fs" should have been a unique_ptr as it doesn't share ownership with anyone.

This makes sense, we don't really need a shared pointer here. Just fixed it.

xndai · 2017-06-22T23:52:49Z

c++/src/OrcHdfsFile.cc

+      }
+      //wrapping file_raw into a unique pointer to guarantee deletion
+      std::unique_ptr<hdfs::FileHandle> file_handle(file_raw);
+      file = std::move(file_handle);


You could combine these two lines into file.reset(file_raw);

Yep, good catch.

xndai · 2017-06-23T22:03:58Z

c++/src/OrcHdfsFile.cc

-          throw ParseError("Error reading the file: " + status.ToString());
-        }
-      } while (true);
+      status = file->PositionRead(buf, static_cast<size_t>(length), static_cast<off_t>(offset), &last_bytes_read);


you still need to put this in a loop in case last_bytes_read is less than length.

This makes sense. I just added a loop here.

xndai · 2017-06-23T22:04:46Z

c++/src/OrcHdfsFile.cc

-      if (!fs) {
+      //Wrapping file_system into a unique pointer to guarantee deletion
+      file_system = std::unique_ptr<hdfs::FileSystem>(hdfs::FileSystem::New(io_service, "", options));
+      if (!file_system) {


are you sure this would work? Should it be if (file_system.get() != nullptr) ?

Yes, unique pointer actually supports this notation (thanks to operator bool)

Unfortunately unique_ptr can be redefined as auto_ptr if platform doesn't support unique_ptr (see orc_config.hh). We talked about removing this redefine, but it hasn't been done yet. So I'd suggest we stick to file_system.get() != nullptr for now.

I changed it here, however '!fs' notation is also used in many other parts of the libhdfs++ library, like examples, tests, tools, and c bindings, so hopefully this does not create too much trouble.

xndai · 2017-06-23T22:06:08Z

c++/src/OrcHdfsFile.cc

-
 namespace orc {

  class HdfsFileInputStream : public InputStream {


do you have corresponding UTs for this new class?

I don't currently have the UTs for this class because to test it we need a hadoop cluster, and in order to fire one up we would need to bring the entire hadoop tree into ORC (which is actually much larger than ORC), and it can make things very heavy.

Does HDFS C++ provide a mocked service interface? My concern is the testability of HdfsFileInputStream.

Currently we don't actually have a mocked service interface in libhdfs++.

For a mock that does real network stuff libhdfs++ uses the C++/JVM bindings over the minidfscluster (java namenode and datanode in a single process) for its unit tests. That seems like a very large dependency to bring into this project. It'd be possible to write a mock that goes over TCP in C/C++, I can think of 2 ways:

Capture network traffic of a read with tcpdump/wireshark for a given request and send the response back. This would be really brittle if IO request sizes changed and pretty ugly to store a bunch of binary data.

Implement at least some of the server side HDFS RPC protocol and DataTransferProtocol to make a mock namenode and datanode. This is possible but would be a considerable amount of work that wouldn't be as useful for libhdfs++ since it already has the minidfscluster.

xndai · 2017-07-05T18:56:33Z

c++/src/OrcHdfsFile.cc

+      size_t last_bytes_read = 0;
+
+      do {
+        status = file->PositionRead(buf, static_cast<size_t>(length) - total_bytes_read, static_cast<off_t>(offset + total_bytes_read), &last_bytes_read);


Make sure to wrap on 80 characters.

Just fixed the whole file to wrap on 80 chars.

jamesclampffer · 2017-07-06T21:12:04Z

c++/libs/libhdfspp/CMakeLists.txt

+find_package(OpenSSL REQUIRED)
+find_package(Protobuf REQUIRED)
+find_package(CyrusSASL)
+find_package(GSasl)


I think you're going to need a flag to make sure it doesn't attempt to find and link against GSasl since the Apache and GPL licenses aren't compatible. Eventually GSasl will be removed from libhdfs++ for the same reason.

majetideepak · 2017-07-07T19:55:41Z

Can we add libhdfspp as a tarball and add a line to untar it?
I am going to make these changes for other libraries in the lib folder as well.

omalley · 2017-08-07T16:32:22Z

Sorry, this patch is really out of date with the master. Can you rebase it?

AnatoliShein · 2017-08-07T16:42:14Z

Hi @omalley ,
Yes, I am working on rebasing it right now. Running into some protobuf problems, but it should not be too bad.

AnatoliShein · 2017-08-07T19:27:30Z

Rebased the branch and resolved the protobuf issues, but still need to move Libhdfspp out of the libs and fix other samll issues. Testing if it passes CI-tests now.

omalley · 2017-08-14T17:59:00Z

Unfortunately, this doesn't build on MacOS. :(

I'll take some time this week to figure out what needs to be fixed. I've gone ahead and rebased it to the current master and squashed the commits in https://github.com/omalley/orc/tree/pr/134

AnatoliShein · 2017-08-15T15:36:43Z

Hi @omalley ,
What error messages do you get when trying to build on MacOS?

omalley · 2017-08-15T15:49:54Z

It looks like it is a "long" vs "long long" problem.

/Users/owen/work/code/orc/build/libhdfspp_ep-prefix/src/libhdfspp_ep/lib/common/configuration.cc:85:12: error: no viable conversion from returned value of type 'optional<long>' to function return type 'optional<long long>'
    return result;
           ^~~~~~
/Users/owen/work/code/orc/build/libhdfspp_ep-prefix/src/libhdfspp_ep/third_party/tr2/optional.hpp:427:13: note: candidate constructor not viable: no known conversion from 'std::experimental::optional<long>' to 'std::experimental::nullopt_t' for 1st argument
  constexpr optional(nullopt_t) noexcept : OptionalBase<T>() {};
            ^
/Users/owen/work/code/orc/build/libhdfspp_ep-prefix/src/libhdfspp_ep/third_party/tr2/optional.hpp:429:3: note: candidate constructor not viable: no known conversion from 'std::experimental::optional<long>' to 'const std::experimental::optional<long long> &' for 1st argument
  optional(const optional& rhs)
  ^
/Users/owen/work/code/orc/build/libhdfspp_ep-prefix/src/libhdfspp_ep/third_party/tr2/optional.hpp:438:3: note: candidate constructor not viable: no known conversion from 'std::experimental::optional<long>' to 'std::experimental::optional<long long> &&' for 1st argument
  optional(optional&& rhs) noexcept(is_nothrow_move_constructible<T>::value)
  ^
/Users/owen/work/code/orc/build/libhdfspp_ep-prefix/src/libhdfspp_ep/third_party/tr2/optional.hpp:447:13: note: candidate constructor not viable: no known conversion from 'std::experimental::optional<long>' to 'const long long &' for 1st argument
  constexpr optional(const T& v) : OptionalBase<T>(v) {}
            ^
/Users/owen/work/code/orc/build/libhdfspp_ep-prefix/src/libhdfspp_ep/third_party/tr2/optional.hpp:449:13: note: candidate constructor not viable: no known conversion from 'std::experimental::optional<long>' to 'long long &&' for 1st argument
  constexpr optional(T&& v) : OptionalBase<T>(constexpr_move(v)) {}
            ^
1 error generated.
make[5]: *** [lib/common/CMakeFiles/common_obj.dir/configuration.cc.o] Error 1
make[4]: *** [lib/common/CMakeFiles/common_obj.dir/all] Error 2
make[3]: *** [all] Error 2

jamesclampffer · 2017-08-15T16:22:19Z

Those errors should go away after HDFS-10787 is finished (or applied to the PR temporarily) since it exposes an API to get at the configuration without using std::tr2::optional. We had to suppress a bunch of errors coming from optional when it was added to libhdfs++; eventually I'd like to get rid of the remaining uses of it in the implementation as well.

majetideepak · 2017-08-15T16:29:06Z

I am adding a Travis test for OSX os. It should ease catching OSX issues.

majetideepak · 2017-08-16T17:59:30Z

Can you check if thread_local is supported by the platform, and then disable libhdfspp on those platforms?

jamesclampffer · 2017-08-18T14:14:29Z

@majetideepak Is there a way to get the OSX CI to use a generic LLVM/Clang install rather than the one that gets shipped with OSX?

Apparently they apply a patch to Clang to break thread_local, my understanding is they don't want to break the ABI if they ever get around to making builtin thread local specialized for their OS.

        .Case("cxx_thread_local",
-                 LangOpts.CPlusPlus11 && PP.getTargetInfo().isTLSSupported() &&
-                 !PP.getTargetInfo().getTriple().isOSDarwin())
+                 LangOpts.CPlusPlus11 && PP.getTargetInfo().isTLSSupported())

Otherwise this won't get coverage on OSX CI builds.

Edit: Once I have some time I'll write a shim on top of pthread TLS which will make this a non-issue but it might be a while before I have a chance. https://issues.apache.org/jira/browse/HDFS-10355

omalley · 2017-08-18T15:06:35Z

.. Owen

On Aug 18, 2017, at 07:14, jamesclampffer ***@***.***> wrote: @majetideepak Is there a way to get the OSX CI to use a generic LLVM/Clang install rather than the one that gets shipped with OSX?

Don't do that. If it doesn't compile with the "real" Mac OS compiler, you'll break my dev platform.

…

Apparently they apply a patch to Clang to break thread_local, my understanding is they don't want to break the ABI if they ever get around to making builtin thread local specialized for their OS. .Case("cxx_thread_local", - LangOpts.CPlusPlus11 && PP.getTargetInfo().isTLSSupported() && - !PP.getTargetInfo().getTriple().isOSDarwin()) + LangOpts.CPlusPlus11 && PP.getTargetInfo().isTLSSupported()) Otherwise this won't get coverage on OSX CI builds. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

AnatoliShein · 2017-08-21T13:21:36Z

Hi all! It looks like thread_local feature is by default supported by Xcode 8.0 (and above) which runs on OS X El Capitan (10.11.5) or newer. Is it possible to update the osx_image of the CI-run from "xcode6.4" to at least "xcode8.0"? For older versions we can just display a warning message and not build libhdfspp.

AnatoliShein · 2017-08-21T13:32:59Z

Looks like THIS is the list of available CI images for OS X. I will try osx_image: xcode8.3 now and see what happens.

omalley · 2017-08-29T16:54:08Z

There are a lot of warnings in libhdfs that clang reports.

It is currently failing in:
[ 91%] Linking CXX shared library libhdfspp.dylib

with link errors about sasl and protobuf.

AnatoliShein · 2017-08-29T17:53:50Z

Hi @omalley , the linking should be fixed with the new commit, and we will fix the warnings shortly.

AnatoliShein · 2017-09-07T12:30:38Z

Hi guys, I now fixed all the build errors and warnings for OS X (with Xcode 8+), and I would like some additional feedback on this pull request if you have any. Thanks!
(@omalley , @majetideepak , @xndai, @jamesclampffer )

xndai · 2017-09-11T09:14:39Z

c++/src/OrcHdfsFile.cc

+      }
+
+      hdfs::FileHandle *file_raw = nullptr;
+      status = file_system->Open(uri.get_path(), &file_raw);


nit: you may want to cache "status" within the class.

I am not quite sure why we will want to cache "status", since it is just used for the return values of HDFS operations (success or error message). Could you please explain?

My feeling is that you may want to return other meta info in the future. But it's your call.

xndai · 2017-09-11T09:14:52Z

c++/src/OrcHdfsFile.cc

+    }
+
+    uint64_t getNaturalReadSize() const override {
+      return 1048576; //1 MB


nit: Use const variable?

This sounds good, I will update this.

xndai · 2017-09-11T09:21:45Z

c++/include/orc/OrcFile.hh


  /**
-   * Create a stream to a local file.
+   * Create a stream to a local file or HDFS file if path begins with "hdfs://"


why do you want readLocalFile() to handle hdfs files? You already have readHdfsFile() which does exactly that.

Yes, I guess this is a little confusing. Currently I am calling readHdfsFile from readLocalFile if it starts with "hdfs://". To fix this I will add a new function readFile which will call either readHdfsFile or readLocalFile.

…s not built

xndai · 2017-09-13T06:12:55Z

LGTM

AnatoliShein · 2017-09-20T14:29:23Z

Now ORC can be built without LIBHDFSPP like this:
cmake -DBUILD_LIBHDFSPP=off ..

omalley · 2017-10-19T14:56:06Z

Ok, this patch breaks the docker scripts on at least centos7, debian8, and ubuntu16 by adding new dependencies for the c++ build. I'm going through and fixing them.

gxkevin · 2020-10-09T09:43:00Z

my hdfs path just like this: hdfs://yycluster02/hive_warehouse/rs_hago_client.db/peta/test/orc_test/my-file.orc

but, I got the error
'''
[ERROR ][Async Runtime ][Fri Oct 9 17:05:09 2020][Thread id = 140438677890880].
[/home/workspace/online_code/orc/build/libhdfspp_ep-prefix/src/libhdfspp_ep/lib/common/namenode_info.cc:170] Unabled to resolve endpoints for host: yycluster02 port: -1
[ERROR ][RPC ][Fri Oct 9 17:05:09 2020][Thread id = 140438677890880].
[/home/workspace/online_code/orc/build/libhdfspp_ep-prefix/src/libhdfspp_ep/lib/rpc/rpc_engine.cc:191] RpcEngine::AsyncRpcCommsError called; status="No endpoints provided" conn=0xf07340 reqs=1
[WARN ][RPC ][Fri Oct 9 17:05:09 2020][Thread id = 140438651348736].
[/home/workspace/online_code/orc/build/libhdfspp_ep-prefix/src/libhdfspp_ep/lib/rpc/rpc_engine.cc:202] RpcEngine::RpcCommsError called; status="No endpoints provided" conn=0xf07340 reqs=1
Can't connect to yycluster02:. No endpoints provided
'''

use it like this:
ORC_UNIQUE_PTR reader;
ReaderOptions options;
reader = createReader(readFile(file_path), options);

### What changes were proposed in this pull request? Mark readHdfsFile as deprecated. ### Why are the changes needed? Reading ORC on HDFS was introduced in #134 without any test. It has not been updated for 7 years and updating libhdfspp will result in extra dependency like boost library. Staying at an old version of libhdfspp will also prohibit us from updating other libraries like protobuf. ### How was this patch tested? It does not need test. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #1885 from wgtmac/ORC-1669. Authored-by: Gang Wu <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

### What changes were proposed in this pull request? Mark readHdfsFile as deprecated. ### Why are the changes needed? Reading ORC on HDFS was introduced in #134 without any test. It has not been updated for 7 years and updating libhdfspp will result in extra dependency like boost library. Staying at an old version of libhdfspp will also prohibit us from updating other libraries like protobuf. ### How was this patch tested? It does not need test. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #1885 from wgtmac/ORC-1669. Authored-by: Gang Wu <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit cea0629) Signed-off-by: Dongjoon Hyun <[email protected]>

xndai reviewed Jun 22, 2017

View reviewed changes

xndai reviewed Jun 23, 2017

View reviewed changes

xndai reviewed Jul 5, 2017

View reviewed changes

jamesclampffer reviewed Jul 6, 2017

View reviewed changes

AnatoliShein force-pushed the ORC-17 branch from 4e28adf to ebd823c Compare July 31, 2017 15:08

squashed all commits

817fe46

AnatoliShein force-pushed the ORC-17 branch from e91b43b to 817fe46 Compare August 30, 2017 15:17

Anatoli Shein added 3 commits August 30, 2017 11:22

fix for CI script

f90798d

fix 2 for CI script

36ab858

fix 3 for CI script

1adb233

xndai reviewed Sep 11, 2017

View reviewed changes

fixed function names and a const value, added warning if Libhdfspp wa…

36a4c35

…s not built

added variable BUILD_LIBHDFSPP

32270c6

asfgit closed this in 5831033 Oct 19, 2017

wgtmac mentioned this pull request Apr 10, 2024

ORC-1669: [C++] Deprecate HDFS support #1885

Closed


		namespace orc {

		class HdfsFileInputStream : public InputStream {

Orc 17 #134

Orc 17 #134

Uh oh!

Conversation

AnatoliShein commented Jun 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majetideepak commented Jul 7, 2017

Uh oh!

omalley commented Aug 7, 2017

Uh oh!

AnatoliShein commented Aug 7, 2017

Uh oh!

AnatoliShein commented Aug 7, 2017

Uh oh!

omalley commented Aug 14, 2017

Uh oh!

AnatoliShein commented Aug 15, 2017

Uh oh!

omalley commented Aug 15, 2017

Uh oh!

jamesclampffer commented Aug 15, 2017

Uh oh!

majetideepak commented Aug 15, 2017

Uh oh!

majetideepak commented Aug 16, 2017

Uh oh!

jamesclampffer commented Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omalley commented Aug 18, 2017 via email

Uh oh!

AnatoliShein commented Aug 21, 2017

Uh oh!

AnatoliShein commented Aug 21, 2017

Uh oh!

omalley commented Aug 29, 2017

Uh oh!

AnatoliShein commented Aug 29, 2017

Uh oh!

AnatoliShein commented Sep 7, 2017

jamesclampffer commented Aug 18, 2017 •

edited

Loading

gxkevin commented Oct 9, 2020 •

edited

Loading