Copilot commented on code in PR #60120:
URL: https://github.com/apache/doris/pull/60120#discussion_r2715657026
##########
be/test/io/cache/block_file_cache_test_lru_dump.cpp:
##########
@@ -517,7 +517,12 @@ TEST_F(BlockFileCacheTest,
cached_remote_file_reader_direct_read_order_check) {
ASSERT_TRUE(FileCacheFactory::instance()->create_file_cache(cache_base_path,
settings).ok());
auto cache = FileCacheFactory::instance()->_path_to_cache[cache_base_path];
-
+ for (int i = 0; i < 100; i++) {
+ if (cache->get_async_open_success()) {
+ break;
+ };
+ std::this_thread::sleep_for(std::chrono::milliseconds(1));
+ }
Review Comment:
The wait loop for cache initialization lacks an assertion to verify that
initialization succeeded. Looking at existing patterns in the codebase (e.g.,
file_cache_action_test.cpp line 77-80, segment_corruption_test.cpp line
124-127), the loop typically iterates 1000 times instead of 100, and is
followed by an ASSERT_TRUE to verify successful initialization. After the loop,
add an assertion like: ASSERT_TRUE(cache->get_async_open_success());
```suggestion
for (int i = 0; i < 1000; i++) {
if (cache->get_async_open_success()) {
break;
};
std::this_thread::sleep_for(std::chrono::milliseconds(1));
}
ASSERT_TRUE(cache->get_async_open_success());
```
##########
be/test/io/cache/block_file_cache_test.cpp:
##########
@@ -3620,6 +3635,13 @@ TEST_F(BlockFileCacheTest,
cached_remote_file_reader_concurrent) {
context.stats = &rstats;
context.query_id = query_id;
ASSERT_TRUE(FileCacheFactory::instance()->create_file_cache(cache_base_path,
settings).ok());
+ auto cache = FileCacheFactory::instance()->_path_to_cache[cache_base_path];
+ for (int i = 0; i < 100; i++) {
+ if (cache->get_async_open_success()) {
+ break;
+ };
+ std::this_thread::sleep_for(std::chrono::milliseconds(1));
+ }
Review Comment:
The wait loop for cache initialization lacks an assertion to verify that
initialization succeeded. Looking at existing patterns in the codebase (e.g.,
file_cache_action_test.cpp line 77-80, segment_corruption_test.cpp line
124-127), the loop typically iterates 1000 times instead of 100, and is
followed by an ASSERT_TRUE to verify successful initialization. After the loop,
add an assertion like: ASSERT_TRUE(cache->get_async_open_success());
```suggestion
for (int i = 0; i < 1000; i++) {
if (cache->get_async_open_success()) {
break;
};
std::this_thread::sleep_for(std::chrono::milliseconds(1));
}
ASSERT_TRUE(cache->get_async_open_success());
```
##########
be/test/io/cache/block_file_cache_test.cpp:
##########
@@ -6805,6 +6841,13 @@ TEST_F(BlockFileCacheTest,
reader_dryrun_when_download_file_cache) {
context.stats = &rstats;
context.query_id = query_id;
ASSERT_TRUE(FileCacheFactory::instance()->create_file_cache(cache_base_path,
settings).ok());
+ auto cache = FileCacheFactory::instance()->_path_to_cache[cache_base_path];
+ for (int i = 0; i < 100; i++) {
+ if (cache->get_async_open_success()) {
+ break;
+ };
+ std::this_thread::sleep_for(std::chrono::milliseconds(1));
+ }
Review Comment:
The wait loop for cache initialization lacks an assertion to verify that
initialization succeeded. Looking at existing patterns in the codebase (e.g.,
file_cache_action_test.cpp line 77-80, segment_corruption_test.cpp line
124-127), the loop typically iterates 1000 times instead of 100, and is
followed by an ASSERT_TRUE to verify successful initialization. After the loop,
add an assertion like: ASSERT_TRUE(cache->get_async_open_success());
```suggestion
for (int i = 0; i < 1000; i++) {
if (cache->get_async_open_success()) {
break;
};
std::this_thread::sleep_for(std::chrono::milliseconds(1));
}
ASSERT_TRUE(cache->get_async_open_success());
```
##########
be/test/io/cache/block_file_cache_test.cpp:
##########
@@ -4214,6 +4243,13 @@ TEST_F(BlockFileCacheTest,
cached_remote_file_reader_opt_lock) {
context.stats = &rstats;
context.query_id = query_id;
ASSERT_TRUE(FileCacheFactory::instance()->create_file_cache(cache_base_path,
settings).ok());
+ auto cache = FileCacheFactory::instance()->_path_to_cache[cache_base_path];
+ for (int i = 0; i < 100; i++) {
+ if (cache->get_async_open_success()) {
+ break;
+ };
+ std::this_thread::sleep_for(std::chrono::milliseconds(1));
+ }
Review Comment:
The wait loop for cache initialization lacks an assertion to verify that
initialization succeeded. Looking at existing patterns in the codebase (e.g.,
file_cache_action_test.cpp line 77-80, segment_corruption_test.cpp line
124-127), the loop typically iterates 1000 times instead of 100, and is
followed by an ASSERT_TRUE to verify successful initialization. After the loop,
add an assertion like: ASSERT_TRUE(cache->get_async_open_success());
```suggestion
for (int i = 0; i < 1000; i++) {
if (cache->get_async_open_success()) {
break;
};
std::this_thread::sleep_for(std::chrono::milliseconds(1));
}
ASSERT_TRUE(cache->get_async_open_success());
```
##########
be/test/io/cache/block_file_cache_test.cpp:
##########
@@ -3410,6 +3418,13 @@ TEST_F(BlockFileCacheTest,
cached_remote_file_reader_tail) {
context.stats = &rstats;
context.cache_type = io::FileCacheType::NORMAL;
ASSERT_TRUE(FileCacheFactory::instance()->create_file_cache(cache_base_path,
settings).ok());
+ auto cache = FileCacheFactory::instance()->_path_to_cache[cache_base_path];
+ for (int i = 0; i < 100; i++) {
+ if (cache->get_async_open_success()) {
+ break;
+ };
+ std::this_thread::sleep_for(std::chrono::milliseconds(1));
+ }
Review Comment:
The wait loop for cache initialization lacks an assertion to verify that
initialization succeeded. Looking at existing patterns in the codebase (e.g.,
file_cache_action_test.cpp line 77-80, segment_corruption_test.cpp line
124-127), the loop typically iterates 1000 times instead of 100, and is
followed by an ASSERT_TRUE to verify successful initialization. After the loop,
add an assertion like: ASSERT_TRUE(cache->get_async_open_success());
```suggestion
for (int i = 0; i < 1000; i++) {
if (cache->get_async_open_success()) {
break;
};
std::this_thread::sleep_for(std::chrono::milliseconds(1));
}
ASSERT_TRUE(cache->get_async_open_success());
```
##########
be/test/io/cache/block_file_cache_test.cpp:
##########
@@ -3304,6 +3305,13 @@ TEST_F(BlockFileCacheTest, cached_remote_file_reader) {
context.stats = &rstats;
context.query_id = query_id;
ASSERT_TRUE(FileCacheFactory::instance()->create_file_cache(cache_base_path,
settings).ok());
+ auto cache = FileCacheFactory::instance()->_path_to_cache[cache_base_path];
+ for (int i = 0; i < 100; i++) {
+ if (cache->get_async_open_success()) {
+ break;
+ };
+ std::this_thread::sleep_for(std::chrono::milliseconds(1));
+ }
Review Comment:
The wait loop for cache initialization lacks an assertion to verify that
initialization succeeded. Looking at existing patterns in the codebase (e.g.,
file_cache_action_test.cpp line 77-80, segment_corruption_test.cpp line
124-127), the loop typically iterates 1000 times instead of 100, and is
followed by an ASSERT_TRUE to verify successful initialization. After the loop,
add an assertion like: ASSERT_TRUE(cache->get_async_open_success());
```suggestion
for (int i = 0; i < 1000; i++) {
if (cache->get_async_open_success()) {
break;
};
std::this_thread::sleep_for(std::chrono::milliseconds(1));
}
ASSERT_TRUE(cache->get_async_open_success());
```
##########
be/test/io/cache/block_file_cache_test.cpp:
##########
@@ -7613,6 +7663,14 @@ TEST_F(BlockFileCacheTest,
cached_remote_file_reader_direct_read_bytes_check) {
context.stats = &rstats;
context.query_id = query_id;
ASSERT_TRUE(FileCacheFactory::instance()->create_file_cache(cache_base_path,
settings).ok());
+ auto cache = FileCacheFactory::instance()->_path_to_cache[cache_base_path];
+ for (int i = 0; i < 100; i++) {
+ if (cache->get_async_open_success()) {
+ break;
+ };
+ std::this_thread::sleep_for(std::chrono::milliseconds(1));
+ }
Review Comment:
The wait loop for cache initialization lacks an assertion to verify that
initialization succeeded. Looking at existing patterns in the codebase (e.g.,
file_cache_action_test.cpp line 77-80, segment_corruption_test.cpp line
124-127), the loop typically iterates 1000 times instead of 100, and is
followed by an ASSERT_TRUE to verify successful initialization. After the loop,
add an assertion like: ASSERT_TRUE(cache->get_async_open_success());
```suggestion
for (int i = 0; i < 1000; i++) {
if (cache->get_async_open_success()) {
break;
};
std::this_thread::sleep_for(std::chrono::milliseconds(1));
}
ASSERT_TRUE(cache->get_async_open_success());
```
##########
be/test/io/cache/block_file_cache_test.cpp:
##########
@@ -3697,6 +3719,13 @@ TEST_F(BlockFileCacheTest,
cached_remote_file_reader_concurrent_2) {
context.stats = &rstats;
context.query_id = query_id;
ASSERT_TRUE(FileCacheFactory::instance()->create_file_cache(cache_base_path,
settings).ok());
+ auto cache = FileCacheFactory::instance()->_path_to_cache[cache_base_path];
+ for (int i = 0; i < 100; i++) {
+ if (cache->get_async_open_success()) {
+ break;
+ };
+ std::this_thread::sleep_for(std::chrono::milliseconds(1));
+ }
Review Comment:
The wait loop for cache initialization lacks an assertion to verify that
initialization succeeded. Looking at existing patterns in the codebase (e.g.,
file_cache_action_test.cpp line 77-80, segment_corruption_test.cpp line
124-127), the loop typically iterates 1000 times instead of 100, and is
followed by an ASSERT_TRUE to verify successful initialization. After the loop,
add an assertion like: ASSERT_TRUE(cache->get_async_open_success());
```suggestion
for (int i = 0; i < 1000; i++) {
if (cache->get_async_open_success()) {
break;
};
std::this_thread::sleep_for(std::chrono::milliseconds(1));
}
ASSERT_TRUE(cache->get_async_open_success());
```
##########
be/test/io/cache/block_file_cache_test.cpp:
##########
@@ -7528,6 +7572,13 @@ TEST_F(BlockFileCacheTest,
DISABLE_cached_remote_file_reader_direct_read_and_evi
context.stats = &rstats;
context.query_id = query_id;
ASSERT_TRUE(FileCacheFactory::instance()->create_file_cache(cache_base_path,
settings).ok());
+ auto cache = FileCacheFactory::instance()->_path_to_cache[cache_base_path];
+ for (int i = 0; i < 100; i++) {
+ if (cache->get_async_open_success()) {
+ break;
+ };
+ std::this_thread::sleep_for(std::chrono::milliseconds(1));
+ }
Review Comment:
The wait loop for cache initialization lacks an assertion to verify that
initialization succeeded. Looking at existing patterns in the codebase (e.g.,
file_cache_action_test.cpp line 77-80, segment_corruption_test.cpp line
124-127), the loop typically iterates 1000 times instead of 100, and is
followed by an ASSERT_TRUE to verify successful initialization. After the loop,
add an assertion like: ASSERT_TRUE(cache->get_async_open_success());
```suggestion
for (int i = 0; i < 1000; i++) {
if (cache->get_async_open_success()) {
break;
};
std::this_thread::sleep_for(std::chrono::milliseconds(1));
}
ASSERT_TRUE(cache->get_async_open_success());
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
