fix(bulkload): bulkload cause many node coredump #2077
Conversation
| LOG_WARNING_PREFIX("Cancel download_sst_file task, because bulk_load local_status is {}. " | ||
| "local_dir: {} , file_index is {}.", |
There was a problem hiding this comment.
| LOG_WARNING_PREFIX("Cancel download_sst_file task, because bulk_load local_status is {}. " | |
| "local_dir: {} , file_index is {}.", | |
| LOG_WARNING_PREFIX("Cancel downloading sst files, because bulk load local status is {}. " | |
| "local_dir: {}, file_index is {}.", |
There was a problem hiding this comment.
And why mention local_dir and file_index, is it necessary?
There was a problem hiding this comment.
It's not necessary, but I think capturing local_dir and file_index information in the logs would be beneficial.
There was a problem hiding this comment.
Why not update this? It's not a good idea to use varible names in logs, the common administators don't know what do they mean.
Keep in mind that there are difference between users, administators and developers.
ruojieranyishen
force-pushed
the
bulkload_coredump_fix
branch
from
34cede1
to
fdd04eb
Compare
| { | ||
| zauto_read_lock l(_lock); | ||
| if (file_index + 1 < _metadata.files.size()) { | ||
| f_meta = _metadata.files[file_index + 1]; | ||
| if (!_metadata.files.empty()) { |
There was a problem hiding this comment.
Nowthat _metadata is updated here, it's needed to use "write lock", i.e. zauto_write_lock.
| @@ -520,38 +520,25 @@ void replica_bulk_loader::download_files(const std::string &provider_name, | |||
|
|
|||
| // download sst files asynchronously | |||
| if (!_metadata.files.empty()) { | |||
| const file_meta &f_meta = _metadata.files[0]; | |||
| const file_meta f_meta = _metadata.files.back(); | |||
There was a problem hiding this comment.
It's needed to protect the scope by a write lock.
There was a problem hiding this comment.
Thank you for the reminder, I've learned a lot from your suggestions. I'll make the necessary modifications to ensure the code passes the unit tests.
ruojieranyishen
force-pushed
the
bulkload_coredump_fix
branch
from
710a155
to
5469727
Compare
…sst_file (apache#2006) replica_bulk_loader::clear_bulk_load_states function cannot cancel already downloading sst task, which access `_metadata.files` references. But clear_bulk_load_states function will clear `_metadata.files`. It's cause core dump. I use a copy of `_metadata.files` to solve this problem.
ruojieranyishen
force-pushed
the
bulkload_coredump_fix
branch
5 times, most recently
from
ac809ae
to
99100fd
Compare
ruojieranyishen
force-pushed
the
bulkload_coredump_fix
branch
from
a7608ec
to
020b105
Compare
What problem does this PR solve?
Related issue:
#2006
What is changed and how does it work?
Avoid using _metadata.files reference ，and add a read_lock
Tests
Because bulkload imports a large amount of data.
Test 1: bulkload files miss four sst files.
After fix : Table ingest_p4_10G partition1 is missing files, and the table ballot does not increase after bulkload.
Test 2: Bulkload Download stage restart a node
After fix ： No continuous core dumps on multiple nodes.
Side effects
Locking
_metadata.filesmay incur a performance penalty.