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

On Fri, 11 Dec 2009, J.H. wrote:

<snip>

quoted

quoted

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

quoted

quoted

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

quoted

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.

quoted

quoted

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

quoted

quoted

+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