-
Notifications
You must be signed in to change notification settings - Fork 59
feat(bulk-load): bulk load download part4 - replica report download status and progress #479
Conversation
} | ||
|
||
total_progress /= _replica->_primary_states.membership.max_replica_count; | ||
ddebug_replica("total download progress = {}%", total_progress); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about use float type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
total_progress
will report to meta server in bulk_load_response
, defined in thrift, it seems that we don't use float in thrift before, and I think it is okay to define it as integer.
case bulk_load_status::BLS_DOWNLOADING: | ||
case bulk_load_status::BLS_DOWNLOADED: | ||
bulk_load_state.__set_download_progress(_download_progress.load()); | ||
bulk_load_state.__set_download_status(_download_status.load()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always report the two type of status, so why separate to two types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that why not combine download progress and status into one structure? I used to define a structure called download_states
before, but I finally separate it, it will make code more complex, besides, download_progress
and download_status
is more clearer than download_states
.
…load status and progress XiaoMi#479
The whole bulk load download process is like:
This pull request is about replica report download status and progress, step 6 and step 7 above.