From mboxrd@z Thu Jan 1 00:00:00 1970 From: Danny Milosavljevic Subject: Re: GSoC: Adding a web interface similar to the Hydra web interface Date: Tue, 12 Jun 2018 18:35:04 +0200 Message-ID: <20180612183504.2621cefa@scratchpost.org> References: <87vac3twbe.fsf@gnu.org> <87o9hog2ye.fsf@elephly.net> <87d0xyn9zs.fsf@elephly.net> <87d0xswvls.fsf@elephly.net> <87r2m4ntk4.fsf@mdc-berlin.de> <87tvqxy4i9.fsf@elephly.net> <87in78hxo2.fsf@elephly.net> <878t7xb58o.fsf@elephly.net> <874lijbqvf.fsf@elephly.net> <20180606200210.7a9c4dd6@scratchpost.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; boundary="Sig_/0AIBm0NUHhzKollWSDS0+Aw"; protocol="application/pgp-signature" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:57025) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fSmGU-0005QM-BD for guix-devel@gnu.org; Tue, 12 Jun 2018 12:35:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fSmGR-0008Rp-77 for guix-devel@gnu.org; Tue, 12 Jun 2018 12:35:26 -0400 Received: from dd26836.kasserver.com ([85.13.145.193]:33880) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fSmGQ-0008N4-Nt for guix-devel@gnu.org; Tue, 12 Jun 2018 12:35:22 -0400 In-Reply-To: List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: Tatiana Sholokhova Cc: guix-devel --Sig_/0AIBm0NUHhzKollWSDS0+Aw Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Hi Tatiana, nice work! I have a few comments: db-get-builds looks fine and we could merge this change to master. But you= have other changes in the same commit, so we can't directly cherry-pick it= . (not so bad, but somewhat cumbersome now) I'd suggest to rename "db-get-evaluations-info" to "db-get-evaluations-buil= d-summary". Please don't use nonsensical names like "succ" or "que" in pub= lic interfaces (f.e. #:que ). Nitpick: You have a few variables with "_" like "eval_cnt". This is not cu= stomary in Scheme. Also, having funny abbreviations like "cnt" isn't eithe= r. I suggest "evaluation-count". For db-get-evaluations-count: Please add a short docstring documenting what= it's for (i.e. "Return the number of evaluations of the given specificatio= n SPEC"). In src/cuirass/http.scm : I don't think that HTML5 is XML-compatible. So if not, please use XHTML pr= eamble and content-type (I think that that is even broken on master - but i= t's not difficult to fix). It's also easier on web browsers if they can as= sume well-formedness once their XML parser is done parsing. respond-static-file: We should not second-guess the VFS layer. Checking fo= r ".." gives an illusion of security when in fact random things could be mo= unted and also the VFS could have funny syntax for who knows what on the fi= lesystem. Let's rather have a static list of permissible names and allow t= hose (whitelist). That's the intention of the check anyway, right? Also, in light of an ever-changing backing store (cuirass continusly evalua= tes things), the way you are doing pagination is not the correct way to do = it because the data set will scroll underneath you and you will miss items = (or see duplicate items) as an user. Also, it's slow and the DBMS can't re= use anything because you are cutting it off and offseting it over and over = again and the transaction isolation level is too low for the DBMS to be abl= e to do anything about it[1]. See also https://use-the-index-luke.com/blog/2013-07/pagination-done-the-po= stgresql-way for a much better way. Basically, remember the last value and= use it as a limit in the WHERE condition by this one (also, sorting always= ). That means if you have the (sorted) list: A B C E G H and for some reason only 3 fit the page, print first page: A B C <--- print, and remember this one for the boundary of the next page Second page is "after C", so "... WHERE ... > 'C'": E G H <--- print; and remember that no next page exists. Note that this also works if, in the mean time, "D" appeared in the backing= store. Then second page is "after C", so "... WHERE ... > 'C'": D E G <--- print, and remember this one for the boundary of the next page This would only cause a problem if items vanished, so we should keep this i= n mind. And jumps to random pages by page number would be difficult now (b= ut who needs that in this case? Much better to be able to jump by actual t= imestamp, which then would be easily possible). In the case of evaluations, it would be good to sort by timestamp and then = by evaluation id - since no matter what this would be a stable sort criteri= a. (maybe revision, too) [1] We should fix the latter eventually, too. --Sig_/0AIBm0NUHhzKollWSDS0+Aw Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAlsf9jgACgkQ5xo1VCww uqUytQf+NoXa+AI96RrrUPWZUVGJ5Qwg3oQZLqogRGuiAIpi1TXk2VSidtS9/hzK AkeY3RT7tptd1YYw4CtYW0QWAgt+D3RK+Ie8DrdPAeqKPivwbq7U9QPqP9PskbHc 9PbMXSHmH/JYmDBekUjEL95p00bx3TAK+yyYcaZP/ngj6c4gaVyJLwigaluavjL1 yBNaVu5asZ0cepGEaElTvFGYr4VnHBhJ74JmyxLPZHmRvRQLZ8P++DKonnofKZmq 3Hxph1lbnNZJXIArUhgOBGTsIKlSxPz4y423TfoINRT/iycLbO285pEdciadFVHB jsg3+2CseQFur5W/rQrM//8cmVUu+w== =xllZ -----END PGP SIGNATURE----- --Sig_/0AIBm0NUHhzKollWSDS0+Aw--