Skip to content

Commit 096a785

Browse files
committed
[fix](hdfs) Remove query profile from HDFS file reader
### What problem does this PR solve? Issue Number: None Related PR: apache#67293 Problem Summary: `CachedRemoteFileReader::prefetch_range()` submits fire-and-forget dry-run tasks that can outlive the query `RuntimeProfile`. `HdfsFileReader` stored raw pointers to query-profile timers and counters, so a delayed prefetch read could update them after the query profile had been destroyed and trigger a use-after-free. Remove the query `RuntimeProfile` dependency and HDFS-specific query-profile timers and counters from `HdfsFileReader`, remove the unused profile passthrough from `HdfsFileSystem`, and update all affected call sites. Generic file-reader and file-cache statistics plus process-wide HDFS bvars remain unchanged. ### Release note None ### Check List (For Author) - Test: Manual test - `build-support/clang-format.sh` - `build-support/check-format.sh` - `build-support/check-build-hygiene.sh` - `git diff --check upstream/master...HEAD` - Behavior changed: Yes. Query profiles no longer expose the HDFS-specific `HdfsIO` timer and HDFS read-statistics counters; HDFS reads and file-cache behavior are unchanged. - Does this need documentation: No (cherry picked from commit 4e45489)
1 parent fea01e6 commit 096a785

10 files changed

Lines changed: 28 additions & 127 deletions

File tree

