Re: [NFS] [PATCH] nfs-utils: nfs-iostat.py option to sort by ops/s

[Chuck Lever <chuck.lever@oracle.com>]

On Aug 25, 2009, at 1:34 PM, Steve Dickson wrote:
> On 08/25/2009 12:29 PM, Chuck Lever wrote:
>> [ Cc: changed to correct mailing list.  sf.net list is deprecated. ]
>>
>> On Aug 24, 2009, at 7:24 PM, Lans Carstensen wrote:
>>> Hi,
>>>
>>> I've recently made tools/nfs-iostat/nfs-iostat.py more useful in our
>>> autofs environment with a variety of cleanups and am offering this
>>> patch up for discussion and/or inclusion in nfs-utils.  It does the
>>> following:
>>>
>>> * Adds a --top flag to sort the display of mountpoint entries by  
>>> ops/s.
>>> * Adds a --<n> flag to only display stats for the first <n>  
>>> mountpoints
>>> * Re-reads the mountpoint list on intervals since it's dynamic in an
>>> autofs environment.
>>> * Conforms the Python path to the LSB 3.2+ standard of /usr/bin/ 
>>> python
>>> http://refspecs.freestandards.org/LSB_3.2.0/LSB-Languages/LSB-Languages/pylocation.html
>>>
>>
>> A couple of overall comments.
>>
>> 1.  These seem to be logically independent changes, so we would  
>> prefer
>> them in separate patches.  Each logical change can be refined and  
>> voted
>> up or down separately.
> I guess you could break this up into more patches... but overall its
> by no means unruly....

Didn't mean to imply "unruly" but we do _prefer_ having logical  
changes separated.

>> 2.  Sorting by ops is OK, but not sure about "--top".  Since the  
>> script
>> doesn't generate a curses like display like "top" does, maybe "--sort
>> <n>" would be a better name, and --top and --<n> could be combined.
> Hmm.. I kinda like like the --top flag... its pretty descriptive to
> what it does..

Some people aren't so imaginative, might expect "--top" to produce a  
full curses-like display, then be pissed when they get a simple list.   
And it seems to me that the two new options are related and could be  
combined to good effect.

I think "--top" should be reserved for an actual top-like  
implementation (which might be pretty cool).

>> 3.  The distributors should weigh in on the Python path change.
> Using '#!/usr/bin/python' is better than '#!/usr/bin/env python'
> since you know exactly which python binary you will be using.
> It was also pointed out the env convention will confuse
> rpm's dependency generator

Good enough for me.  Lans, it would be nice to document this last bit  
(about rpm) in the patch description.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com