Re: [PATCH 1/6] GITWEB - Load Checking

From: J.H. <warthog9@kernel.org>
Date: Dec 11, 2009, 02:19 UTC
Message-ID: <4B21AC4D.2020407@kernel.org>
In-Reply-To: <m34onye3h8.fsf@localhost.localdomain>

<snip>

Show 6 quoted lines

>> 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.

Show 5 quoted lines

>> 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

> 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.

Show 5 quoted lines

> 
>> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
> 
> Why signoff is different from author (warthog9@kernel.org)?  Why this
> email for signoff?  Just curious...

My bad, did the patches up on my laptop but had to send them out from kernel.org, thus the miss-match: I.E. user error.

<snip>

Show 9 quoted lines

>> +# 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.

<snip>

Show 20 quoted lines

>> +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.

- John 'Warthog9' Hawley