Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/24838 )
Change subject: IMPALA-15145: Propagate credential metadata to Scan Nodes / Execution Plan ...................................................................... Patch Set 5: Code-Review+1 (8 comments) Left a few smaller comments, otherwise LGTM! http://gerrit.cloudera.org:8080/#/c/24838/5/be/src/runtime/CMakeLists.txt File be/src/runtime/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/24838/5/be/src/runtime/CMakeLists.txt@54 PS5, Line 54: query-credentials.cc nit: not in alphabetic order http://gerrit.cloudera.org:8080/#/c/24838/5/be/src/runtime/hdfs-fs-cache.h File be/src/runtime/hdfs-fs-cache.h: http://gerrit.cloudera.org:8080/#/c/24838/5/be/src/runtime/hdfs-fs-cache.h@100 PS5, Line 100: HdfsFsCache(HdfsFsCache const& src); : HdfsFsCache& operator=(HdfsFsCache const& rhs); Do you plan to define these in follow-up patches? If not, "= delete" could be added. http://gerrit.cloudera.org:8080/#/c/24838/5/be/src/runtime/hdfs-fs-cache.cc File be/src/runtime/hdfs-fs-cache.cc: http://gerrit.cloudera.org:8080/#/c/24838/5/be/src/runtime/hdfs-fs-cache.cc@126 PS5, Line 126: hdfsBuilder* hdfs_builder = hdfsNewBuilder(); : hdfsBuilderSetNameNode(hdfs_builder, namenode.c_str()); : if (cred_props != nullptr || has_options) { : // Use a new instance of the filesystem object to be sure that it picks up the : // configuration changes we're going to make. Without this call, a cached : // filesystem object is used which is unaffected by calls to : // hdfsBuilderConfSetStr(). This is unexpected behavior in the HDFS API, but is : // unlikely to change. : hdfsBuilderSetForceNewInstance(hdfs_builder); : if (cred_props != nullptr) { : for (const auto& kv : *cred_props) { : hdfsBuilderConfSetStr(hdfs_builder, kv.first.c_str(), kv.second.c_str()); : } : } : if (has_options) { : for (const auto& kv : *options) { : hdfsBuilderConfSetStr(hdfs_builder, kv.first.c_str(), kv.second.c_str()); : } : } : VLOG(1) << "Building hdfsFS for namenode '" << namenode << "' credential_prefix='" : << (entry != nullptr ? entry->prefix : "") : << "' num_options=" << (has_options ? options->size() : 0); : } : *fs = hdfsBuilderConnect(hdfs_builder); : if (*fs == NULL) { : return Status(GetHdfsErrorMsg("Failed to connect to FS: ", namenode)); : } Can we do this part without holding the mutex? http://gerrit.cloudera.org:8080/#/c/24838/5/be/src/runtime/hdfs-fs-cache.cc@153 PS5, Line 153: fs_map_.insert(make_pair(cache_key, *fs)); Any plan to remove old entries? fs_map_ now can grow indefinitely. http://gerrit.cloudera.org:8080/#/c/24838/5/be/src/runtime/query-credentials.h File be/src/runtime/query-credentials.h: http://gerrit.cloudera.org:8080/#/c/24838/5/be/src/runtime/query-credentials.h@58 PS5, Line 58: a proper prefix sorts before its extensions isn't it the opposite? http://gerrit.cloudera.org:8080/#/c/24838/5/be/src/runtime/query-credentials.h@61 PS5, Line 61: using is_transparent = void; Unused? http://gerrit.cloudera.org:8080/#/c/24838/5/fe/src/main/java/org/apache/impala/catalog/iceberg/VendedCredentialsFileIO.java File fe/src/main/java/org/apache/impala/catalog/iceberg/VendedCredentialsFileIO.java: http://gerrit.cloudera.org:8080/#/c/24838/5/fe/src/main/java/org/apache/impala/catalog/iceberg/VendedCredentialsFileIO.java@90 PS5, Line 90: 0 Should we have a max expiryMs, and use it for 0? E.g. we could expiry such creds after 3 hours. http://gerrit.cloudera.org:8080/#/c/24838/5/fe/src/main/java/org/apache/impala/catalog/iceberg/VendedCredentialsFileIO.java@301 PS5, Line 301: Closes and drops cached FileSystems whose credential expired a while ago. What happens when we get new credentials for the same prefix, but expiry=0? -- To view, visit http://gerrit.cloudera.org:8080/24838 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I73592a1521f29374316ed001341f956b40f9c0d5 Gerrit-Change-Number: 24838 Gerrit-PatchSet: 5 Gerrit-Owner: Peter Rozsa <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 28 Sep 2026 13:53:28 +0000 Gerrit-HasComments: Yes
