On Fri, 11 Dec 2009, J.H. wrote:
> <snip>
>>> adds $maxload configuration variable. Default is a load of 300,
>>> which for most cases should never be hit.
>>
>> Your patch doesn't allow for *turning off* this feature. Reasonable
>> solution would be to use 'undef' or negative number to turn off this
>> check (this feature).
>
> Well there's the opposite argument that setting the number arbitrarily
> high, 4096 for instance would also in essence negate this (though I'll
> admit I've reached and exceeded those numbers before)
>
> That said I agree, being able to turn this off needs to be added and
> will be shortly.
Simplest solution would be to used 'undef' (undefined value) for
"turned off", i.e.:
if (defined $maxload && get_loadavg() > $maxload) {
>>> Please note this makes the assumption that /proc/loadavg exists
>>> as there is no good way to read load averages on a great number of
>>> platforms [READ: Windows], or that it's reasonably accurate.
>>
>> What about MacOS X, or FreeBSD, or OpenSolaris?
>
> Will comment on this further down
I think it would be better to write in commit message that because finding
load average is OS dependent, there is provided (sample) solution which
uses /proc/loadavg and works (at least) on Linux. And that for platforms
which do not have /proc/loadavg the feature is simply turned off (by the
way of using load=0 if load cannot be determined).
>> You should mention that it is intended that if gitweb cannot read load
>> average (for example /proc/loadavg does not exist), then the feature
>> is turned off, i.e. the check always succeeds. Which is reasonable.
>
> That's fine.
See above proposal. This information should be present in commit message,
and perhaps maybe even as one-line comment above opening /proc/loadavg.
>>> +# loadavg throttle
>>> +sub get_loadavg() {
>>> + my $load;
>>> + my @loads;
>>> +
>>> + open($load, '<', '/proc/loadavg') or return 0;
>>
>> Why not use one of existing CPAN modules: Sys::Info::Device::CPU,
>> BSD::getloadavg, Sys::CpuLoad?
>
> Here's the fundamental problem:
>
> Sys:Info:Device:CPU
> Windows:
> Using this method under Windows is not recommended
> since, the WMI interface will possibly take at least 2
> seconds to complete the request.
>
> BSD::getloadavg
> While this more or less supports anything with a libc getloadavg
> (and thus might be the best one I've seen, I'll admit I didn't
> notice this one when I looked years ago) getting it to work on
> windows looks, exciting.
>
> Sys::CpuLoad:
> http://cpansearch.perl.org/src/CLINTDW/Sys-CpuLoad-0.03/README
> Specifically:
> - Currently FreeBSD and OpenBSD are supported.
> - Wanted: HPUX 11.11 ...
> - Todo: Win32 support
>
> So this doesn't really buy me anything but, maybe, BSD support.
>
> So at the end of the day, none of those really gets me a "useful" cross
> platform load checker (though like I said BSD::getloadavg looks to be
> the best of the ones you mentioned) and more or less Windows is going to
> lose this as a usable feature no matter what.
>
> I think I'd almost rather set this up so that if it can't get something
> useful (I.E. /proc/loadavg is missing) it just skips past it as if the
> load was 0.
>
> I might try out the BSD::getloadavg but I want to take a look and see if
> that's easily installed or not, if it's not it might be difficult to
> justify that as a dependency.
After thinking about this a bit, now I don't think that it is terribly
important. You *might* describe alternate approaches (roads not taken)
in commit message, but requiring /proc/loadavg for the feature to work
is fine for first patch (it makes patch simpler).
>>> +if (get_loadavg()> $maxload) {
>>> + print "Content-Type: text/plain\n";
>>> + print "Status: 503 Excessive load on server\n";
>>> + print "\n";
>>> + print "The load average on the server is too high\n";
>>> + exit 0;
>>
>> Why not use die_error subroutine? Is it to have generate absolutely
>> minimal load, and that is why you do not use die_error(), or even
>> $cgi->header()?
>>
>> Wouldn't a better solution be to use here-doc syntax?
>>
>> + print <<'EOF';
>> +Content-Type: text/plain; charset=utf-8
>> +Status: 503 Excessive load on server
>> +
>> +The load average on the server is too high
>> +EOF
>> + exit 0;
>
> It was intended to be the most minimal possible, mainly get in, get out.
>
> Also not sure the die_error existed in gitweb when this was originally
> written. Probably worth switching to it now since it's there either
> way, and I don't think using it would add enough overhead to matter.
Well, if you are not worring excessively about overhead, then I think
using die_error would be the best solution, as it would preserve look
of gitweb. It would require extending die_error by 503 response, or
rather %http_responses hash and comment above die_error.
Also I think that Status: should be before Content-Type: header (but
probably it is not required by the standard).
--
Jakub Narebski
Poland
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html