Skip to content

Add Buffer Size Parameter to dump create#234

Open
gugacavalieri wants to merge 3 commits into
Qovery:mainfrom
gugacavalieri:add-buffer-size-parameter
Open

Add Buffer Size Parameter to dump create#234
gugacavalieri wants to merge 3 commits into
Qovery:mainfrom
gugacavalieri:add-buffer-size-parameter

Conversation

@gugacavalieri

@gugacavalieri gugacavalieri commented Nov 5, 2022

Copy link
Copy Markdown

Changelog

  • Add Argument to set buffer_size when dumping the database
  • Set the default size to 100 (This was the previous value)

Motivation

I was running into the same issue when dumping tables bigger than 100MB (#208). The order of the Statements were wrong and I was not able to restore the dumps. I still don't know 100% what is cause the issue but it's probably related when the compressed chunks are being read from the datastore and are being restored in the wrong order.

So, this PR aims to make the arguments more flexible and serve as a workaround for the issue above.

PS. I am still learning Rust, so please let me know if this is a good way to move parameters around 😄

@gugacavalieri gugacavalieri marked this pull request as ready for review November 5, 2022 19:16
@evoxmusic

Copy link
Copy Markdown
Contributor

Thank you for the contribution. I'm going to take a look this weekend

Comment thread replibyte/src/commands/dump.rs Outdated
Comment thread replibyte/src/commands/dump.rs Outdated
Comment thread replibyte/src/commands/dump.rs Outdated
Comment thread replibyte/src/commands/dump.rs Outdated
Comment thread replibyte/src/commands/dump.rs Outdated
Comment thread replibyte/src/commands/dump.rs Outdated
Comment thread replibyte/src/tasks/full_dump.rs Outdated
S: Source,
{
pub fn new(source: S, datastore: Box<dyn Datastore>, options: SourceOptions<'a>) -> Self {
pub fn new(source: S, datastore: Box<dyn Datastore>, options: SourceOptions<'a>, arg_buffer_size: usize) -> Self {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My 2 comments:

  1. Include arg_buffer_size into options and remove arg_buffer_size parameter
  2. rename options.arg_buffer_size into something more explicit like dump_chunk_size or anything else

@gugacavalieri

Copy link
Copy Markdown
Author

Good idea! I will do some work on the fixes.

@gugacavalieri

gugacavalieri commented Nov 20, 2022

Copy link
Copy Markdown
Author

@evoxmusic, added dump_chunk_size to the SourceOptions struct.

Also have to play a little bit with the test methods 😃

@evoxmusic

Copy link
Copy Markdown
Contributor

@gugacavalieri can you check tests errors?

@michaelrode

Copy link
Copy Markdown

Thanks for adding this, can you also update the README with an example of how to use this?

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