Allow specifying table exclusions in Database Dump action #47

NickLaMuro · 2018-06-05T22:20:16Z

This pull request exposes the functionality of the --exclude-table-data flag in pg_dump, which was added to manageiq and manageiq-gems-pending in the following pull requests respectively:

In the user experience would look something like this:

Would you like to exclude tables in the dump? (Y/N): y
To exclude tables from the dump, enter them in a space separated
list.  For example:

    > metrics_* vim_performance_states event_streams

Enter the tables to exclude: |metrics_* vim_performance_states event_streams| event_streams

Links

The `#ask_yn?` method is one of the few methods in `Prompts` that does not make use of the `just_ask` question. `#just_ask`, by default, has everything flow through `Readline`, which has a different output than the standard commands. Because of this, if you try mixing the two in the specs, you can get some strange behavior, such EOFError: The input stream is exhausted. Which comes from the Highline lib. This happens because the input is trying to be read from two different sources at once, and doesn't function as expected doing that (one ends up being blank, even though some input is still available). * * * Instead of making the risky change of forcing `q.readline` for every usage of `ask_yn?`, allow for a block, so this can be done a-la-carte as needed.

NickLaMuro · 2018-06-05T22:44:50Z

Code Climate failure is failing on something that I modified, and really isn't worth fixing at this point in my opinion.

carbonin · 2018-06-06T19:31:17Z

This looks good, I'm just not sure about the defaults for the exclude. I would lean towards not excluding any tables by default. Would want some more opinions before we merge this.

@jdeubel @jrafanie @dmetzger57 what do you think?

dmetzger57 · 2018-06-06T19:44:44Z

I'm good with the tables chosen as the defaults to be excluded (they are the ones typically bloating a dump and are not needed), and at a higher level in having a default set of tables on this option. However, I defer to support on whether a default would be good or bad in the wild.

jdeubel · 2018-06-06T19:56:17Z

I would say by default have vim_performance_states and event_streams as these tables can take up a bit of space and are very rarely needed from my experience. For the metrics* tables I feel the customer is just going to hit enter since it is quick and easy. We should leave it up to (support) to tell them what other tables are needed (such as metrics) as the customer might not know.

jrafanie · 2018-06-06T20:02:51Z

lib/manageiq/appliance_console/database_admin.rb

@@ -96,6 +97,26 @@ def ask_to_delete_backup_after_restore
        end
      end

+      def ask_for_tables_to_exclude_in_dump
+        if action == :dump && should_exlcude_tables?


typo: should_exlcude_tables -> should_exclude_tables

+2 points for making the typo consistently in the method name. 😉

Don't worry, I type destory instead of destroy.

performance is the bane of my existence usually... ends up as preformance...

Good catch. Thanks!

+2 points for making the typo consistently in the method name. 😉

Autocomplete...

jrafanie · 2018-06-06T20:20:21Z

I know we don't want feature creep but showing them which tables are the top x in size, might make it easier for them to know which to exclude.

I don't have a strong opinion on default tables either way. We can always ask for the missing tables in the event we're missing tables we need. What @jdeubel said is good with me.

NickLaMuro · 2018-06-06T20:43:49Z

I know we don't want feature creep but showing them which tables are the top x in size, might make it easier for them to know which to exclude.

I am going to say no to this for now, possible future feature though. Reason being is we really rely on the getting DB connection info from the application, and the appliance_console is just a thin wrapper around rake tasks from the app in most cases. I feel like adding a rake tasks, or something similar that takes seconds to run (because of application bootup time specifically) to get this feature in would be a deterrent more than a help (since most people, as mentioned earlier, would just want to hit ENTER).

This looks good, I'm just not sure about the defaults for the exclude...

Just for the record, the tables I chose were not picked at random, but plucked from the previous ticket and discussion:

https://bugzilla.redhat.com/show_bug.cgi?id=1535345#c12

Specifically to @jdeubel 's comment, the metrics tables from metrics_* are just the ones that are collected (and purged) on a daily (every 4 hours?) basis (so metrics_00, metrics_01, etc.), which I assumed in most cases was not the issue that has gotten support involved at this point. These are not the long term storage metric tables.

The other reason I wanted to have the metrics_* in there it is a small hint/example that globbing is possible with this, which I think is important to avoid having them avoid as much typing as possible.

