feature/#140 Add visit_batch method to CopyVisitor trait to cp CLI #148
Conversation
atimin
left a comment
atimin
left a comment
There was a problem hiding this comment.
Thanks you for the PR. I haven't tested it yet, I think we need to change the implementation first. Please, read my comments.
PS: please, install pre-commit. The project has confiuration for this, it can lint the project automatically.
src/cmd/cp/b2b.rs
Outdated
| .send() | ||
| .await | ||
| } | ||
| async fn visit_batch(&self, entry_name: &str, records: Vec<Record>) -> Result<BTreeMap<u64, ReductError>, ReductError> { |
There was a problem hiding this comment.
I think you don't need this, change signature of the visit method:
async fn visit(&self, entry_name: &str, records: Vec<Record>) -> Result<(), ReductError> {
if records.len() == 1 {
let record = records.pop();
self.dst_bucket
.write_record(entry_name)
.timestamp_us(record.timestamp_us())
.labels(record.labels().clone())
.content_type(record.content_type())
.content_length(record.content_length() as u64)
.stream(record.stream_bytes())
.send()
.await
} else {
self.dst_bucket
.write_record(entry_name)
.timestamp_us(record.timestamp_us())
.labels(record.labels().clone())
.content_type(record.content_type())
.content_length(record.content_length() as u64)
.stream(record.stream_bytes())
.send()
.await
}
}
src/cmd/cp.rs
Outdated
| .arg( | ||
| make_ext_arg() | ||
| ) | ||
| .arg( |
There was a problem hiding this comment.
no parameter is needed, we can do batching automatically
| src_bucket: Bucket, | ||
| query_params: QueryParams, | ||
| visitor: V, | ||
| dst_bucket_v: V, |
There was a problem hiding this comment.
the visitor name wasn't nice, but the bucket is missliding. It can be a folder on a file system. Let's call it dst or something like this.
src/cmd/cp/helpers.rs
Outdated
| while let Some(record) = record_stream.next().await { | ||
| if let Err(err) = record { | ||
| if let Err(e) = make_attempt!(err) { | ||
| if let Err(e) = make_attempt(&mut attempts, &mut transfer_progress, &mut params, record_count, timestamp, err) { |
There was a problem hiding this comment.
you don't need to remove the macros and change the method a lot, just add batching before the visit call, like you do in your method.
There was a problem hiding this comment.
I leave only one macros - print_error_progress. make_attempt convert to a method, because the method needed in couple other methods.
Rumpelshtinskiy
force-pushed
the
enable_batching
branch
2 times, most recently
from
bec8b04 to
44e99bd
Compare
Rumpelshtinskiy
force-pushed
the
enable_batching
branch
from
44e99bd to
c849336
Compare
Rumpelshtinskiy
force-pushed
the
enable_batching
branch
from
c849336 to
8360c1f
Compare
Rumpelshtinskiy
force-pushed
the
enable_batching
branch
from
67e3fc9 to
f6f5a67
Compare
Closes #
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
feature
What was changed?
Extended CopyToBucketVisitor trait. Added batch copy method and using it.