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	`9c058b0`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/67184d321be84b0008529d2a

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.

CMakeLists.txt

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

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

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

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

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

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.

assignUser

LGTM now.

majetideepak

@JkSelf one minor comment. I will add the merge label once that is fixed.

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

JkSelf · 2024-10-23T08:49:50Z

@majetideepak Please help to add the ready to merge label. Thanks.

facebook-github-bot · 2024-10-24T00:02:15Z

@pedroerp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-10-24T17:12:00Z

@pedroerp merged this pull request in 10cdf6f.

conbench-facebook · 2024-10-24T17:40:55Z

Conbench analyzed the 1 benchmark run on commit 10cdf6fd.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: 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. Pull Request resolved: facebookincubator#9835 Reviewed By: xiaoxmeng, DanielHunte Differential Revision: D64872596 Pulled By: pedroerp fbshipit-source-id: 995ee73a9f8474f8a6467b926a56246073fee75e

Yohahaha · 2024-10-29T03:53:01Z

velox/external/hdfs/CMakeLists.txt

+  velox_link_libraries(
+        velox_external_hdfs
+        PRIVATE
+        arrow)


why use PRIVATE?
If arrow has already installed in non-system path, and be included with CMAKE_PREFIX_PATH, velox_hdfs target can not found arrow header files.

# PRIVATE /usr/lib64/ccache/c++ -I/root/oap-velox/. -I/root/oap-velox/velox/external/xxhash -isystem /root/oap-velox/velox -isystem /root/oap-velox/velox/external -isystem /usr/include/libdwarf -isystem /root/oap-velox/velox/external/date/velox/external -o velox/connectors/hive/storage_adapters/hdfs/CMakeFiles/velox_hdfs.dir/HdfsReadFile.cpp.o -c /root/oap-velox/velox/connectors/hive/storage_adapters/hdfs/HdfsReadFile.cpp # NON-PRIVATE /usr/lib64/ccache/c++ -I/root/oap-velox/. -I/root/oap-velox/velox/external/xxhash -isystem /root/oap-velox/velox -isystem /root/oap-velox/velox/external -isystem /opt/extra_lib/arrow_ep/include -isystem /root/apache-arrow/cpp/_build/thrift_ep-install/include -isystem /usr/include/libdwarf -isystem /root/oap-velox/velox/external/date/velox/external -o velox/connectors/hive/storage_adapters/hdfs/CMakeFiles/velox_hdfs.dir/HdfsReadFile.cpp.o -c /root/oap-velox/velox/connectors/hive/storage_adapters/hdfs/HdfsReadFile.cpp

@JkSelf @PHILO-HE

@Yohahaha, thanks for raising up this issue. The PRIVATE linkage will make the headers bound to target arrow not accessible to target velox_external_hdfs. If those headers are in system path, it is not an issue since compiler can always find them from system path. Otherwise, build error will occur. @JkSelf, if there is no special reason to use PRIVATE, could you change it to PUBLIC?

@Yohahaha @PHILO-HE Yes. I will file a pr to change to PUBLIC. Thanks.

Sorry I did not catch this. I saw the arrow headers in the implementation file and didn't check the header as I usually only include things that are used privately in the implementation and everything else in the header.