That last bit about "avoiding typing" was a big goal for this, and I hope it influences the overall decision here. I put a y/n question right before this so if excluding was not wanted at all, then the default excludes would be omitted entirely. But if exclusions were desired, I wanted the "95% case" to be something that could just be another ENTER keystroke away, without making this feature too complex.

Anyway, that is my </2cents>. Would have put it in the original PR description, but it probably wouldn't have been read anyway then. 😭

Adds a set of questions to be queried for `:dump` based actions in DatabaseAdmin. It asks: - If any tables wish to be excluded - If yes, what tables should be excluded The defaults for this are set as some sensible and standard defaults to be used, and allow for quickly moving on, but no tables will be excluded if the user answers no to the first question.

miq-bot · 2018-06-06T20:51:17Z

Checked commits NickLaMuro/manageiq-appliance_console@01cf151~...79b9fec with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

jrafanie · 2018-06-07T11:29:59Z

Specifically to @jdeubel 's comment, the metrics tables from metrics_* are just the ones that are collected (and purged) on a daily (every 4 hours?) basis (so metrics_00, metrics_01, etc.), which I assumed in most cases was not the issue that has gotten support involved at this point. These are not the long term storage metric tables.

The other reason I wanted to have the metrics_* in there it is a small hint/example that globbing is possible with this, which I think is important to avoid having them avoid as much typing as possible.

Those seem like good reasons to include metrics_* in the default exclusion list.

jrafanie

LGTM, @jdeubel @carbonin are you ok with it?

jdeubel · 2018-06-07T11:42:58Z

@jrafanie I'm ok with that. It all depends on the case for what we need but I would assume they would have our guidance on what to exclude anyways.

NickLaMuro added 2 commits June 1, 2018 14:42

Fixes database_admin_spec.rb indentation

01cf151

NickLaMuro force-pushed the exclude-tables-in-pg-dump branch 2 times, most recently from 2f7842e to e45bf12 Compare June 5, 2018 22:43

jrafanie reviewed Jun 6, 2018

View reviewed changes

NickLaMuro added 2 commits June 6, 2018 15:49

Ask for table excludes in DatabaseAdmin#ask_questions

79b9fec

NickLaMuro force-pushed the exclude-tables-in-pg-dump branch from e45bf12 to 79b9fec Compare June 6, 2018 20:49

jrafanie approved these changes Jun 7, 2018

View reviewed changes

carbonin approved these changes Jun 7, 2018

View reviewed changes

carbonin self-assigned this Jun 7, 2018

carbonin added the enhancement label Jun 7, 2018

carbonin added this to the Sprint 88 Ending Jun 18, 2018 milestone Jun 7, 2018

carbonin merged commit e460b5f into ManageIQ:master Jun 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow specifying table exclusions in Database Dump action #47

Allow specifying table exclusions in Database Dump action #47

NickLaMuro commented Jun 5, 2018

NickLaMuro commented Jun 5, 2018

carbonin commented Jun 6, 2018

dmetzger57 commented Jun 6, 2018

jdeubel commented Jun 6, 2018 •

edited

Loading

jrafanie Jun 6, 2018

NickLaMuro Jun 6, 2018

NickLaMuro Jun 6, 2018

jrafanie commented Jun 6, 2018

NickLaMuro commented Jun 6, 2018 •

edited

Loading

miq-bot commented Jun 6, 2018

jrafanie commented Jun 7, 2018

jrafanie left a comment

jdeubel commented Jun 7, 2018

Allow specifying table exclusions in Database Dump action #47

Allow specifying table exclusions in Database Dump action #47

Conversation

NickLaMuro commented Jun 5, 2018

Links

NickLaMuro commented Jun 5, 2018

carbonin commented Jun 6, 2018

dmetzger57 commented Jun 6, 2018

jdeubel commented Jun 6, 2018 • edited Loading

jrafanie Jun 6, 2018

Choose a reason for hiding this comment

NickLaMuro Jun 6, 2018

Choose a reason for hiding this comment

NickLaMuro Jun 6, 2018

Choose a reason for hiding this comment

jrafanie commented Jun 6, 2018

NickLaMuro commented Jun 6, 2018 • edited Loading

miq-bot commented Jun 6, 2018

jrafanie commented Jun 7, 2018

jrafanie left a comment

Choose a reason for hiding this comment

jdeubel commented Jun 7, 2018

jdeubel commented Jun 6, 2018 •

edited

Loading

NickLaMuro commented Jun 6, 2018 •

edited

Loading