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

Fixes #20809 - Add interface for retrieving pending step counts #251

Merged
merged 7 commits into from Oct 6, 2017

Conversation

adamruzicka
Copy link
Contributor

No description provided.

Copy link
Member

@iNecas iNecas left a comment

Choose a reason for hiding this comment

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

Haven't got though the full end2end test due to issues found, but the approach looks good: nice and simple. Pending tests as well

post('/worlds/pending_steps') do
@worlds = world.coordinator.find_worlds
@worlds.each do |w|
steps = world.get_pending_steps(w.data['id'], nil, 5).value!.values.sum
Copy link
Member

Choose a reason for hiding this comment

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

I'm getting NoMethodError at /worlds/pending_steps undefined method `sum' for []:Array when running pure dynflow, without Rails, as this methods comes from ActiveSupport

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it was added to pure ruby in 2.4.0, will fix anyway.

end

Response = Algebrick.type do
variants Accepted = atom,
Failed = type { fields! error: String },
Done = atom,
Pong = atom
Pong = atom,
PendingSteps = type { fields! pending_steps: Hash }
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if Steps is the best word, as it has already some meaning: the items there might be either steps or events for the steps: it would be more like execution items

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I'll try to come up with better name for it

@adamruzicka
Copy link
Contributor Author

API still pending

@adamruzicka
Copy link
Contributor Author

Created separate issue for the API part

Copy link
Member

@iNecas iNecas left a comment

Choose a reason for hiding this comment

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

Tested and works well, some suggestions about limiting the scope of worlds we are reaching to, inline. I would be happy to merge that after that.

@@ -62,6 +71,12 @@ def start_termination(*args)
try_to_terminate
end

def execution_items(execution_plan_id = nil)
Copy link
Member

Choose a reason for hiding this comment

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

Since we added more things than just the execution items. something more generic, such as execution_status or something like that would match more what it actually does

@@ -37,6 +37,16 @@ class Console < Sinatra::Base
erb :worlds
end

post('/worlds/execution_items') do
Copy link
Member

Choose a reason for hiding this comment

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

Again: Since we added more things than just the execution items. something more generic, such as execution_status or something like that would match more what it actually does.

@@ -37,6 +37,16 @@ class Console < Sinatra::Base
erb :worlds
end

post('/worlds/execution_items') do
@worlds = world.coordinator.find_worlds
Copy link
Member

Choose a reason for hiding this comment

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

We should limit this to active executors only (so find_worlds(true)) as no reason for reaching to the client worlds.

@iNecas
Copy link
Member

iNecas commented Sep 25, 2017

I don't want the PR to grow to much, there fore here are some post-merge ideas to continue:

  • separate the execution worlds table from the client worlds. The client worlds can stay as table, but the executor ones might deserve something like patternfly cards. Btw. I wonder how much work it would be to teach dynflow some patternfly :)

  • one reason why the table might not be best for the executor worlds is this idea: showing the info
    about steps (including event info and link to the execution plan) that are being executed (ideally with
    info on how long the item has been processed: this would allow us to easily see, what work items
    are blocking the executors).

Again: I'm ok with taking these outside of the PR, but would be great addition to the console regarding
showing the info about the runtime

@adamruzicka
Copy link
Contributor Author

Those items would be definitely nice to have but will have to wait for another PR

Copy link
Member

@iNecas iNecas left a comment

Choose a reason for hiding this comment

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

One last comment. Will test after the last change is made but looks promising.

else
{ execution_plan_id => @jobs.fetch(execution_plan_id, []) }
end
source.reduce({}) { |acc, cur| acc.update(cur.first => cur.last.count) }
Copy link
Member

Choose a reason for hiding this comment

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

What about using destructuring assignment here?:

source.reduce({}) { |acc, (plan_id, work_items)| acc.update(plan_id => work_items.count) }

@iNecas
Copy link
Member

iNecas commented Oct 6, 2017

Tested and worked well. Thanks @adamruzicka

@iNecas iNecas merged commit 8a8e9e9 into Dynflow:master Oct 6, 2017
@adamruzicka adamruzicka deleted the pending-steps branch October 6, 2017 07:32
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.

None yet

2 participants