…hdfs (#11371) Summary: Fix #9835 (comment) Pull Request resolved: #11371 Reviewed By: Yuhta Differential Revision: D65420201 Pulled By: mbasmanova fbshipit-source-id: 4bf4fc418f61c246b4113960e499e054c6b7ece8

This reverts commit 10cdf6f.

boneanxs · 2024-11-21T12:42:38Z

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

@@ -15,7 +15,12 @@
 */
 #include "velox/common/file/FileSystems.h"

+namespace velox::filesystems::arrow::io::internal {


Is this change necessary? I can see in HdfsFileSystem.cpp, it includes ArrowHdfsInternal.h, which has LibHdfsShim already.

Btw, is the namespace correct here? Should be facebook::velox::filesystems::arrow::io::internal??

…in HdfsReadFile (#11580) Summary: To address this [comment](#9835 (comment)), we moved the thread local parameter `folly::ThreadLocal<HdfsFile> file_` from a member parameter to a template parameter in the `HdfsReadFile::preadInternal` method. The frequent initialization of the thread local parameter in the method caused about a 5% performance degradation. We reverted the thread local parameter to a member parameter in this PR. Pull Request resolved: #11580 Reviewed By: kgpai Differential Revision: D66376694 Pulled By: kagamiori fbshipit-source-id: ba639dd9bd52a81a71d2580d3af5259324a8c92a

…in HdfsReadFile (facebookincubator#11580) Summary: To address this [comment](facebookincubator#9835 (comment)), we moved the thread local parameter `folly::ThreadLocal<HdfsFile> file_` from a member parameter to a template parameter in the `HdfsReadFile::preadInternal` method. The frequent initialization of the thread local parameter in the method caused about a 5% performance degradation. We reverted the thread local parameter to a member parameter in this PR. Pull Request resolved: facebookincubator#11580 Reviewed By: kgpai Differential Revision: D66376694 Pulled By: kagamiori fbshipit-source-id: ba639dd9bd52a81a71d2580d3af5259324a8c92a

Summary: After merging PR #9835, Gluten encountered the following compilation error. This PR refactors the package namespace to resolve the ambiguity issue. ``` /mnt/DP_disk3/jk/projects/gluten/cpp/velox/memory/VeloxColumnarBatch.cc:33:5: error: reference to ‘velox’ is ambiguous 33 | velox::memory::MemoryPool* pool, | ^~~~~ In file included from /mnt/DP_disk3/jk/projects/gluten/cpp/velox/operators/writer/VeloxParquetDataSource.h:43, from /mnt/DP_disk3/jk/projects/gluten/cpp/velox/compute/VeloxRuntime.h:25, from /mnt/DP_disk3/jk/projects/gluten/cpp/velox/memory/VeloxColumnarBatch.cc:18: /mnt/DP_disk3/jk/projects/gluten/dev/../ep/build-velox/build/velox_ep/velox/connectors/hive/storage_adapters/hdfs/HdfsFileSystem.h:18:11: note: candidates are: ‘namespace velox { }’ 18 | namespace velox::filesystems::arrow::io::internal { | ^~~~~ In file included from /mnt/DP_disk3/jk/projects/gluten/dev/../ep/build-velox/build/velox_ep/velox/common/base/VeloxException.h:29, from /mnt/DP_disk3/jk/projects/gluten/dev/../ep/build-velox/build/velox_ep/velox/common/base/Exceptions.h:27, from /mnt/DP_disk3/jk/projects/gluten/dev/../ep/build-velox/build/velox_ep/velox/common/base/BitUtil.h:19, from /mnt/DP_disk3/jk/projects/gluten/dev/../ep/build-velox/build/velox_ep/velox/common/caching/AsyncDataCache.h:27, from /mnt/DP_disk3/jk/projects/gluten/cpp/velox/compute/VeloxBackend.h:27, from /mnt/DP_disk3/jk/projects/gluten/cpp/velox/memory/VeloxMemoryManager.h:20, from /mnt/DP_disk3/jk/projects/gluten/cpp/velox/memory/VeloxColumnarBatch.h:21, from /mnt/DP_disk3/jk/projects/gluten/cpp/velox/memory/VeloxColumnarBatch.cc:17: ``` Pull Request resolved: #11585 Reviewed By: xiaoxmeng Differential Revision: D67119176 Pulled By: kgpai fbshipit-source-id: d8eab6f8570ffa7b35bdc1b890c658f6926b8313

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

Merged

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

assignUser approved these changes Oct 22, 2024

View reviewed changes

majetideepak approved these changes Oct 22, 2024

View reviewed changes

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

JkSelf force-pushed the libhdfs branch from a3f5a01 to 9c058b0 Compare October 23, 2024 01:11

JkSelf added 2 commits October 23, 2024 16:09

Add jvm libhdfs support

ad55b61

Resolve comments

9c058b0

majetideepak approved these changes Oct 23, 2024

View reviewed changes

majetideepak added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Oct 23, 2024

facebook-github-bot closed this in 10cdf6f Oct 24, 2024

facebook-github-bot added the Merged label Oct 24, 2024

Yohahaha reviewed Oct 29, 2024

View reviewed changes

This was referenced Oct 29, 2024

Change the arrow dependency from private to public in velox_external_hdfs #11371

Closed

[VL] Track Jvm based libhdfs performance gap issue apache/incubator-gluten#7746

Closed

JkSelf added a commit to JkSelf/velox that referenced this pull request Nov 14, 2024

Revert "Support jvm version libhdfs in velox (facebookincubator#9835)"

3c10247

This reverts commit 10cdf6f.

JkSelf added a commit to JkSelf/velox that referenced this pull request Nov 15, 2024

Revert "Support jvm version libhdfs in velox (facebookincubator#9835)"

2a23b1d

This reverts commit 10cdf6f.

JkSelf added a commit to JkSelf/velox that referenced this pull request Nov 18, 2024

Revert "Support jvm version libhdfs in velox (facebookincubator#9835)"

f1333af

This reverts commit 10cdf6f.

JkSelf added a commit to JkSelf/velox that referenced this pull request Nov 18, 2024

Revert "Support jvm version libhdfs in velox (facebookincubator#9835)"

52ea30d

This reverts commit 10cdf6f.

This was referenced Nov 19, 2024

fix: Move the thread local parameter from method to member parameter in HdfsReadFile #11580

Closed

fix: Refactor package namespace in HdfsFileSystem #11585

Closed

boneanxs reviewed Nov 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support jvm version libhdfs in velox #9835

Support jvm version libhdfs in velox #9835

JkSelf commented May 16, 2024 •

edited

Loading

netlify bot commented May 16, 2024 •

edited

Loading

JkSelf commented May 16, 2024

mbasmanova commented May 16, 2024

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

leoluan2009 Jun 24, 2024

FelixYBW commented Aug 13, 2024

wForget commented Sep 2, 2024

assignUser left a comment

majetideepak left a comment

JkSelf commented Oct 23, 2024

facebook-github-bot commented Oct 24, 2024

facebook-github-bot commented Oct 24, 2024

conbench-facebook bot commented Oct 24, 2024

Yohahaha Oct 29, 2024

PHILO-HE Oct 29, 2024

JkSelf Oct 29, 2024

assignUser Oct 30, 2024

boneanxs Nov 21, 2024

Support jvm version libhdfs in velox #9835

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

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

leoluan2009 Jun 24, 2024

Choose a reason for hiding this comment

FelixYBW commented Aug 13, 2024

wForget commented Sep 2, 2024

assignUser left a comment

Choose a reason for hiding this comment

majetideepak left a comment

Choose a reason for hiding this comment

JkSelf commented Oct 23, 2024

facebook-github-bot commented Oct 24, 2024

facebook-github-bot commented Oct 24, 2024

conbench-facebook bot commented Oct 24, 2024

Yohahaha Oct 29, 2024

Choose a reason for hiding this comment

PHILO-HE Oct 29, 2024

Choose a reason for hiding this comment

JkSelf Oct 29, 2024

Choose a reason for hiding this comment

assignUser Oct 30, 2024

Choose a reason for hiding this comment

boneanxs Nov 21, 2024

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