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

Queue operations for pglogical replication set-up #4628

Merged

Conversation

yrudman
Copy link
Contributor

@yrudman yrudman commented Sep 7, 2018

Queue operations for pglogical replication set-up

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1562956

depends on ManageIQ/manageiq#17956

BEFORE: Page was locked (and ui worker occupied) for the duration of save operation
before

AFTER: Request to save queued and page unlocked
after

@yrudman yrudman force-pushed the move-replication-setup-to-dedicated-class branch from 19fbce4 to aef6fd7 Compare September 7, 2018 16:42
@miq-bot miq-bot added the wip label Sep 7, 2018
@yrudman yrudman force-pushed the move-replication-setup-to-dedicated-class branch from aef6fd7 to a653f2f Compare September 7, 2018 18:00
@yrudman yrudman force-pushed the move-replication-setup-to-dedicated-class branch 5 times, most recently from 5b9115a to 6f6b7d9 Compare September 12, 2018 13:35
@yrudman yrudman changed the title [WIP] Queue operations for pglogical replication set-up Queue operations for pglogical replication set-up Sep 12, 2018
@miq-bot miq-bot removed the wip label Sep 12, 2018
@yrudman
Copy link
Contributor Author

yrudman commented Sep 12, 2018

@miq-bot add-label enhancement

\cc @gtanzillo

@yrudman yrudman closed this Sep 12, 2018
@yrudman yrudman reopened this Sep 12, 2018
@mzazrivec
Copy link
Contributor

The CI failure

1) OpsController OpsController::Settings::Common #update_exclude_tables_for_remote_region updates the exclude tables for the remote region
Failure/Error:super(options).to_str
ActionController::UrlGenerationError:
  No route matches {:action=>"wait_for_task", :escape=>false, :task_id=>32000000000011}
# ./app/helpers/application_helper.rb:1352:in `remote_function'
# ./app/controllers/application_controller/wait_for_task.rb:31:in `block in browser_refresh_task'
# ./app/controllers/application_controller/wait_for_task.rb:29:in `browser_refresh_task'
# ./app/controllers/application_controller/wait_for_task.rb:61:in `initiate_wait_for_task'
# ./app/controllers/ops_controller/settings/common.rb:209:in `pglogical_save_subscriptions'
# ./spec/controllers/ops_controller/settings/common_spec.rb:215:in `block (4 levels) in <top (required)>'

might be related.

@yrudman yrudman force-pushed the move-replication-setup-to-dedicated-class branch from 6f6b7d9 to 305c5f0 Compare September 13, 2018 16:08
@yrudman yrudman force-pushed the move-replication-setup-to-dedicated-class branch from 305c5f0 to 4538215 Compare September 13, 2018 16:21
@miq-bot
Copy link
Member

miq-bot commented Sep 13, 2018

Checked commits yrudman/manageiq-ui-classic@2e9833f~...4538215 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@yrudman
Copy link
Contributor Author

yrudman commented Sep 13, 2018

@mzazrivec thank you, failing test fixed

@mzazrivec mzazrivec self-assigned this Sep 14, 2018
@mzazrivec mzazrivec added this to the Sprint 95 Ending Sep 24, 2018 milestone Sep 14, 2018
@mzazrivec mzazrivec merged commit ecd81a8 into ManageIQ:master Sep 14, 2018
@yrudman yrudman deleted the move-replication-setup-to-dedicated-class branch September 14, 2018 11:30
@yrudman
Copy link
Contributor Author

yrudman commented Sep 14, 2018

thank you for merging!
@mzazrivec @himdel there is some small UX changes/degradation in this PR and I am not sure how to fix it: there is no confirmation sent to user after task queued using initiate_wait_for_task,
I was getting Error caught: [AbstractController::DoubleRenderError] Render and/or redirect were called multiple times in this action.... error if add_flash + javascript_flash used together with initiate_wait_for_task.
Do you know how/if there is some work around it?

@himdel
Copy link
Contributor

himdel commented Sep 14, 2018

@yrudman That's because initiate_wait_for_task is doing its own render (see browser_refresh_task in ApplicationController::WaitForTask).

So.. you would probably have to extend browser_refresh_task to support outputting flash messages.

@yrudman
Copy link
Contributor Author

yrudman commented Sep 14, 2018

@himdel thank you for looking at it.
So, they way to fix it is to add extra parameter for confirmation message to initiate_wait_for_task and pass it down to browser_refresh_task with rewriting implementation of browser_refresh_task.

@himdel
Copy link
Contributor

himdel commented Sep 14, 2018

@yrudman yes, sounds about right .. except maybe it'd be better to just make that extra parameter enable pushing the flash messages, instead of just hardcoding one

(as in, use add_flash as now, and then call initiate_wait_for_task with something like :render_flash => true added)

@yrudman
Copy link
Contributor Author

yrudman commented Sep 26, 2018

@himdel I do not think i can implement it (combine two render), it is out of my very limited UI skills, - different league...

@himdel
Copy link
Contributor

himdel commented Oct 4, 2018

Ok, not sure this is still needed, but should it become so:

diff --git a/app/controllers/application_controller/wait_for_task.rb b/app/controllers/application_controller/wait_for_task.rb
index f0c37121c..67456c4d6 100644
--- a/app/controllers/application_controller/wait_for_task.rb
+++ b/app/controllers/application_controller/wait_for_task.rb
@@ -24,12 +24,13 @@ module ApplicationController::WaitForTask
     end
   end
 
-  def browser_refresh_task(task_id)
+  def browser_refresh_task(task_id, should_flash = false)
     session[:async][:interval] += 250 if session[:async][:interval] < 5000 # Slowly move up to 5 second retries
     render :update do |page|
       page << javascript_prologue
       ajax_call = remote_function(:url => {:action => 'wait_for_task', :task_id => task_id})
       page << "setTimeout(\"#{ajax_call}\", #{session[:async][:interval]});"
+      page.replace("flash_msg_div", :partial => "layouts/flash_msg") if should_flash
     end
   end
   private :browser_refresh_task
@@ -38,6 +39,7 @@ module ApplicationController::WaitForTask
   # :task_id => id of task to wait for
   # :action  => 'action_to_call' -- action to be called when the task finishes
   # :rx_action => 'method_to_call' -- a method to create a RxJs message
+  # :flash => true|false -- output queued flash messages *while waiting*
   #
   def initiate_wait_for_task(options = {})
     task_id = options[:task_id]
@@ -58,7 +60,7 @@ module ApplicationController::WaitForTask
       session[:async][:params][:rx_action] = options[:rx_action]
     end
 
-    browser_refresh_task(task_id)
+    browser_refresh_task(task_id, !!options[:flash])
   end
   private :initiate_wait_for_task
 

Is all you'd need to be able to call something like...

add_flash(_("Queued"))
initiate_wait_for_task(:task_id => task_id, :action => "pglogical_save_finished", :flash => true)

and have that flash appear while the spinner is spinning.
(If the message needs to be shown after the action, it needs to come from pglogical_save_finished.)

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.

4 participants