git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jakub Narebski <jnareb@gmail.com>
Date
Dec 11, 2009, 10:09 UTC
Message-ID
<200912111109.17047.jnareb@gmail.com>
In-Reply-To
<4B21AC4D.2020407@kernel.org>
On Fri, 11 Dec 2009, J.H. wrote:
Show 14 quoted lines
> <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) {
Show 7 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
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).
Show 5 quoted lines
>> 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.

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

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

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

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
Previous: Junio C HamanoNext: Jakub Narebski
Message 24 of 40 in “Gitweb caching changes v2”
  1. 0/6 Gitweb caching changes v2John 'Warthog9' Hawley, Dec 10, 2009
  2. 1/6 GITWEB - Load CheckingJohn 'Warthog9' Hawley, Dec 10, 2009
  3. 2/6 GITWEB - Missmatching git w/ gitwebJohn 'Warthog9' Hawley, Dec 10, 2009
  4. 3/6 GITWEB - Add git:// link to summary pagesJohn 'Warthog9' Hawley, Dec 10, 2009
  5. 4/6 GITWEB - Makefile changesJohn 'Warthog9' Hawley, Dec 10, 2009
  6. Jakub NarebskiDec 11, 2009
  7. J.H.Dec 11, 2009
  8. Jakub NarebskiDec 11, 2009
  9. 4/6 gitweb: Makefile improvementsJakub Narebski, Dec 19, 2009
  10. Johannes SchindelinDec 11, 2009
  11. Jakub NarebskiDec 11, 2009
  12. 3/6 gitweb: Optionally add "git" links in project list pageJakub Narebski, Dec 18, 2009
  13. Jakub NarebskiDec 11, 2009
  14. 2/6 gitweb: Add option to force version matchJakub Narebski, Dec 18, 2009
  15. Johannes SchindelinDec 11, 2009
  16. Sverre RabbelierDec 10, 2009
  17. Jakub NarebskiDec 11, 2009
  18. Junio C HamanoDec 11, 2009
  19. J.H.Dec 11, 2009
  20. Junio C HamanoDec 11, 2009
  21. J.H.Dec 11, 2009
  22. J.H.Dec 11, 2009
  23. Junio C HamanoDec 11, 2009
  24. Jakub NarebskiDec 11, 2009
  25. 1/6 gitweb: Load checkingJakub Narebski, Dec 18, 2009
  26. Mihamina RakotomandimbyDec 11, 2009
  27. Sverre RabbelierDec 10, 2009
  28. Jakub NarebskiDec 11, 2009
  29. 6/6 GITWEB - Separate defaults from main fileJohn 'Warthog9' Hawley, Dec 10, 2009
  30. Jakub NarebskiDec 11, 2009
  31. J.H.Dec 11, 2009
  32. Jakub NarebskiDec 11, 2009
  33. Junio C HamanoDec 16, 2009
  34. J.H.Dec 16, 2009
  35. Jakub NarebskiDec 16, 2009
  36. J.H.Dec 16, 2009
  37. Jakub NarebskiDec 16, 2009
  38. Jakub NarebskiDec 11, 2009
  39. J.H.Dec 11, 2009
  40. Jakub NarebskiDec 12, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.