These are chat archives for ManageIQ/manageiq/performance

5th
Jul 2016
Oleg Barenboim
@chessbyte
Jul 05 2016 14:31
@kbrock backported a bunch of PRs to Darga last night and I think one of yours broke the Darga build: https://travis-ci.org/ManageIQ/manageiq/jobs/142358474
Keenan Brock
@kbrock
Jul 05 2016 14:31
wat?
aah
that is me
thanks
Oleg Barenboim
@chessbyte
Jul 05 2016 14:32
maybe there was another one I need to backport to make it right
Keenan Brock
@kbrock
Jul 05 2016 14:33
I recognize that method
will track it down
for the record, I clearly stated darga/no for the PR that introduced that method. :(
Oleg Barenboim
@chessbyte
Jul 05 2016 14:37
so, we have darga/no for the PR on which this PR was based?
Jason Frey
@Fryguy
Jul 05 2016 14:37
yeah....I did not expect virtual_total to come back to darga
Oleg Barenboim
@chessbyte
Jul 05 2016 14:37
we can revert guys
Jason Frey
@Fryguy
Jul 05 2016 14:38
@kbrock which PR introduced it?
Keenan Brock
@kbrock
Jul 05 2016 14:40
@Fryguy ManageIQ/manageiq#8870
that one is invasive
VERY cool performance implications BUT... yea - that is an aggressive PR
Jason Frey
@Fryguy
Jul 05 2016 14:40
yeah, way too much enhancements for darga
Keenan Brock
@kbrock
Jul 05 2016 14:41
currently if you use an miq_expression with a virtual column - it will definitely pull back all records
that PR was never intended for darga
Oleg Barenboim
@chessbyte
Jul 05 2016 14:41
so, let's revert
hodge podge version
Oleg Barenboim
@chessbyte
Jul 05 2016 14:41
I defer to @Fryguy
Keenan Brock
@kbrock
Jul 05 2016 14:42
@Fryguy what PR are we talking about reverting?
the virtual totals one is darga/no
aah -- ManageIQ/manageiq#9263
aah - yea, I think revert
@Fryguy virtual totals are not back there. I mislabeled. I was thinking this was an important fix. but what it fixed is probably not on darga
Oleg Barenboim
@chessbyte
Jul 05 2016 14:45
@Fryguy - how do I revert? or can you take care of it?
I need to think more. sorry. researching
Keenan Brock
@kbrock
Jul 05 2016 14:54
@Fryguy looks like most of virtual totals is on darga. and there is a bug there which this PR fixes. We're missing that one method I am linking. Don't think rolling back is the right course of action
Jason Frey
@Fryguy
Jul 05 2016 14:55
@kbrock Can you sort out what should or should not be there, and @chessbyte nd I can sort out the mechanics of fixing darga
Keenan Brock
@kbrock
Jul 05 2016 14:55
We're missing 1 method (2 line method) from ar_virtual.rb
Jason Frey
@Fryguy
Jul 05 2016 14:55
is that in a different PR?
Keenan Brock
@kbrock
Jul 05 2016 14:56
yes. that was added in that bigger pr (that you and I say NO)
I could "cherry pick" that 1 method
create a pr
Jason Frey
@Fryguy
Jul 05 2016 15:01
yeah create a PR to darga
Keenan Brock
@kbrock
Jul 05 2016 15:02
ManageIQ/manageiq#9607
Jason Frey
@Fryguy
Jul 05 2016 15:02
So, virtual totals made it to darga, but ManageIQ/manageiq#8870 did not (that is, using virtual totals in MiqExpression), correct?
just trying to get clarity
Keenan Brock
@kbrock
Jul 05 2016 15:03
virtual totals made it. miq_expression fixes did not. Both rely upon ar_virtual (arel in sql). 8870 introduced a helper method to ar_virtual
Jason Frey
@Fryguy
Jul 05 2016 15:03
got it thanks
Keenan Brock
@kbrock
Jul 05 2016 15:04
it was one of those changes during pr cleanup...
@Fryguy noticing darga has very different gemfile. just fyi
Jason Frey
@Fryguy
Jul 05 2016 15:07
how so?
Keenan Brock
@kbrock
Jul 05 2016 15:19
@Fryguy typed bundle install from darga - and different net-ssh and a few others. not sure if it matters
Oleg Barenboim
@chessbyte
Jul 05 2016 15:20
not everything in master is on darga
see my spreadsheet (in email) of what got backported already
Keenan Brock
@kbrock
Jul 05 2016 15:23
thanks
Jason Frey
@Fryguy
Jul 05 2016 15:29
yeah, I expect differences, but not a lot of differences
Keenan Brock
@kbrock
Jul 05 2016 15:31
aah - I may have over spoken
Oleg Barenboim
@chessbyte
Jul 05 2016 15:54
@kbrock care to elaborate?
Keenan Brock
@kbrock
Jul 05 2016 16:06
@chessbyte I had checked out darga and did a bundle install and was surprised it required a bundle update
seems there are only 5-6 changes
amazon provider, patternfly, hawkular, net-ssh, winrm(and fs), proctable, along with some trivials
Jason Frey
@Fryguy
Jul 05 2016 16:10
winrm is interesting...would have though that would be backported
but the rest is kind of expected
Oleg Barenboim
@chessbyte
Jul 05 2016 17:36
@Fryguy we can backport anything we need, if there is rationale to do so
@kbrock https://travis-ci.org/ManageIQ/manageiq/builds/142492086 still red -- is that still from your PR or one of the others I backported?
looks like the failure is something else
Keenan Brock
@kbrock
Jul 05 2016 18:36
@chessbyte I just asked on the main room
Keenan Brock
@kbrock
Jul 05 2016 20:34
@NickLaMuro ping? ManageIQ/manageiq#9559
@Fryguy you ok with this version of status? [ref]
Nick LaMuro
@NickLaMuro
Jul 05 2016 20:43
See my most recent comments
Jason Frey
@Fryguy
Jul 05 2016 20:44
@kbrock ah the 'ol "hammer-eggs" I see ;)
Keenan Brock
@kbrock
Jul 05 2016 20:44
lol
lets get this merged
Jason Frey
@Fryguy
Jul 05 2016 20:44
:hammer: :egg: ;)
Keenan Brock
@kbrock
Jul 05 2016 20:44
:)
aah - much better
Jason Frey
@Fryguy
Jul 05 2016 20:44
no need to rush things
Keenan Brock
@kbrock
Jul 05 2016 20:44
I like making status smarter
Jason Frey
@Fryguy
Jul 05 2016 20:45
it's not like we are sev 1'ing this feature in, so a healthy amount of debate discussion is fine by me
Keenan Brock
@kbrock
Jul 05 2016 20:46
I try to abide by capping it at 1 day per line of change. and this introduces 2 statements (complex ones) - so maybe 1 week max?
Jason Frey
@Fryguy
Jul 05 2016 20:46
not counting weekends and holidays I hope :)
Keenan Brock
@kbrock
Jul 05 2016 20:46
I like adding a little foo to the sql query, and the existing attributes work as expected
lol
prefer not to say "if I tack on this to the query - then use a different method to get essentially the same attribute"
but again - if someone feels strongly, then I'm good.
Keenan Brock
@kbrock
Jul 05 2016 20:51
@Fryguy if you want me to rebase ManageIQ/manageiq#9611 onto ManageIQ/manageiq#9588 - it would be easier if you merged the test only pr first? :)
Jason Frey
@Fryguy
Jul 05 2016 20:55
@kbrock I agree, but leaving that PR up to @gtanzillo to review and merge
Since I am not as familiar with alerts
Keenan Brock
@kbrock
Jul 05 2016 21:08
@NickLaMuro so that PR, it is basically just a delegate to another model
that is why we introduced virtual_delegate
it is a virtual column that is a delegate into another table
it automatically defines the type, and does the arel/sql
it will even work in sorting and parameters - out of the box
BUT ;)
the naming is the same naming as delegate, and the default it nil
had spoken about supporting a different delegate
but if you don't want to deal with that, you can use the existing code as an example for the :arel to use for that virtual_column
the goal is to get away from retyping all these common patterns. especially when we want similar optimizations in our reporting
Nick LaMuro
@NickLaMuro
Jul 05 2016 21:20
What I was hoping to do with all of this was to not instantiate the MiqTask models at all
Keenan Brock
@kbrock
Jul 05 2016 21:32
exactly
virtual delegate does not
(or with :arel =>)
in our screens, it asks the attribute if it can be done in sql, if it can, then it will be accessed that way
for sorting for sure, I'm pretty sure for other access
if not, then fixing that will be a big win across the board
but the arel in general is buying us the biggest gains - to avoid ruby
think we may need to rethink virtual_delegate. Using delegate under the covers is probably not buying us much and probably costing us.
by the way the try(:column_name) is nice/simple. I've been using rescue and stuff :(