github-actions[bot] commented on code in PR #33059: URL: https://github.com/apache/doris/pull/33059#discussion_r1544726383
########## cloud/src/recycler/hdfs_accessor.cpp: ########## @@ -0,0 +1,286 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "recycler/hdfs_accessor.h" + +#include <gen_cpp/cloud.pb.h> + +#include <string_view> + +#include "common/config.h" +#include "common/logging.h" + +namespace doris::cloud { +namespace { + +// Removes any leading, and trailing `c` +void strip(std::string& str, char c) { + if (!str.empty()) { + size_t start = str.find_first_not_of(c); + if (start == std::string::npos) { + str = ""; + } else { + size_t end = str.find_last_not_of(c); + if (start > 0 || end < str.size() - 1) { + str = str.substr(start, end - start + 1); + } + } + } +} + +std::string_view hdfs_error() { +#ifdef USE_HADOOP_HDFS + char* root_cause = hdfsGetLastExceptionRootCause(); + return root_cause ? root_cause : ""; +#else + return hdfsGetLastError(); +#endif +} + +} // namespace + +class HDFSBuilder { +public: + ~HDFSBuilder() { +#ifdef USE_LIBHDFS3 + // for hadoop hdfs, the `hdfs_builder_` will be freed in hdfsConnect + if (hdfs_builder_ != nullptr) { + hdfsFreeBuilder(hdfs_builder_); + } +#endif + } + + // TODO(plat1ko): template <class Params> + static hdfsFS create_fs(const HdfsBuildConf& params) { + HDFSBuilder builder; + int ret = builder.init_hdfs_builder(); + if (ret != 0) { + return nullptr; + } + + ret = builder.init(params); + if (ret != 0) { + hdfsFreeBuilder(builder.hdfs_builder_); + return nullptr; + } + + return hdfsBuilderConnect(builder.hdfs_builder_); + } + +private: + HDFSBuilder() = default; + + // returns 0 for success otherwise error + int init(const HdfsBuildConf& conf) { Review Comment: warning: method 'init' can be made static [readability-convert-member-functions-to-static] ```suggestion static int init(const HdfsBuildConf& conf) { ``` ########## cloud/src/recycler/hdfs_accessor.cpp: ########## @@ -0,0 +1,286 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "recycler/hdfs_accessor.h" + +#include <gen_cpp/cloud.pb.h> + +#include <string_view> + +#include "common/config.h" +#include "common/logging.h" + +namespace doris::cloud { +namespace { + +// Removes any leading, and trailing `c` +void strip(std::string& str, char c) { + if (!str.empty()) { + size_t start = str.find_first_not_of(c); + if (start == std::string::npos) { + str = ""; + } else { + size_t end = str.find_last_not_of(c); + if (start > 0 || end < str.size() - 1) { + str = str.substr(start, end - start + 1); + } + } + } +} + +std::string_view hdfs_error() { +#ifdef USE_HADOOP_HDFS + char* root_cause = hdfsGetLastExceptionRootCause(); + return root_cause ? root_cause : ""; +#else + return hdfsGetLastError(); +#endif +} + +} // namespace + +class HDFSBuilder { +public: + ~HDFSBuilder() { +#ifdef USE_LIBHDFS3 + // for hadoop hdfs, the `hdfs_builder_` will be freed in hdfsConnect + if (hdfs_builder_ != nullptr) { + hdfsFreeBuilder(hdfs_builder_); + } +#endif + } + + // TODO(plat1ko): template <class Params> + static hdfsFS create_fs(const HdfsBuildConf& params) { + HDFSBuilder builder; + int ret = builder.init_hdfs_builder(); + if (ret != 0) { + return nullptr; + } + + ret = builder.init(params); + if (ret != 0) { + hdfsFreeBuilder(builder.hdfs_builder_); + return nullptr; + } + + return hdfsBuilderConnect(builder.hdfs_builder_); + } + +private: + HDFSBuilder() = default; + + // returns 0 for success otherwise error + int init(const HdfsBuildConf& conf) { + DCHECK(hdfs_builder_); + hdfsBuilderSetNameNode(hdfs_builder_, conf.fs_name().c_str()); + // set kerberos conf + bool kerberos_login = false; + if (conf.has_hdfs_kerberos_keytab()) { + kerberos_login = true; +#ifdef USE_HADOOP_HDFS + hdfsBuilderSetKerb5Conf(hdfs_builder_, config::kerberos_krb5_conf_path.c_str()); + hdfsBuilderSetKeyTabFile(hdfs_builder_, conf.hdfs_kerberos_keytab().c_str()); +#endif + } + + if (conf.has_hdfs_kerberos_principal()) { + kerberos_login = true; + hdfsBuilderSetPrincipal(hdfs_builder_, conf.hdfs_kerberos_keytab().c_str()); + } else if (conf.has_user()) { + hdfsBuilderSetUserName(hdfs_builder_, conf.user().c_str()); +#ifdef USE_HADOOP_HDFS + hdfsBuilderSetKerb5Conf(hdfs_builder_, nullptr); + hdfsBuilderSetKeyTabFile(hdfs_builder_, nullptr); +#endif + } + + // set other conf + for (const auto& kv : conf.hdfs_confs()) { + hdfsBuilderConfSetStr(hdfs_builder_, kv.key().c_str(), kv.value().c_str()); +#ifdef USE_HADOOP_HDFS + // Set krb5.conf, we should define java.security.krb5.conf in catalog properties + if (kv.key() == "java.security.krb5.conf") { + hdfsBuilderSetKerb5Conf(hdfs_builder_, kv.value().c_str()); + } +#endif + } + + if (kerberos_login) { + int ret = check_krb_params(conf.hdfs_kerberos_principal(), conf.hdfs_kerberos_keytab()); + if (ret != 0) { + return ret; + } + } + + hdfsBuilderConfSetStr(hdfs_builder_, "ipc.client.fallback-to-simple-auth-allowed", "true"); + return 0; + } + + // returns 0 for success otherwise error + int init_hdfs_builder() { Review Comment: warning: method 'init_hdfs_builder' can be made static [readability-convert-member-functions-to-static] ```suggestion static int init_hdfs_builder() { ``` ########## cloud/src/recycler/hdfs_accessor.h: ########## @@ -0,0 +1,75 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#pragma once + +#ifdef USE_HADOOP_HDFS +#include <hadoop_hdfs/hdfs.h> // IWYU pragma: export +#else +#include <hdfs/hdfs.h> // IWYU pragma: export Review Comment: warning: 'hdfs/hdfs.h' file not found [clang-diagnostic-error] ```cpp #include <hdfs/hdfs.h> // IWYU pragma: export ^ ``` ########## cloud/src/recycler/hdfs_accessor.cpp: ########## @@ -0,0 +1,286 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "recycler/hdfs_accessor.h" + +#include <gen_cpp/cloud.pb.h> + +#include <string_view> + +#include "common/config.h" +#include "common/logging.h" + +namespace doris::cloud { +namespace { + +// Removes any leading, and trailing `c` +void strip(std::string& str, char c) { + if (!str.empty()) { + size_t start = str.find_first_not_of(c); + if (start == std::string::npos) { + str = ""; + } else { + size_t end = str.find_last_not_of(c); + if (start > 0 || end < str.size() - 1) { + str = str.substr(start, end - start + 1); + } + } + } +} + +std::string_view hdfs_error() { +#ifdef USE_HADOOP_HDFS + char* root_cause = hdfsGetLastExceptionRootCause(); + return root_cause ? root_cause : ""; +#else + return hdfsGetLastError(); +#endif +} + +} // namespace + +class HDFSBuilder { +public: + ~HDFSBuilder() { +#ifdef USE_LIBHDFS3 + // for hadoop hdfs, the `hdfs_builder_` will be freed in hdfsConnect + if (hdfs_builder_ != nullptr) { + hdfsFreeBuilder(hdfs_builder_); + } +#endif + } + + // TODO(plat1ko): template <class Params> + static hdfsFS create_fs(const HdfsBuildConf& params) { + HDFSBuilder builder; + int ret = builder.init_hdfs_builder(); + if (ret != 0) { + return nullptr; + } + + ret = builder.init(params); + if (ret != 0) { + hdfsFreeBuilder(builder.hdfs_builder_); + return nullptr; + } + + return hdfsBuilderConnect(builder.hdfs_builder_); + } + +private: + HDFSBuilder() = default; + + // returns 0 for success otherwise error + int init(const HdfsBuildConf& conf) { + DCHECK(hdfs_builder_); + hdfsBuilderSetNameNode(hdfs_builder_, conf.fs_name().c_str()); + // set kerberos conf + bool kerberos_login = false; + if (conf.has_hdfs_kerberos_keytab()) { + kerberos_login = true; +#ifdef USE_HADOOP_HDFS + hdfsBuilderSetKerb5Conf(hdfs_builder_, config::kerberos_krb5_conf_path.c_str()); + hdfsBuilderSetKeyTabFile(hdfs_builder_, conf.hdfs_kerberos_keytab().c_str()); +#endif + } + + if (conf.has_hdfs_kerberos_principal()) { + kerberos_login = true; + hdfsBuilderSetPrincipal(hdfs_builder_, conf.hdfs_kerberos_keytab().c_str()); + } else if (conf.has_user()) { + hdfsBuilderSetUserName(hdfs_builder_, conf.user().c_str()); +#ifdef USE_HADOOP_HDFS + hdfsBuilderSetKerb5Conf(hdfs_builder_, nullptr); + hdfsBuilderSetKeyTabFile(hdfs_builder_, nullptr); +#endif + } + + // set other conf + for (const auto& kv : conf.hdfs_confs()) { + hdfsBuilderConfSetStr(hdfs_builder_, kv.key().c_str(), kv.value().c_str()); +#ifdef USE_HADOOP_HDFS + // Set krb5.conf, we should define java.security.krb5.conf in catalog properties + if (kv.key() == "java.security.krb5.conf") { + hdfsBuilderSetKerb5Conf(hdfs_builder_, kv.value().c_str()); + } +#endif + } + + if (kerberos_login) { + int ret = check_krb_params(conf.hdfs_kerberos_principal(), conf.hdfs_kerberos_keytab()); + if (ret != 0) { + return ret; + } + } + + hdfsBuilderConfSetStr(hdfs_builder_, "ipc.client.fallback-to-simple-auth-allowed", "true"); + return 0; + } + + // returns 0 for success otherwise error + int init_hdfs_builder() { + hdfs_builder_ = hdfsNewBuilder(); + if (hdfs_builder_ == nullptr) { + LOG(WARNING) << "failed to init HDFSBuilder, please check check be/conf/hdfs-site.xml"; + return -1; + } + hdfsBuilderSetForceNewInstance(hdfs_builder_); + return 0; + } + + // returns 0 for success otherwise error + int check_krb_params(std::string_view hdfs_kerberos_principal, Review Comment: warning: method 'check_krb_params' can be made static [readability-convert-member-functions-to-static] ```suggestion static int check_krb_params(std::string_view hdfs_kerberos_principal, ``` ########## cloud/src/recycler/checker.h: ########## @@ -24,6 +24,7 @@ #include <unordered_map> #include <unordered_set> +#include "recycler/s3_accessor.h" Review Comment: warning: 'recycler/s3_accessor.h' file not found [clang-diagnostic-error] ```cpp #include "recycler/s3_accessor.h" ^ ``` ########## cloud/src/recycler/hdfs_accessor.cpp: ########## @@ -0,0 +1,286 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "recycler/hdfs_accessor.h" Review Comment: warning: 'recycler/hdfs_accessor.h' file not found [clang-diagnostic-error] ```cpp #include "recycler/hdfs_accessor.h" ^ ``` ########## cloud/src/recycler/hdfs_accessor.cpp: ########## @@ -0,0 +1,286 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "recycler/hdfs_accessor.h" + +#include <gen_cpp/cloud.pb.h> + +#include <string_view> + +#include "common/config.h" +#include "common/logging.h" + +namespace doris::cloud { +namespace { + +// Removes any leading, and trailing `c` +void strip(std::string& str, char c) { + if (!str.empty()) { + size_t start = str.find_first_not_of(c); + if (start == std::string::npos) { + str = ""; + } else { + size_t end = str.find_last_not_of(c); + if (start > 0 || end < str.size() - 1) { + str = str.substr(start, end - start + 1); + } + } + } +} + +std::string_view hdfs_error() { +#ifdef USE_HADOOP_HDFS + char* root_cause = hdfsGetLastExceptionRootCause(); + return root_cause ? root_cause : ""; +#else + return hdfsGetLastError(); +#endif +} + +} // namespace + +class HDFSBuilder { +public: + ~HDFSBuilder() { Review Comment: warning: use '= default' to define a trivial destructor [modernize-use-equals-default] ```cpp ~HDFSBuilder() { ^ ``` ########## cloud/src/recycler/s3_accessor.h: ########## @@ -18,60 +18,15 @@ #pragma once #include <memory> -#include <string> -#include <vector> + +#include "recycler/obj_store_accessor.h" Review Comment: warning: 'recycler/obj_store_accessor.h' file not found [clang-diagnostic-error] ```cpp #include "recycler/obj_store_accessor.h" ^ ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org For additional commands, e-mail: commits-h...@doris.apache.org
