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

Adds file splitting support to dump/backup commands #50

Merged

Conversation

NickLaMuro
Copy link
Member

@NickLaMuro NickLaMuro commented Jul 21, 2018

NOTE: Currently built off of #51 Update: This has been rebased and this branch updated accordingly.

This exposes the :byte_count flag that is added to the evm:db:backup and evm:db:dump tasks here:

The user experience will look something like the following:

Would you like to split the dump output into multiple parts? (Y/N): y
Enter the byte size to split by: |500M| 750M

Links

@NickLaMuro NickLaMuro force-pushed the file_split_for_db_backup_and_db_dump branch 4 times, most recently from afdeb8c to 284fef1 Compare July 26, 2018 23:37
@NickLaMuro NickLaMuro force-pushed the file_split_for_db_backup_and_db_dump branch 4 times, most recently from a04492a to 1d51ff0 Compare August 2, 2018 16:06
@NickLaMuro NickLaMuro changed the title Adds file splitting support to dump/backup commands [WIP] Adds file splitting support to dump/backup commands Aug 3, 2018
@NickLaMuro
Copy link
Member Author

Might change how we are handling file splitting, so setting this as [WIP] for now. Most likely, not much is going to change with this, but the underlying pre-requisite PRs needed for this are most likely going to change, so this can't be merged until those are sorted out.

See ManageIQ/manageiq#17798 (comment) for some further details.

@@ -147,6 +148,12 @@ def ask_for_tables_to_exclude_in_dump
end || true
end

def ask_to_split_up_output
if action != :restore && should_split_output?
Copy link
Member

Choose a reason for hiding this comment

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

I think we should only support this for the :dump action on the first pass since restore from split backups won't work and it wasn't really part of the original RFE.

Copy link
Member Author

@NickLaMuro NickLaMuro Sep 19, 2018

Choose a reason for hiding this comment

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

since restore from split backups won't work

Neither restoring from :dump or :backup is supported with splits currently, since there is no way to fetch the splits (via a glob) and combine them into a single stream.

Adding support for restoring from a split would be a separate RFE, but it would effectively be the same work to do it for both as it would be for one. They basically would need some form of a pipeline to first take the files in order and read their data in order as if they were a single stream.

Copy link
Member

Choose a reason for hiding this comment

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

I guess what I'm saying is that we shouldn't give the user the option to backup using splits if we can't restore with splits. I'm fine with the option to dump using splits because the intention was never to restore it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess what I'm saying is that we shouldn't give the user the option to backup using splits if we can't restore with splits.

I guess that is fair.

I figured that someone would eventually request the splitting functionality for backups as well anyway, specifically for s3, since there limits to file size.


Please note that I am also partially arguing this point simply because I don't want to delete the appliance console integration tests that I have written that already support running against and validating split based backups...

(deep down... I am really a simple human being really...)

Copy link
Member

Choose a reason for hiding this comment

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

Definitely not arguing against creating split backups at some point, but I think we need split restore at that point as well. If we leave this as is, some user will be rather unhappy when we have to tell them that they can't restore a backup that we let them create.

Copy link
Member Author

Choose a reason for hiding this comment

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

Welp, I can do that. Despite not wanting to comment out/remove some of my personal tests, I think I agree with that line of thinking.

Will try and take care of that code change shortly.

@NickLaMuro NickLaMuro changed the title [WIP] Adds file splitting support to dump/backup commands Adds file splitting support to dump/backup commands Sep 19, 2018
@miq-bot miq-bot removed the wip label Sep 19, 2018
Adds a set of questions to be queried for `:dump` and `:restore` actions
in DatabaseAdmin.  It asks:

- If the wishes to split up the output of the dump/restore
- If yes, how big should the split files be (default 500M)
@NickLaMuro NickLaMuro force-pushed the file_split_for_db_backup_and_db_dump branch from 68db4a9 to b7ab9cb Compare September 19, 2018 21:07
@miq-bot
Copy link
Member

miq-bot commented Sep 19, 2018

Checked commits NickLaMuro/manageiq-appliance_console@2f084da~...b7ab9cb with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 2 offenses detected

spec/database_admin_spec.rb

@carbonin carbonin self-assigned this Sep 20, 2018
@carbonin carbonin merged commit f7741cf into ManageIQ:master Sep 20, 2018
@carbonin carbonin added this to the Sprint 95 Ending Sep 24, 2018 milestone Sep 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants