These are chat archives for ManageIQ/manageiq/performance

8th
Jul 2016
Oleg Barenboim
@chessbyte
Jul 08 2016 12:55
@kbrock ManageIQ/manageiq#9447 still needs to be backported
Keenan Brock
@kbrock
Jul 08 2016 13:05
@chessbyte create a 1 off PR on darga?
Oleg Barenboim
@chessbyte
Jul 08 2016 13:05
@kbrock - yes please
Keenan Brock
@kbrock
Jul 08 2016 13:05
perfect - thanks
Oleg Barenboim
@chessbyte
Jul 08 2016 13:06
in the PR, be clear about what it is related to/derived from
@kbrock and try to keep as close to ManageIQ/manageiq#9447 as possible. NO shiny objects please! :-)
Keenan Brock
@kbrock
Jul 08 2016 14:09
+1
Keenan Brock
@kbrock
Jul 08 2016 14:15
@chessbyte ManageIQ/manageiq#9681 - that one was a tricky one
Oleg Barenboim
@chessbyte
Jul 08 2016 14:16
@kbrock that's why I wanted the author (you) to deal with it
anything that I cannot resolve in 15 seconds is too much for me for PRs I did not author
thanks @kbrock !!
Keenan Brock
@kbrock
Jul 08 2016 14:17
+1
@chessbyte I guess we could have applied the other patch first (which was completely overwritten by the second patch)
git would have been happier - not sure if you care
Oleg Barenboim
@chessbyte
Jul 08 2016 14:18
if that would have worked and low risk, you can suggest that - and I can try that locally
want me to try that path?
Keenan Brock
@kbrock
Jul 08 2016 14:18
nah
both work
clicking merge seems easier :) for you. work already done
Oleg Barenboim
@chessbyte
Jul 08 2016 14:19
is ManageIQ/manageiq#8450 a performance boost needed for Darga?
Keenan Brock
@kbrock
Jul 08 2016 14:19
it fixed a minor bug
but was deemed not big enough
Oleg Barenboim
@chessbyte
Jul 08 2016 14:20
but your Darga PR addresses that bug?
Keenan Brock
@kbrock
Jul 08 2016 14:20
yes
that method was scrapped and rewritten
so the starting point (including or not including 8450) is moot
Oleg Barenboim
@chessbyte
Jul 08 2016 14:20
so, in essence, your PR does #8450 and then #9447 ?
Keenan Brock
@kbrock
Jul 08 2016 14:21
hmm. I guess
er
end result is the same
but I just changed the one commit in the middle to accept the current state of the function instead of actually applying both patches
for tracking purposes, may be better to apply both PRs
Oleg Barenboim
@chessbyte
Jul 08 2016 14:22
cherry-picking both PRs to Darga is clean
Keenan Brock
@kbrock
Jul 08 2016 14:22
ok
Oleg Barenboim
@chessbyte
Jul 08 2016 14:22
I am going in that direction
Keenan Brock
@kbrock
Jul 08 2016 14:22
you want me to test locally?
I agree
Oleg Barenboim
@chessbyte
Jul 08 2016 14:23
hoping the tests are enough
Keenan Brock
@kbrock
Jul 08 2016 14:23
ooh
I meant hoping it would clean merge
no bugs will be introduced
Oleg Barenboim
@chessbyte
Jul 08 2016 14:23
it does clean merge
no conflicts
Keenan Brock
@kbrock
Jul 08 2016 14:23
+1
then that is the right route for sure
I'll delete PR
Oleg Barenboim
@chessbyte
Jul 08 2016 14:23
thx
both are now in upstream darga
Keenan Brock
@kbrock
Jul 08 2016 14:25
s/chessbyte/george_washington_cherry_pick_master/
Oleg Barenboim
@chessbyte
Jul 08 2016 14:31
haha
Keenan Brock
@kbrock
Jul 08 2016 17:49
@chessbyte have you found a git client you like? one that does stuff like what you are doing
squash / fixup / rearrange / split commits in history / cherry-pick / ...
Oleg Barenboim
@chessbyte
Jul 08 2016 17:51
@kbrock command-line and, sometimes, SourceTree
Keenan Brock
@kbrock
Jul 08 2016 17:52
k
Nick LaMuro
@NickLaMuro
Jul 08 2016 20:37
Minor refactoring PR that I am going to use in the future with 9559, but since it can be standalone, I extracted it out: ManageIQ/manageiq#9698
Keenan Brock
@kbrock
Jul 08 2016 20:37
looking thanks
in all honestly, can we merge those 2 fields into 1?
Nick LaMuro
@NickLaMuro
Jul 08 2016 20:39
Was going to avoid that incase 9559 was going to be backported to darga at some point
Keenan Brock
@kbrock
Jul 08 2016 20:39
k
Nick LaMuro
@NickLaMuro
Jul 08 2016 20:40
and I honestly don't know the scope of where state and status are used
haven't investigated really
Keenan Brock
@kbrock
Jul 08 2016 20:40
guess we could delegate :status, :state, :to => :miq_task, and mix that directly into miq_report_results
feel free to rewrite virtual_delegate any way you want. I already intend on adding in the logic to detect if the value is in the query - it is needed for us to fix the speed of tables.
Nick LaMuro
@NickLaMuro
Jul 08 2016 20:43
roger
Keenan Brock
@kbrock
Jul 08 2016 20:43
kinda walking there, waiting for @imtayadeway to make the majority of his changes over in miq_expression
Jason Frey
@Fryguy
Jul 08 2016 20:43
@NickLaMuro I commented on your PR...I'm fine with it as is, but curious if we are relying on it incorrectly in models
wondering if it's just a UI concern, then we can make the backend code work with status normally, and it's up to the UI to present however it wants
Nick LaMuro
@NickLaMuro
Jul 08 2016 20:45
@Fryguy Don't disagree with you, just wasn't trying to rock the UI boat with something that was already in place and was recently tweaked in the last few months
human_status exist like what... 3 times in the codebase
basically it is used by MiqReportResult and MiqWidget
Jason Frey
@Fryguy
Jul 08 2016 20:47
interesting
Nick LaMuro
@NickLaMuro
Jul 08 2016 20:47
both basically to define the "status" of both of those objects
FYI, I am going to add some of my comments from above to the PR, just for the people following along at home, don't feel the need to respond in both places
Jason Frey
@Fryguy
Jul 08 2016 20:50
:+1:
Nick LaMuro
@NickLaMuro
Jul 08 2016 21:06
Hey, only 500+ ruby files in the app dir to check that have the word state in them... no big, right?
Jason Frey
@Fryguy
Jul 08 2016 21:08
yeah, that one's kinda hard to search for :)
Nick LaMuro
@NickLaMuro
Jul 08 2016 21:09
Well, I had to limit it to ruby files too, other wise I got bombarded by locale files...
Joe Rafaniello
@jrafanie
Jul 08 2016 21:26
state and status are my :confused:
@NickLaMuro @kbrock have you played with peek: https://github.com/peek/peek ?
Jason Frey
@Fryguy
Jul 08 2016 21:30
@jrafanie That is cool
We could probably add that for dev mode or something
Rocio showed it a bit at GORUCO
it has different things you can "peek" into: https://github.com/peek/peek#available-peek-views
pg, memcached, gc, etc.
and you can click around the UI as usual... reminds me a bit of the rack mini profiler
I haven't played with it, just remembered it from GORUCO now that they're posting some videos
Nick LaMuro
@NickLaMuro
Jul 08 2016 21:39
@jrafanie looks cool, but no. This is the first I have heard of it
Joe Rafaniello
@jrafanie
Jul 08 2016 21:40
Cool, Rocio had a few tools she suggested, scientist was another
Nick LaMuro
@NickLaMuro
Jul 08 2016 21:42
yeah, I have heard of that one
but that is more for A/B testing if I remember correctly
which I feel is a little bit harder to do with appliance type applications compared to a hosted single web app