be/src/agent/task_worker_pool.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1750,9 +1750,9 @@ void update_hdfs_resource(const TStorageResource& param, io::RemoteFileSystemSPt
17501750

17511751
if (!existed_fs) {
17521752
// No such FS instance on BE
1753-
auto res = io::HdfsFileSystem::create(
1754-
param.hdfs_storage_param, param.hdfs_storage_param.fs_name,
1755-
std::to_string(param.id), nullptr, std::move(root_path));
1753+
auto res = io::HdfsFileSystem::create(param.hdfs_storage_param,
1754+
param.hdfs_storage_param.fs_name,
1755+
std::to_string(param.id), std::move(root_path));
17561756
if (!res.has_value()) {
17571757
st = std::move(res).error();
17581758
} else {

be/src/cloud/cloud_storage_engine.cpp

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -166,8 +166,8 @@ struct VaultCreateFSVisitor {
166166
// TODO(ByteYue): Make sure enable_java_support is on
167167
Status operator()(const cloud::HdfsVaultInfo& vault) const {
168168
auto hdfs_params = io::to_hdfs_params(vault);
169-
auto fs = DORIS_TRY(io::HdfsFileSystem::create(hdfs_params, hdfs_params.fs_name, id,
170-
nullptr, vault.prefix()));
169+
auto fs = DORIS_TRY(
170+
io::HdfsFileSystem::create(hdfs_params, hdfs_params.fs_name, id, vault.prefix()));
171171
put_storage_resource(id, {std::move(fs), path_format}, 0);
172172
LOG_INFO("successfully create hdfs vault, vault id {}", id);
173173
return Status::OK();
@@ -196,9 +196,8 @@ struct RefreshFSVaultVisitor {
196196

197197
Status operator()(const cloud::HdfsVaultInfo& vault) const {
198198
auto hdfs_params = io::to_hdfs_params(vault);
199-
auto hdfs_fs =
200-
DORIS_TRY(io::HdfsFileSystem::create(hdfs_params, hdfs_params.fs_name, id, nullptr,
201-
vault.has_prefix() ? vault.prefix() : ""));
199+
auto hdfs_fs = DORIS_TRY(io::HdfsFileSystem::create(
200+
hdfs_params, hdfs_params.fs_name, id, vault.has_prefix() ? vault.prefix() : ""));
202201
auto hdfs = std::static_pointer_cast<io::HdfsFileSystem>(hdfs_fs);
203202
put_storage_resource(id, {std::move(hdfs), path_format}, 0);
204203
return Status::OK();

be/src/exec/sink/writer/vfile_result_writer.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -447,7 +447,7 @@ Status VFileResultWriter::_delete_dir() {
447447
case TStorageBackendType::HDFS: {
448448
THdfsParams hdfs_params = parse_properties(_file_opts->broker_properties);
449449
auto fs = DORIS_TRY(io::HdfsFileSystem::create(hdfs_params, hdfs_params.fs_name,
450-
io::FileSystem::TMP_FS_ID, nullptr));
450+
io::FileSystem::TMP_FS_ID));
451451
return fs->delete_directory(dir);
452452
}
453453
case TStorageBackendType::S3: {

be/src/io/file_factory.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ Result<io::FileSystemSPtr> FileFactory::create_fs(const io::FSPropertiesRef& fs_
123123
case TFileType::FILE_HDFS: {
124124
std::string fs_name = _get_fs_name(file_description);
125125
return io::HdfsFileSystem::create(*fs_properties.properties, fs_name,
126-
io::FileSystem::TMP_FS_ID, nullptr);
126+
io::FileSystem::TMP_FS_ID);
127127
}
128128
case TFileType::FILE_HTTP: {
129129
const auto& kv = *fs_properties.properties;
@@ -249,7 +249,7 @@ Result<io::FileReaderSPtr> FileFactory::_create_file_reader_internal(
249249
RETURN_IF_ERROR_RESULT(ExecEnv::GetInstance()->hdfs_mgr()->get_or_create_fs(
250250
system_properties.hdfs_params, *fs_name, &handler));
251251
return io::HdfsFileReader::create(file_description.path, handler->hdfs_fs, *fs_name,
252-
reader_options, profile)
252+
reader_options)
253253
.and_then([&](auto&& reader) {
254254
return io::create_cached_file_reader(std::move(reader), reader_options);
255255
});

be/src/io/fs/benchmark/hdfs_benchmark.hpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ class HdfsCreateWriteBenchmark : public BaseBenchmark {
9595
io::FileWriterPtr writer;
9696
THdfsParams hdfs_params = parse_properties(_conf_map);
9797
auto fs = DORIS_TRY(io::HdfsFileSystem::create(hdfs_params, hdfs_params.fs_name,
98-
io::FileSystem::TMP_FS_ID, nullptr));
98+
io::FileSystem::TMP_FS_ID));
9999
RETURN_IF_ERROR(fs->create_file(file_path, &writer));
100100
return write(state, writer.get());
101101
}
@@ -116,7 +116,7 @@ class HdfsRenameBenchmark : public BaseBenchmark {
116116
auto new_file_path = file_path + "_new";
117117
THdfsParams hdfs_params = parse_properties(_conf_map);
118118
auto fs = DORIS_TRY(io::HdfsFileSystem::create(hdfs_params, hdfs_params.fs_name,
119-
io::FileSystem::TMP_FS_ID, nullptr));
119+
io::FileSystem::TMP_FS_ID));
120120

121121
auto start = std::chrono::high_resolution_clock::now();
122122
RETURN_IF_ERROR(fs->rename(file_path, new_file_path));
@@ -143,7 +143,7 @@ class HdfsExistsBenchmark : public BaseBenchmark {
143143

144144
THdfsParams hdfs_params = parse_properties(_conf_map);
145145
auto fs = DORIS_TRY(io::HdfsFileSystem::create(hdfs_params, hdfs_params.fs_name,
146-
io::FileSystem::TMP_FS_ID, nullptr));
146+
io::FileSystem::TMP_FS_ID));
147147

148148
auto start = std::chrono::high_resolution_clock::now();
149149
bool res = false;

be/src/io/fs/hdfs_file_reader.cpp

Lines changed: 3 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -20,19 +20,15 @@
2020
#include <stdint.h>
2121

2222
#include <algorithm>
23-
#include <filesystem>
24-
#include <ostream>
2523
#include <utility>
2624

2725
#include "bvar/latency_recorder.h"
2826
#include "bvar/reducer.h"
2927
#include "common/compiler_util.h" // IWYU pragma: keep
30-
#include "common/logging.h"
3128
#include "common/metrics/doris_metrics.h"
3229
#include "cpp/sync_point.h"
3330
#include "io/fs/err_utils.h"
3431
#include "io/hdfs_util.h"
35-
#include "runtime/file_scan_profile.h"
3632
#include "runtime/thread_context.h"
3733
#include "runtime/workload_group/workload_group.h"
3834
#include "runtime/workload_management/io_throttle.h"
@@ -63,49 +59,24 @@ Result<FileHandleCache::Accessor> get_file(const hdfsFS& fs, const Path& file, i
6359
} // namespace
6460

6561
Result<FileReaderSPtr> HdfsFileReader::create(Path full_path, const hdfsFS& fs, std::string fs_name,
66-
const FileReaderOptions& opts,
67-
RuntimeProfile* profile) {
62+
const FileReaderOptions& opts) {
6863
auto path = convert_path(full_path, fs_name);
6964
return get_file(fs, path, opts.mtime, opts.file_size).transform([&](auto&& accessor) {
7065
return std::make_shared<HdfsFileReader>(std::move(path), std::move(fs_name),
71-
std::move(accessor), profile, opts.mtime);
66+
std::move(accessor), opts.mtime);
7267
});
7368
}
7469

7570
HdfsFileReader::HdfsFileReader(Path path, std::string fs_name, FileHandleCache::Accessor accessor,
76-
RuntimeProfile* profile, int64_t mtime)
71+
int64_t mtime)
7772
: _path(std::move(path)),
7873
_fs_name(std::move(fs_name)),
7974
_accessor(std::move(accessor)),
80-
_profile(profile),
8175
_mtime(mtime) {
8276
_handle = _accessor.get();
8377

8478
DorisMetrics::instance()->hdfs_file_open_reading->increment(1);
8579
DorisMetrics::instance()->hdfs_file_reader_total->increment(1);
86-
if (_profile != nullptr && is_hdfs(_fs_name)) {
87-
#ifdef USE_HADOOP_HDFS
88-
const char* hdfs_profile_name = "HdfsIO";
89-
_total_read_time =
90-
ADD_CHILD_TIMER(_profile, hdfs_profile_name,
91-
file_scan_profile::parent_or_root(_profile, file_scan_profile::IO));
92-
_hdfs_profile.total_bytes_read =
93-
ADD_CHILD_COUNTER(_profile, "TotalBytesRead", TUnit::BYTES, hdfs_profile_name);
94-
_hdfs_profile.total_local_bytes_read =
95-
ADD_CHILD_COUNTER(_profile, "TotalLocalBytesRead", TUnit::BYTES, hdfs_profile_name);
96-
_hdfs_profile.total_short_circuit_bytes_read = ADD_CHILD_COUNTER(
97-
_profile, "TotalShortCircuitBytesRead", TUnit::BYTES, hdfs_profile_name);
98-
_hdfs_profile.total_total_zero_copy_bytes_read = ADD_CHILD_COUNTER(
99-
_profile, "TotalZeroCopyBytesRead", TUnit::BYTES, hdfs_profile_name);
100-
101-
_hdfs_profile.total_hedged_read =
102-
ADD_CHILD_COUNTER(_profile, "TotalHedgedRead", TUnit::UNIT, hdfs_profile_name);
103-
_hdfs_profile.hedged_read_in_cur_thread = ADD_CHILD_COUNTER(
104-
_profile, "HedgedReadInCurThread", TUnit::UNIT, hdfs_profile_name);
105-
_hdfs_profile.hedged_read_wins =
106-
ADD_CHILD_COUNTER(_profile, "HedgedReadWins", TUnit::UNIT, hdfs_profile_name);
107-
#endif
108-
}
10980
}
11081

11182
HdfsFileReader::~HdfsFileReader() {
@@ -122,7 +93,6 @@ Status HdfsFileReader::close() {
12293

12394
Status HdfsFileReader::read_at_impl(size_t offset, Slice result, size_t* bytes_read,
12495
const IOContext* io_ctx) {
125-
SCOPED_TIMER(_total_read_time);
12696
auto st = do_read_at_impl(offset, result, bytes_read, io_ctx);
12797
if (!st.ok()) {
12898
_handle = nullptr;
@@ -254,46 +224,4 @@ Status HdfsFileReader::do_read_at_impl(size_t offset, Slice result, size_t* byte
254224
}
255225
#endif
256226

257-
void HdfsFileReader::_collect_profile_before_close() {
258-
if (_profile != nullptr && is_hdfs(_fs_name)) {
259-
#ifdef USE_HADOOP_HDFS
260-
if (_handle == nullptr) [[unlikely]] {
261-
return;
262-
}
263-
264-
struct hdfsReadStatistics* hdfs_statistics = nullptr;
265-
auto r = hdfsFileGetReadStatistics(_handle->file(), &hdfs_statistics);
266-
if (r != 0) {
267-
LOG(WARNING) << "Failed to run hdfsFileGetReadStatistics(): " << r
268-
<< ", name node: " << _fs_name;
269-
return;
270-
}
271-
COUNTER_UPDATE(_hdfs_profile.total_bytes_read, hdfs_statistics->totalBytesRead);
272-
COUNTER_UPDATE(_hdfs_profile.total_local_bytes_read, hdfs_statistics->totalLocalBytesRead);
273-
COUNTER_UPDATE(_hdfs_profile.total_short_circuit_bytes_read,
274-
hdfs_statistics->totalShortCircuitBytesRead);
275-
COUNTER_UPDATE(_hdfs_profile.total_total_zero_copy_bytes_read,
276-
hdfs_statistics->totalZeroCopyBytesRead);
277-
hdfsFileFreeReadStatistics(hdfs_statistics);
278-
279-
struct hdfsHedgedReadMetrics* hdfs_hedged_read_statistics = nullptr;
280-
r = hdfsGetHedgedReadMetrics(_handle->fs(), &hdfs_hedged_read_statistics);
281-
if (r != 0) {
282-
LOG(WARNING) << "Failed to run hdfsGetHedgedReadMetrics(): " << r
283-
<< ", name node: " << _fs_name;
284-
return;
285-
}
286-
287-
COUNTER_UPDATE(_hdfs_profile.total_hedged_read, hdfs_hedged_read_statistics->hedgedReadOps);
288-
COUNTER_UPDATE(_hdfs_profile.hedged_read_in_cur_thread,
289-
hdfs_hedged_read_statistics->hedgedReadOpsInCurThread);
290-
COUNTER_UPDATE(_hdfs_profile.hedged_read_wins,
291-
hdfs_hedged_read_statistics->hedgedReadOpsWin);
292-
293-
hdfsFreeHedgedReadMetrics(hdfs_hedged_read_statistics);
294-
hdfsFileClearReadStatistics(_handle->file());
295-
#endif
296-
}
297-
}
298-
299227
} // namespace doris::io

be/src/io/fs/hdfs_file_reader.h

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -42,10 +42,10 @@ class HdfsFileReader final : public FileReader {
4242
// - /path_to_file
4343
// TODO(plat1ko): Support related path for cloud mode
4444
static Result<FileReaderSPtr> create(Path path, const hdfsFS& fs, std::string fs_name,
45-
const FileReaderOptions& opts, RuntimeProfile* profile);
45+
const FileReaderOptions& opts);
4646

4747
HdfsFileReader(Path path, std::string fs_name, FileHandleCache::Accessor accessor,
48-
RuntimeProfile* profile, int64_t mtime = 0);
48+
int64_t mtime = 0);
4949

5050
~HdfsFileReader() override;
5151

@@ -63,35 +63,15 @@ class HdfsFileReader final : public FileReader {
6363
Status read_at_impl(size_t offset, Slice result, size_t* bytes_read,
6464
const IOContext* io_ctx) override;
6565

66-
void _collect_profile_before_close() override;
67-
6866
Status do_read_at_impl(size_t offset, Slice result, size_t* bytes_read,
6967
const IOContext* io_ctx);
7068

7169
private:
72-
#ifdef USE_HADOOP_HDFS
73-
struct HDFSProfile {
74-
RuntimeProfile::Counter* total_bytes_read = nullptr;
75-
RuntimeProfile::Counter* total_local_bytes_read = nullptr;
76-
RuntimeProfile::Counter* total_short_circuit_bytes_read = nullptr;
77-
RuntimeProfile::Counter* total_total_zero_copy_bytes_read = nullptr;
78-
79-
RuntimeProfile::Counter* total_hedged_read = nullptr;
80-
RuntimeProfile::Counter* hedged_read_in_cur_thread = nullptr;
81-
RuntimeProfile::Counter* hedged_read_wins = nullptr;
82-
};
83-
#endif
84-
8570
Path _path;
8671
std::string _fs_name;
8772
FileHandleCache::Accessor _accessor;
8873
CachedHdfsFileHandle* _handle = nullptr; // owned by _cached_file_handle
8974
std::atomic<bool> _closed = false;
90-
RuntimeProfile* _profile = nullptr;
91-
RuntimeProfile::Counter* _total_read_time = nullptr;
9275
int64_t _mtime;
93-
#ifdef USE_HADOOP_HDFS
94-
HDFSProfile _hdfs_profile;
95-
#endif
9676
};
9777
} // namespace doris::io

be/src/io/fs/hdfs_file_system.cpp

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -54,14 +54,13 @@ namespace doris::io {
5454

5555
Result<std::shared_ptr<HdfsFileSystem>> HdfsFileSystem::create(
5656
const std::map<std::string, std::string>& properties, std::string fs_name, std::string id,
57-
RuntimeProfile* profile, std::string root_path) {
57+
std::string root_path) {
5858
return HdfsFileSystem::create(parse_properties(properties), std::move(fs_name), std::move(id),
59-
profile, std::move(root_path));
59+
std::move(root_path));
6060
}
6161

6262
Result<std::shared_ptr<HdfsFileSystem>> HdfsFileSystem::create(const THdfsParams& hdfs_params,
6363
std::string fs_name, std::string id,
64-
RuntimeProfile* profile,
6564
std::string root_path) {
6665
#ifdef USE_HADOOP_HDFS
6766
if (!config::enable_java_support) {
@@ -70,18 +69,17 @@ Result<std::shared_ptr<HdfsFileSystem>> HdfsFileSystem::create(const THdfsParams
7069
"true."));
7170
}
7271
#endif
73-
std::shared_ptr<HdfsFileSystem> fs(new HdfsFileSystem(
74-
hdfs_params, std::move(fs_name), std::move(id), profile, std::move(root_path)));
72+
std::shared_ptr<HdfsFileSystem> fs(new HdfsFileSystem(hdfs_params, std::move(fs_name),
73+
std::move(id), std::move(root_path)));
7574
RETURN_IF_ERROR_RESULT(fs->init());
7675
return fs;
7776
}
7877

7978
HdfsFileSystem::HdfsFileSystem(const THdfsParams& hdfs_params, std::string fs_name, std::string id,
80-
RuntimeProfile* profile, std::string root_path)
79+
std::string root_path)
8180
: RemoteFileSystem(std::move(root_path), std::move(id), FileSystemType::HDFS),
8281
_hdfs_params(hdfs_params),
83-
_fs_name(std::move(fs_name)),
84-
_profile(profile) {
82+
_fs_name(std::move(fs_name)) {
8583
if (_fs_name.empty()) {
8684
_fs_name = hdfs_params.fs_name;
8785
}
@@ -112,8 +110,7 @@ Status HdfsFileSystem::create_file_impl(const Path& file, FileWriterPtr* writer,
112110
Status HdfsFileSystem::open_file_internal(const Path& file, FileReaderSPtr* reader,
113111
const FileReaderOptions& opts) {
114112
CHECK_HDFS_HANDLER(_fs_handler);
115-
*reader =
116-
DORIS_TRY(HdfsFileReader::create(file, _fs_handler->hdfs_fs, _fs_name, opts, _profile));
113+
*reader = DORIS_TRY(HdfsFileReader::create(file, _fs_handler->hdfs_fs, _fs_name, opts));
117114
return Status::OK();
118115
}
119116

be/src/io/fs/hdfs_file_system.h

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@
3232
#include "io/fs/hdfs.h"
3333
#include "io/fs/path.h"
3434
#include "io/fs/remote_file_system.h"
35-
#include "runtime/runtime_profile.h"
3635

3736
namespace doris {
3837
class THdfsParams;
@@ -48,12 +47,11 @@ class HdfsFileSystem final : public RemoteFileSystem {
4847
public:
4948
static Result<std::shared_ptr<HdfsFileSystem>> create(const THdfsParams& hdfs_params,
5049
std::string fs_name, std::string id,
51-
RuntimeProfile* profile,
5250
std::string root_path = "");
5351

5452
static Result<std::shared_ptr<HdfsFileSystem>> create(
5553
const std::map<std::string, std::string>& properties, std::string fs_name,
56-
std::string id, RuntimeProfile* profile, std::string root_path = "");
54+
std::string id, std::string root_path = "");
5755

5856
~HdfsFileSystem() override;
5957

@@ -85,11 +83,10 @@ class HdfsFileSystem final : public RemoteFileSystem {
8583
private:
8684
friend class HdfsFileWriter;
8785
HdfsFileSystem(const THdfsParams& hdfs_params, std::string fs_name, std::string id,
88-
RuntimeProfile* profile, std::string root_path);
86+
std::string root_path);
8987
const THdfsParams& _hdfs_params; // Only used in init, so we can use reference here
9088
std::string _fs_name;
9189
std::shared_ptr<HdfsHandler> _fs_handler = nullptr;
92-
RuntimeProfile* _profile = nullptr;
9390
};
9491
} // namespace io
9592
} // namespace doris

be/src/runtime/snapshot_loader.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -766,7 +766,7 @@ Status BaseSnapshotLoader::init(TStorageBackendType::type type, const std::strin
766766
} else if (TStorageBackendType::type::HDFS == type) {
767767
THdfsParams hdfs_params = parse_properties(_prop);
768768
_remote_fs = DORIS_TRY(io::HdfsFileSystem::create(hdfs_params, hdfs_params.fs_name,
769-
io::FileSystem::TMP_FS_ID, nullptr));
769+
io::FileSystem::TMP_FS_ID));
770770
} else if (TStorageBackendType::type::BROKER == type) {
771771
std::shared_ptr<io::BrokerFileSystem> fs;
772772
_remote_fs = DORIS_TRY(

0 commit comments

Comments
 (0)