Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

kv: use header.TargetBytes for ExportRequest #69435

Open
dt opened this issue Aug 26, 2021 · 3 comments
Open

kv: use header.TargetBytes for ExportRequest #69435

dt opened this issue Aug 26, 2021 · 3 comments
Labels
A-disaster-recovery A-kv-client Relating to the KV client and the KV interface. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-disaster-recovery
Projects
KV Disaster Recovery Backlog

Comments

@dt
Copy link
Member

dt commented Aug 26, 2021 •
edited by exalate-issue-sync bot

Now that we don't have cloud-file writing anymore, it is confusing that ExportRequest has a TargetFileSize while its header also has header.TargetBytes, and both need to be set, I believe, to paginate correctly?

Instead, should we have all callers just put their desired response size in header.TargetBytes and deprecate the field in ExportRequest? I think we'd then just always set reply.NumBytes to header.TargetBytes since we always want, I think, for a caller to immediately get whatever file we produced instead of distsender trying to go back for more (but not much more, since it'll be - NumBytes) and waiting to stitch them into a multi-file reply for the caller.

Jira issue: CRDB-9595

Epic CRDB-19061
@dt dt added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-kv-client Relating to the KV client and the KV interface. labels Aug 26, 2021
@dt dt added this to Incoming in KV via automation Aug 26, 2021
@blathers-crl blathers-crl bot added the T-kv KV Team label Aug 26, 2021
@dt
Copy link
Member Author

dt commented Aug 26, 2021

cc @aliher1911

@dt dt added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) and removed C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. labels Aug 26, 2021
@dt dt mentioned this issue Aug 26, 2021
Closed
@lunevalex lunevalex moved this from Incoming to Prioritized in KV Oct 20, 2021
@erikgrinaker
Copy link
Contributor

I think we'd then just always set reply.NumBytes to header.TargetBytes since we always want, I think, for a caller to immediately get whatever file we produced instead of distsender trying to go back for more (but not much more, since it'll be - NumBytes) and waiting to stitch them into a multi-file reply for the caller.

This would be addressed by #70763, which can disable pagination to subsequent ranges with small limits.

@lunevalex lunevalex moved this from Prioritized to Quick Wins in KV Nov 2, 2021
adityamaru added a commit to adityamaru/cockroach that referenced this issue Feb 3, 2023
@adityamaru
backupccl,kvserver: adds a max_allowed_file_size_overage field to Exp…
f5a1c6d 
…ortRequest

This change passes in the value of kv.bulk_sst.max_allowed_overage
as an argument in the ExportRequest instead of reading it at the time
of evaluation. This allows tenants to configure this setting to a
desired value instead of always using the system tenants cluster setting
value.

Informs: cockroachdb#69435

Release note: None
@adityamaru adityamaru mentioned this issue Feb 3, 2023
Closed
@adityamaru adityamaru self-assigned this Feb 6, 2023
@exalate-issue-sync exalate-issue-sync bot added T-disaster-recovery and removed T-kv KV Team labels Feb 6, 2023
@blathers-crl
Copy link

blathers-crl bot commented Feb 6, 2023

cc @cockroachdb/disaster-recovery

@blathers-crl blathers-crl bot added this to Triage in Disaster Recovery Backlog Feb 6, 2023
adityamaru added a commit to adityamaru/cockroach that referenced this issue Feb 6, 2023
@adityamaru
backupccl,kvserver: use header.TargetBytes to convey target file size
46095f0 
Previously, ExportRequest would set its `TargetFileSize` field
to the target size of each SST it expected as part of the response.
Additionally, it set `header.TargetBytes` to a sentinel value of 1
to force every ExportRequest to paginate regardless of if the generated
SST was of the target file size or not.

This change teaches ExportRequest to stop setting the `TargetFileSize`
field, but instead exclusively use the `header.TargetBytes` to control
the target size of the SST returned as part of the ExportResponse. To prevent
DistSender from sending an ExportRequest to a subsequent range with a small,
remaining TargetBytes, we also set `header.ReturnOnRangeBoundary` to true.
Aside from a more intuitive use of DistSender limits there are no changes
expected to the pagination of ExportRequests sent during a backup. We will
not aggregate ExportRequests across range boundaries and will continue to
generate SST files of a size controlled by `kv.bulk_sst.target_size`.

For mixed version compatability, if all nodes in the cluster are not running
a 23.1 binary we will fallback to the legacy behaviour described above.

Informs: cockroachdb#69435

Release note: None
@adityamaru adityamaru mentioned this issue Feb 27, 2023
Closed
adityamaru added a commit to adityamaru/cockroach that referenced this issue Feb 27, 2023
@adityamaru
backupccl,kvserver: use header.TargetBytes to convey target file size
f75c894 
Previously, ExportRequest would set its `TargetFileSize` field
to the target size of each SST it expected as part of the response.
Additionally, it set `header.TargetBytes` to a sentinel value of 1
to force every ExportRequest to paginate regardless of if the generated
SST was of the target file size or not.

This change teaches ExportRequest to stop setting the `TargetFileSize`
field, but instead exclusively use the `header.TargetBytes` to control
the target size of the SST returned as part of the ExportResponse. To prevent
DistSender from sending an ExportRequest to a subsequent range with a small,
remaining TargetBytes, we also set `header.ReturnOnRangeBoundary` to true.
Aside from a more intuitive use of DistSender limits there are no changes
expected to the pagination of ExportRequests sent during a backup. We will
not aggregate ExportRequests across range boundaries and will continue to
generate SST files of a size controlled by `kv.bulk_sst.target_size`.

For mixed version compatability, if all nodes in the cluster are not running
a 23.1 binary we will fallback to the legacy behaviour described above.

Informs: cockroachdb#69435

Release note: None
@adityamaru adityamaru removed their assignment Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-disaster-recovery A-kv-client Relating to the KV client and the KV interface. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-disaster-recovery
Projects
Disaster Recovery Backlog
Backlog
KV
Quick Wins
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dt @erikgrinaker @adityamaru