From: Jakub Narebski <jnareb@gmail•com>
To: "J.H." <warthog9@kernel•org>
Cc: git@vger•kernel.org, "John 'Warthog9' Hawley" <warthog9@eaglescrag•net>
Subject: Re: [PATCH 1/6] GITWEB - Load Checking
Date: Fri, 11 Dec 2009 11:09:16 +0100 [thread overview]
Message-ID: <200912111109.17047.jnareb@gmail.com> (raw)
In-Reply-To: <4B21AC4D.2020407@kernel.org>
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
next prev parent reply other threads:[~2009-12-11 10:08 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-12-10 23:45 [PATCH 0/6] Gitweb caching changes v2 John 'Warthog9' Hawley
2009-12-10 23:45 ` [PATCH 1/6] GITWEB - Load Checking John 'Warthog9' Hawley
2009-12-10 23:45 ` [PATCH 2/6] GITWEB - Missmatching git w/ gitweb John 'Warthog9' Hawley
2009-12-10 23:45 ` [PATCH 3/6] GITWEB - Add git:// link to summary pages John 'Warthog9' Hawley
2009-12-10 23:45 ` [PATCH 4/6] GITWEB - Makefile changes John 'Warthog9' Hawley
[not found] ` <1260488743-25855-6-git-send-email-warthog9@kernel.org>
2009-12-10 23:45 ` [PATCH 6/6] GITWEB - Separate defaults from main file John 'Warthog9' Hawley
2009-12-11 15:46 ` Jakub Narebski
2009-12-11 15:58 ` J.H.
2009-12-11 22:53 ` Jakub Narebski
2009-12-16 1:22 ` Junio C Hamano
2009-12-16 2:00 ` J.H.
2009-12-16 19:52 ` Jakub Narebski
2009-12-16 20:04 ` J.H.
2009-12-16 2:22 ` Jakub Narebski
2009-12-11 14:28 ` [PATCH 4/6] GITWEB - Makefile changes Jakub Narebski
2009-12-11 16:22 ` J.H.
2009-12-11 16:41 ` Jakub Narebski
2009-12-19 13:32 ` [PATCH/RFCv2 4/6] gitweb: Makefile improvements Jakub Narebski
2009-12-11 12:52 ` [PATCH 3/6] GITWEB - Add git:// link to summary pages Johannes Schindelin
2009-12-11 13:44 ` Jakub Narebski
2009-12-18 21:02 ` [PATCHv2 3/6] gitweb: Optionally add "git" links in project list page Jakub Narebski
2009-12-11 10:52 ` [PATCH 2/6] GITWEB - Missmatching git w/ gitweb Jakub Narebski
2009-12-18 19:18 ` [RFC/PATCHv2 2/6] gitweb: Add option to force version match Jakub Narebski
2009-12-11 12:49 ` [PATCH 2/6] GITWEB - Missmatching git w/ gitweb Johannes Schindelin
2009-12-10 23:54 ` [PATCH 1/6] GITWEB - Load Checking Sverre Rabbelier
2009-12-11 0:52 ` Jakub Narebski
2009-12-11 1:10 ` Junio C Hamano
2009-12-11 2:19 ` J.H.
2009-12-11 2:50 ` Junio C Hamano
2009-12-11 2:58 ` J.H.
2009-12-11 3:07 ` J.H.
2009-12-11 3:09 ` Junio C Hamano
2009-12-11 10:09 ` Jakub Narebski [this message]
2009-12-18 16:36 ` [PATCHv2 1/6] gitweb: Load checking Jakub Narebski
2009-12-11 13:53 ` [PATCH 1/6] GITWEB - Load Checking Mihamina Rakotomandimby
2009-12-10 23:53 ` [PATCH 0/6] Gitweb caching changes v2 Sverre Rabbelier
2009-12-11 15:51 ` Jakub Narebski
[not found] ` <4B226D56.7000004@kernel.org>
2009-12-11 18:01 ` Jakub Narebski
2009-12-11 18:26 ` J.H.
2009-12-12 1:37 ` Jakub Narebski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=200912111109.17047.jnareb@gmail.com \
--to=jnareb@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=warthog9@eaglescrag$(echo .)net \
--cc=warthog9@kernel$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox