Support jvm version libhdfs in velox #9835

JkSelf · 2024-05-16T02:43:07Z

Currently, Gluten will throw hdfs connection failures when executing queries if their HDFS system employs Kerberos authentication and Viewfs support. This is due to the fact that the existing libhdfs3 API does not support Kerberos authentication, whereas the JVM version of libhdfs is capable of invoking APIs that support Kerberos authentication. If the user's system has the HADOOP_HOME environment variable set, the JVM version of libhdfs will be used during the compilation of Gluten; if not set, the default libhdfs3 will be used instead.

netlify · 2024-05-16T02:43:24Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`db2f658`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/67122fb37d074f00080ebc96

JkSelf · 2024-05-16T02:43:30Z

@mbasmanova @majetideepak Can you help to review? Thanks.

mbasmanova · 2024-05-16T09:43:11Z

@JkSelf CI is red. Please, take a look.

jinchengchenghh · 2024-05-16T12:12:47Z

CMakeLists.txt

+    find_library(
+      LIBJVM
+      NAMES libjvm.so
+      HINTS "${JAVA_HOME}/jre/lib/amd64/server/" REQUIRED)


Does the java path is same for all the Velox supported platform? Mac?

jinchengchenghh · 2024-05-16T12:16:37Z

velox/connectors/hive/storage_adapters/hdfs/CMakeLists.txt

+                        arrow)
+  target_include_directories(velox_hdfs PRIVATE ${HADOOP_HOME}/include/)
+  if(${VELOX_BUILD_TESTING})
+    add_subdirectory(tests)


Do we may add_subdirectory(tests) twice when both VELOX_ENABLE_HDFS and VELOX_ENABLE_HDFS3 is ON

jinchengchenghh · 2024-05-16T12:18:53Z

velox/connectors/hive/storage_adapters/hdfs/HdfsFileSystem.cpp

@@ -31,12 +35,17 @@ class HdfsFileSystem::Impl {
    hdfsBuilderSetNameNode(builder, endpoint.host.c_str());
    hdfsBuilderSetNameNodePort(builder, atoi(endpoint.port.data()));
    hdfsClient_ = hdfsBuilderConnect(builder);
+#ifdef VELOX_ENABLE_HDFS3


Which definition has high priority? Do we need to check this tow definitions conflict?

jinchengchenghh · 2024-05-16T12:19:46Z

velox/connectors/hive/storage_adapters/hdfs/HdfsReadFile.cpp

+#elif VELOX_ENABLE_HDFS
+
+    auto errMsg =
+        fmt::format("Unable to get file path info for file: {}", filePath_);


Use this format VELOX_FAIL("Write past Buffer capacity() {}", capacity_);

jinchengchenghh · 2024-05-16T12:21:14Z

velox/connectors/hive/storage_adapters/hdfs/HdfsReadFile.h

@@ -33,19 +37,28 @@ struct HdfsFile {
  void open(hdfsFS client, const std::string& path) {
    client_ = client;
    handle_ = hdfsOpenFile(client, path.data(), O_RDONLY, 0, 0, 0);
+#ifdef VELOX_ENABLE_HDFS3
    VELOX_CHECK_NOT_NULL(
        handle_,
        "Unable to open file {}. got error: {}",
        path,
        hdfsGetLastError());


Can we just check `` in hdfsGetLastError, return "" or "Can not get error message when VELOX_ENABLE_HDFS3" when #ifdef VELOX_ENABLE_HDFS3, then we don't need to modify so much code

jinchengchenghh · 2024-05-16T12:23:08Z

velox/connectors/hive/storage_adapters/hdfs/CMakeLists.txt

@@ -19,8 +19,18 @@ add_library(velox_hdfs RegisterHdfsFileSystem.cpp)
 if(VELOX_ENABLE_HDFS)
  target_sources(velox_hdfs PRIVATE HdfsFileSystem.cpp HdfsReadFile.cpp
                                    HdfsWriteFile.cpp)
-  target_link_libraries(velox_hdfs Folly::folly ${LIBHDFS3} xsimd)
+  target_link_libraries(velox_hdfs Folly::folly ${LIBHDFS} ${LIBJVM} xsimd
+                        arrow)


Why need to link arrow?

assignUser · 2024-05-22T18:00:34Z

The C++ libhdfs3 is unmaintained (no commits in 3+ years, hawq is moving into the attic (asf 'archive')) so I think this should replace it entirely! @pedroerp @kgpai we talked about this before, I think this is what arrow does as well but I haven't had a look yet.

kgpai · 2024-05-22T18:48:21Z

cc: @majetideepak

majetideepak · 2024-05-23T07:30:27Z

It seems Arrow's HDFS filesystem depends on some hdfs dynamic library to be present in the system and dynamically links against it. I see this PR does something similar as well.
https://github.com/apache/arrow/blob/main/cpp/src/arrow/io/hdfs_internal.cc#L114

get_potential_libhdfs_paths()
get_potential_libjvm_paths()

majetideepak · 2024-05-23T07:37:32Z

We do dynamically link against LIBHDFS3(hawk) as well. It should not hurt to support it?
We should avoid the multiple ifdefs in this PR.

assignUser · 2024-05-23T16:34:37Z

It should not hurt to support it?

Well it is unmaintained so it should'nt be the default and we should probably mark it deprecated and remove it in the future.

majetideepak · 2024-05-24T00:01:12Z

Well it is unmaintained so it should'nt be the default and we should probably mark it deprecated and remove it in the future.

Switching the default makes sense since hawq is not maintained. But I wonder if the hawq implementation provides additional benefits (say performance) compared to the Java implementation.

JkSelf · 2024-05-27T10:02:17Z

It seems Arrow's HDFS filesystem depends on some hdfs dynamic library to be present in the system and dynamically links against it. I see this PR does something similar as well. https://github.com/apache/arrow/blob/main/cpp/src/arrow/io/hdfs_internal.cc#L114
get_potential_libhdfs_paths()
get_potential_libjvm_paths()

@majetideepak Yes, that makes sense to me. If we proceed this way, we can avoid the need to ensure that the user's system has HDFS and JVM installed during the build phase.

JkSelf · 2024-05-27T10:12:28Z

Well it is unmaintained so it should'nt be the default and we should probably mark it deprecated and remove it in the future.

Switching the default makes sense since hawq is not maintained. But I wonder if the hawq implementation provides additional benefits (say performance) compared to the Java implementation.

In terms of performance, hawq implementation is generally considered to be superior to jvm libhdfs because it is designed for enhanced performance and reduces the additional overhead caused by JNI. However, functionality is equally important to consider. Currently, hawq implementation does not meet user needs in certain functional aspects, such as lacking support for Kerberos authentication and ViewFs.
If there is a preference to retain hawq implementation for its performance, we could offer two compilation options, VELOX_ENABLE_HDFS and VELOX_ENABLE_HDFS3, to support jvm libhdfs and hawq implementation respectively. However, maintaining both options might necessitate the use of multiple ifdef statements to determine which hdfs class to load.
@majetideepak @assignUser What do you think? Thanks.

assignUser · 2024-05-29T03:16:30Z

In terms of performance, hawq implementation is generally considered to be superior t

While better performance is of course great, in this case it comes with the considerable drawback that the library in question has been unmaintained for several years and the parent project is currently in the process of being 'archived' as well. My vote would be to remove it entirely but I am not a user so I'll let others decide :)

JkSelf · 2024-05-29T05:20:32Z

@assignUser Agree to remove hawa implementation entirely. @majetideepak What do you think?

majetideepak · 2024-05-29T06:11:34Z

Agree to remove hawa implementation entirely.

I am not a user as well :). Let's open a discussion on this and get input from the wider community.

JkSelf · 2024-05-29T07:00:58Z

@assignUser @majetideepak Thank you for your response. I have initiated a poll in the community at #9969. Your participation and vote would be greatly appreciated.

leoluan2009 · 2024-06-24T07:18:53Z

CMakeLists.txt

-  find_library(
-    LIBHDFS3
-    NAMES libhdfs3.so libhdfs3.dylib
-    HINTS "${CMAKE_SOURCE_DIR}/hawq/depends/libhdfs3/_build/src/" REQUIRED)
  add_definitions(-DVELOX_ENABLE_HDFS3)


use add_definitions(-DVELOX_ENABLE_HDFS)?

FelixYBW · 2024-08-13T04:48:27Z

@JkSelf can you resolve the conflict?

wForget · 2024-09-02T09:41:55Z

@JkSelf Thank you for your work. Is there any progress? It would be a really nice feature for me to replace with libhdfs so that we can support more file systems based on Hadoop FileSystem.

velox/connectors/hive/storage_adapters/hdfs/tests/CMakeLists.txt

JkSelf · 2024-09-25T07:40:27Z

@majetideepak I resolved all your comments. Can you help to review again? Thanks.

majetideepak

@JkSelf some comments.

velox/connectors/hive/storage_adapters/hdfs/HdfsFileSystem.h

velox/connectors/hive/storage_adapters/hdfs/HdfsReadFile.h

velox/connectors/hive/storage_adapters/hdfs/HdfsWriteFile.h

velox/connectors/hive/storage_adapters/hdfs/tests/CMakeLists.txt

JkSelf · 2024-10-10T02:46:51Z

@majetideepak Thanks for your review. I have resolved all your comments. Can you help to review again. Thanks.

majetideepak

@JkSelf two more comments. Thanks for the changes. The hdfs headers are cleaner now.

velox/connectors/hive/storage_adapters/hdfs/HdfsReadFile.cpp

velox/connectors/hive/storage_adapters/hdfs/tests/HdfsFileSystemTest.cpp

JkSelf · 2024-10-11T01:52:18Z

@majetideepak I have resolved all your comments. Can you help to review again? Thanks.

majetideepak

@JkSelf one last comment.

majetideepak · 2024-10-11T15:09:25Z

velox/connectors/hive/storage_adapters/hdfs/tests/HdfsFileSystemTest.cpp

+  // This prevents a scenario where the first pread call in the
+  // checkReadErrorMessages method might succeed, while a subsequent call fails,
+  // leading to a mismatch in readFailErrorMessage.
+  sleep(10);


@JkSelf Why do we need to add this sleep now which was previously not present?
Instead of sleep, can we query if the miniCluster is down?
sleep is usually a recipe for non-deterministic failures and wasted CI time.

@majetideepak Yes. Updated. Can you help to review again? Thanks.

majetideepak

@JkSelf Some comments and Linux adapters CI is failing. Can you check? Thanks.

velox/connectors/hive/storage_adapters/hdfs/tests/HdfsFileSystemTest.cpp

JkSelf · 2024-10-15T06:57:01Z

@majetideepak Can you help to review again? Thanks.

velox/connectors/hive/storage_adapters/hdfs/tests/HdfsFileSystemTest.cpp

majetideepak

Thanks @JkSelf! Appreciate for addressing all the feedback.

majetideepak · 2024-10-17T09:51:37Z

I will add the merge label once CI is passing.

assignUser · 2024-10-18T13:51:29Z

CMake/resolve_dependency_modules/curl.cmake

+  CMAKE_ARGS -DCMAKE_USE_OPENSSL=ON -DOPENSSL_ROOT_DIR=/usr/local/opt/openssl
+  -DOPENSSL_LIBRARIES=/usr/local/opt/openssl/lib)


No hard coding of paths please. Does curl cmake not find openssl on it's own?

assignUser · 2024-10-18T13:53:38Z

velox/connectors/hive/storage_adapters/hdfs/tests/CMakeLists.txt

+  velox_hdfs_insert_test
+  velox_file
+  velox_hdfs
+  velox_core
+  velox_exec_test_lib
+  velox_hive_connector
+  velox_dwio_common_exception
+  velox_exec


nit: I only see folly, hdfs, gtest and exec being used. Please only link used libs.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 16, 2024

JkSelf force-pushed the libhdfs branch 3 times, most recently from fbb5763 to a33070c Compare May 16, 2024 03:39

mbasmanova requested review from assignUser and majetideepak May 16, 2024 09:42

jinchengchenghh reviewed May 16, 2024

View reviewed changes

JkSelf force-pushed the libhdfs branch from a33070c to bc84e4f Compare June 20, 2024 03:02

leoluan2009 reviewed Jun 24, 2024

View reviewed changes

JkSelf force-pushed the libhdfs branch from ce16b7a to 733e388 Compare July 9, 2024 05:35

JkSelf mentioned this pull request Jul 11, 2024

Upgrade hadoop version and set java/hadoop home in docker image #10446

Closed

JkSelf mentioned this pull request Aug 12, 2024

[GLUTEN-5103][VL] Use jvm libhdfs replace c++ libhdfs3 apache/incubator-gluten#6172

Open

JkSelf force-pushed the libhdfs branch from 60289eb to 8c446dc Compare August 14, 2024 07:07

JkSelf force-pushed the libhdfs branch 4 times, most recently from e7e3f29 to a77234d Compare September 25, 2024 07:09

JkSelf commented Sep 25, 2024

View reviewed changes

velox/connectors/hive/storage_adapters/hdfs/tests/CMakeLists.txt Show resolved Hide resolved

majetideepak reviewed Sep 25, 2024

View reviewed changes

JkSelf force-pushed the libhdfs branch from 80ae0ad to 4cbdc28 Compare September 29, 2024 01:19

JkSelf force-pushed the libhdfs branch 3 times, most recently from 2dac11f to 701cbec Compare October 10, 2024 02:38

majetideepak reviewed Oct 10, 2024

View reviewed changes

velox/connectors/hive/storage_adapters/hdfs/HdfsReadFile.cpp Outdated Show resolved Hide resolved

velox/connectors/hive/storage_adapters/hdfs/tests/HdfsFileSystemTest.cpp Outdated Show resolved Hide resolved

JkSelf force-pushed the libhdfs branch from 701cbec to a9954f8 Compare October 11, 2024 01:51

majetideepak reviewed Oct 11, 2024

View reviewed changes

JkSelf force-pushed the libhdfs branch from ff87756 to fb7cfa7 Compare October 14, 2024 02:19

majetideepak reviewed Oct 14, 2024

View reviewed changes

velox/connectors/hive/storage_adapters/hdfs/tests/HdfsFileSystemTest.cpp Show resolved Hide resolved

velox/connectors/hive/storage_adapters/hdfs/tests/HdfsFileSystemTest.cpp Show resolved Hide resolved

JkSelf force-pushed the libhdfs branch 3 times, most recently from efbe37b to 3f87095 Compare October 15, 2024 05:31

majetideepak reviewed Oct 15, 2024

View reviewed changes

velox/connectors/hive/storage_adapters/hdfs/tests/HdfsFileSystemTest.cpp Outdated Show resolved Hide resolved

Add jvm libhdfs support

3f87095

majetideepak approved these changes Oct 17, 2024

View reviewed changes

Resolve comments

1a74ab2

assignUser requested changes Oct 18, 2024

View reviewed changes

JkSelf added 2 commits October 19, 2024 00:48

test

34699b7

tmp

db2f658

		CMAKE_ARGS -DCMAKE_USE_OPENSSL=ON -DOPENSSL_ROOT_DIR=/usr/local/opt/openssl
		-DOPENSSL_LIBRARIES=/usr/local/opt/openssl/lib)

