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.

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

storageccl: turn default import batch size down to 32MB #18037

Merged
merged 1 commit into from
Aug 30, 2017

Conversation

benesch
Copy link
Contributor

@benesch benesch commented Aug 30, 2017

63MB seemed like a safe size for the import batch size, but resulted in errors
like

command is too large: 68166516 bytes (max: 67108864)

on some restores. Turn the size down to 32MB to avoid these errors.

63MB seemed like a safe size for the import batch size, but resulted in errors
like

    command is too large: 68166516 bytes (max: 67108864)

on some restores. Turn the size down to 32MB to avoid these errors.
@benesch benesch requested review from danhhz and a team August 30, 2017 18:41
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@danhhz
Copy link
Contributor

danhhz commented Aug 30, 2017

:lgtm:

I think it's worth merging this as-is. But we may want to consider removing the maxImportBatchSize logic as a followup. Doesn't seem super useful anymore


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@benesch
Copy link
Contributor Author

benesch commented Aug 30, 2017

Agreed, but I think it's worth understanding why it's broken first. Concerning that Raft seems to be adding 10MB of overhead to the batch request.

@benesch benesch merged commit aeaebbe into cockroachdb:master Aug 30, 2017
@benesch benesch deleted the import-batch-size branch August 30, 2017 19:31
@danhhz
Copy link
Contributor

danhhz commented Aug 30, 2017

Wanna file a 1.2 issue to follow up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants