[Ovirt-devel] [PATCH] Adds max/min methods to StatsDataList. Limited cleanup in graph_controller.rb. First stab at stats data retrieval for new graphing approach.

Thu Aug 7 16:09:23 UTC 2008

Jim

The Stats changes are actually mine, not Steve's. Since he alone needed 
to integrate
with my changes and effectively tested them for me, I asked him to just 
submit
them.

More comments in line

Jim Meyering wrote:
> Steve Linabery <slinabery at redhat.com> wrote:
>   
>> Sorry for the attachment; haven't set up my esmtp yet.
>>
>> I didn't clean up a great deal of the existing code in graph_controller because I suspect most of it will be removed. I did edit that loop on total_memory so that it wouldn't make mongrel time out.
>>
>> Some whitespace cleanup in graph_controller.
>>
>> Main thing for review is the new method I wrote in graph_controller to retrieve stats in either xml or json (the generation of which is yet to be implemented) for a single host or vm. I want to avoid these big long lists of stats requests to Stats.rb, because a) there's no performance hit AFAICT in breaking them up into smaller requests, and b) the graph (or whatever walks the tree and assembles the data lists for the graph) needs to make more precise requests for data.
>>     
>
> Hi Steve,
>
> A fine net change, overall.
>
> It's great to do whitespace clean-up, but please keep that sort of change
> separate from anything substantial.  Otherwise, it's more work for the
> reviewer to separate the trivially-ignorable whitespace changes from
> the ones that are significant.
>
>   
The whitespace changes are mine.  I thought you wanted them out and had
put such an edict in place.  Sorry for not understanding

> If you run this, it'll make git colorize diff output:
> (it modifies settings in ~/.gitconfig)
>
>   git config --global color.ui auto
>
> Then do "git log -p -1" (but don't pipe it into anything) and you'll
> see the most recent commit.  If it's this one, you should see dark
> red rectangles marking the offending white spaces on lines added by
> that commit.
>
> When I applied that change set via "git am FILE", I saw these warnings:
>
>     $ g am r
>     Applying: Adds max/min methods to StatsDataList. Limited cleanup in graph_controller.rb. First stab at stats data retrieval for new graphing approach.
>     /home/meyering/work/co/ovirt/.git/rebase-apply/patch:173: space before tab in indent.
>          scale.push((counter * tick / 1024).to_s) # divide by 1024 to convert to MB /home/meyering/work/co/ovirt/.git/rebase-apply/patch:338: space before tab in indent.
>                   times.push _time_long_format(time)
>     /home/meyering/work/co/ovirt/.git/rebase-apply/patch:345: space before tab in indent.
>                   time = now - x * 28800
>     /home/meyering/work/co/ovirt/.git/rebase-apply/patch:346: space before tab in indent.
>                   times.push _time_short_format(time)
>     /home/meyering/work/co/ovirt/.git/rebase-apply/patch:535: trailing whitespace.
>
>     warning: squelched 8 whitespace errors
>     warning: 13 lines add whitespace errors.
>
> -----------------------
> Another nit-picky, but important detail:
> Note your long subject line.  It seems rude of git to concatenate things
> like that, but that's the way it works.  You can accommodate it by
> changing the log (git commit --amend) to have a single empty
> line after the first "summary" line:
>
>     --------
>     Add max/min methods to StatsDataList.
>
>     Limited cleanup in graph_controller.rb.
>     First stab at stats data retrieval for new graphing approach.
>     ----------------
>
> [with this next bit, we're getting really minor, but comments/logs matter, too]
> Also, note that "Add ..." is recommended over "Adds ..."
> in all types of documentation (active voice vs passive, direct vs
> indirect).
>
> Another example: I'd change this comment:
>
>   -  # returns data for one pool/host/vm, one target
>   +  # return data for one pool/host/vm, one target
>
> ====================================================
> On to the more substantial:
> In Stats.rb, there are four blocks nearly identical to this one:
>
>          if ( final > my_max )
>             my_max = final
>          end
>          if ( final < my_min )
>             my_min = final
>          end
>
> This is more concise and equivalent:
>
>          my_min = [my_min, final].min
>          my_max = [my_max, final].max
>
>   

My bad, I'm a Ruby newbie, been learning enough to get this work done. 
I'm try to learn more Ruby to be more efficient with my code.

> Also, I noticed that the pre-existing style is to initialize variables at
> the top of a block or function.  It's better to avoid that style, and
> instead to place any initialization as near as possible to the first use.
> For example, if you move the initializations of my_max and my_min down
> so they're nearer their first uses, readers don't have to worry about
> whether they're used uninitialized (now the initialization is just before
> the loop), or if the values are modified between initialization and
> whatever use the reader is looking at.
>
>   
I was actually taught just the opposite, init at the top so its easy to find
the inits.  In my more performance sensitive c++ apps, I tend to init just
before the variable is first used (or just outside of the loop as needed)
if there is a chance of getting out of the function before the variable
would be used. Thus avoiding unneeded initializations.

I appreciate the style suggestions, coming from a long C/C++ background,
I tend do things the way I've been doing them for the last 20+ years. I
try to be fairly flexible in how I write or change code. For instance I
matched the style in graph_controller.rb for previous changes that I've
made. However, since the ovirt program doesn't have a predefined style
guide and I am the one writing and maintaining the Stats code, I will
continue to write it in a manner that is easiest for me to support.

I'm sorry if this upsets your style preferences, but functionality is
more important to me over style. I also tend to worry when people focus
on the style w/o specifying a style guide for the program. Don't you
think oVirt has bigger issues to deal with other than trailing whitespace
and where I init my variables?

-mark
> ===================
>
> I like your peak_history and _time_short_format changes.
> Much more readable that way.  I.e., when the resulting lines
> fit in 80-columns, side-by-side diffs are more useful.
>
> ===================
>
> In graph_controller.rb,
>
> +      megabyte = 1024
> +      totalMemory = @pool.hosts.total_memory
> +      tick = megabyte
> +      if totalMemory >= 10 * megabyte && totalMemory < 100 * megabyte
> +        tick = 10 * megabyte
> +      elsif totalMemory >= 100 * megabyte && totalMemory < 1024 * megabyte
> +        tick = 100 * megabyte
> +      else
> +        tick = 1024 * megabyte
> +      end
> +
> +      counter = 0
> +      while counter * tick < totalMemory do
> +        counter += 1 #this gives us one tick mark beyond totalMemory
> +   	scale.push((counter * tick / 1024).to_s) # divide by 1024 to convert to MB
> +      end
>
> Maybe that while loop test should be "<=", in case
> totalMemory is exactly 100*1024 or 1024*1024?
> Otherwise, it looks like there will be no ticks
> for those two edge cases.
>
> _______________________________________________
> Ovirt-devel mailing list
> Ovirt-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/ovirt-devel
>   