Support jvm version libhdfs in velox #9835

Are you sure you want to change the base?

Support jvm version libhdfs in velox #9835

Conversation

JkSelf commented May 16, 2024 • edited Loading

netlify bot commented May 16, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

JkSelf commented May 16, 2024

mbasmanova commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

assignUser commented May 22, 2024

kgpai commented May 22, 2024

majetideepak commented May 23, 2024 • edited Loading

majetideepak commented May 23, 2024

assignUser commented May 23, 2024

majetideepak commented May 24, 2024

JkSelf commented May 27, 2024

JkSelf commented May 27, 2024

assignUser commented May 29, 2024

JkSelf commented May 29, 2024

majetideepak commented May 29, 2024

JkSelf commented May 29, 2024

Choose a reason for hiding this comment

FelixYBW commented Aug 13, 2024

wForget commented Sep 2, 2024

JkSelf commented Sep 25, 2024

majetideepak left a comment

Choose a reason for hiding this comment

JkSelf commented Oct 10, 2024

majetideepak left a comment

Choose a reason for hiding this comment

JkSelf commented Oct 11, 2024

majetideepak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak left a comment • edited Loading

Choose a reason for hiding this comment

JkSelf commented Oct 15, 2024

majetideepak left a comment

Choose a reason for hiding this comment

majetideepak commented Oct 17, 2024

assignUser Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

assignUser Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

JkSelf commented May 16, 2024 •

edited

Loading

netlify bot commented May 16, 2024 •

edited

Loading

majetideepak commented May 23, 2024 •

edited

Loading

majetideepak left a comment •

edited

Loading

assignUser Oct 18, 2024 •

edited

Loading

assignUser Oct 18, 2024 •

edited

Loading