acelyc111 commented on code in PR #2077:
URL:
https://github.com/apache/incubator-pegasus/pull/2077#discussion_r1689099409
##########
src/replica/bulk_load/replica_bulk_loader.cpp:
##########
@@ -534,7 +534,30 @@ void replica_bulk_loader::download_sst_file(const
std::string &remote_dir,
int32_t file_index,
dist::block_service::block_filesystem *fs)
{
- const file_meta &f_meta = _metadata.files[file_index];
+ if (_status != bulk_load_status::BLS_DOWNLOADING) {
+ LOG_WARNING_PREFIX("Cancel download_sst_file task, because bulk_load
local_status is {}. "
+ "local_dir: {} , file_index is {}.",
Review Comment:
```suggestion
LOG_WARNING_PREFIX("Cancel downloading sst files, because bulk load
local status is {}. "
"local_dir: {}, file_index is {}.",
```
##########
src/replica/bulk_load/replica_bulk_loader.cpp:
##########
@@ -534,7 +534,30 @@ void replica_bulk_loader::download_sst_file(const
std::string &remote_dir,
int32_t file_index,
dist::block_service::block_filesystem *fs)
{
- const file_meta &f_meta = _metadata.files[file_index];
+ if (_status != bulk_load_status::BLS_DOWNLOADING) {
+ LOG_WARNING_PREFIX("Cancel download_sst_file task, because bulk_load
local_status is {}. "
+ "local_dir: {} , file_index is {}.",
+ enum_to_string(_status),
+ local_dir,
+ file_index);
+ return;
+ }
+ file_meta f_meta;
+ bool get_f_meta = true;
+ {
+ zauto_read_lock l(_lock);
+ if (file_index < _metadata.files.size()) {
+ f_meta = _metadata.files[file_index];
+ } else {
+ get_f_meta = false;
+ }
+ }
+ if (!get_f_meta) {
+ LOG_WARNING_PREFIX("sst file index {} exceeds number of bulkload sst
files, Cancel "
Review Comment:
How does this happen? It is an error or expected state which means the
downloding is completed?
##########
src/replica/bulk_load/replica_bulk_loader.cpp:
##########
@@ -534,7 +534,30 @@ void replica_bulk_loader::download_sst_file(const
std::string &remote_dir,
int32_t file_index,
dist::block_service::block_filesystem *fs)
{
- const file_meta &f_meta = _metadata.files[file_index];
+ if (_status != bulk_load_status::BLS_DOWNLOADING) {
+ LOG_WARNING_PREFIX("Cancel download_sst_file task, because bulk_load
local_status is {}. "
+ "local_dir: {} , file_index is {}.",
Review Comment:
And why mention local_dir and file_index, is it necessary?
##########
src/replica/bulk_load/replica_bulk_loader.cpp:
##########
@@ -590,9 +613,17 @@ void replica_bulk_loader::download_sst_file(const
std::string &remote_dir,
METRIC_VAR_INCREMENT_BY(bulk_load_download_file_bytes, f_size);
// download next file
- if (file_index + 1 < _metadata.files.size()) {
- const file_meta &next_f_meta = _metadata.files[file_index + 1];
- _download_files_task[next_f_meta.name] =
+ get_f_meta = true;
Review Comment:
This judgement is duplicate with the beginning of this function, right?
I think it's because we need the f_meta.name when emplace
_download_files_task, I didn't find where the string key is used, is it
possible to use int as the key? Then we can just remove the duplicate code.
##########
src/replica/bulk_load/replica_bulk_loader.cpp:
##########
@@ -534,7 +534,30 @@ void replica_bulk_loader::download_sst_file(const
std::string &remote_dir,
int32_t file_index,
dist::block_service::block_filesystem *fs)
{
- const file_meta &f_meta = _metadata.files[file_index];
+ if (_status != bulk_load_status::BLS_DOWNLOADING) {
+ LOG_WARNING_PREFIX("Cancel download_sst_file task, because bulk_load
local_status is {}. "
+ "local_dir: {} , file_index is {}.",
+ enum_to_string(_status),
+ local_dir,
+ file_index);
+ return;
+ }
+ file_meta f_meta;
+ bool get_f_meta = true;
Review Comment:
Could you please add some comments to describe what does `get_f_meta` stand
for? Other places are the same.
##########
src/replica/bulk_load/replica_bulk_loader.cpp:
##########
@@ -534,7 +534,30 @@ void replica_bulk_loader::download_sst_file(const
std::string &remote_dir,
int32_t file_index,
dist::block_service::block_filesystem *fs)
{
- const file_meta &f_meta = _metadata.files[file_index];
+ if (_status != bulk_load_status::BLS_DOWNLOADING) {
+ LOG_WARNING_PREFIX("Cancel download_sst_file task, because bulk_load
local_status is {}. "
+ "local_dir: {} , file_index is {}.",
+ enum_to_string(_status),
+ local_dir,
+ file_index);
+ return;
+ }
+ file_meta f_meta;
+ bool get_f_meta = true;
+ {
+ zauto_read_lock l(_lock);
+ if (file_index < _metadata.files.size()) {
+ f_meta = _metadata.files[file_index];
+ } else {
+ get_f_meta = false;
+ }
+ }
+ if (!get_f_meta) {
Review Comment:
How about judge whether the `name` field of `f_meta` is empty instead, avoid
to introduce extra variable?
--
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]
