=== danilo_ is now known as danilos [09:11] Anyone around for a review? [09:11] https://code.launchpad.net/~wgrant/launchpad/rebuilds-without-nai/+merge/46070 === matsubara-afk is now known as matsubara === mrevell is now known as mrevell-luncheon === mrevell-luncheon is now known as mrevell === jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === matsubara is now known as matsubara-lunch === matsubara-lunch is now known as matsubara === deryck is now known as deryck[lunch] === benji is now known as benji-lunch === gary_poster is now known as gary-lunch === deryck[lunch] is now known as deryck === benji-lunch is now known as benji [17:54] jcsackett, if you please: https://code.launchpad.net/~leonardr/launchpad/fix-apidoc/+merge/46161 === jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [17:55] leonardr: sure thing. === sinzui changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: leonardr || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [18:24] jcsackett: can I push one onto your queue? [18:24] jtv: sure. link to the MP? === jtv changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: leonardr || queue: [sinzui, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [18:25] jcsackett: https://code.launchpad.net/~jtv/launchpad/bug-702228/+merge/46165 === gary-lunch is now known as gary === gary is now known as gary_poster [18:36] leonardr: r=me. i've requested follow up from sinzui. [18:36] jcsackett,great [18:36] sinzui: i probably should not review your branch, since you're still my mentor. [18:37] jcsackett: I think you should because someone needs to know about mailman oddities. [18:37] I will find a mentor after your review [18:37] sinzui: fair enough. [18:37] sinzui: okay. === jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: sinzui || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [18:41] leonardr: ping [18:43] sinzui, yo [18:43] leonardr: what is the magic with scalar_value? [18:43] ScalarValue I mean [18:44] sinzui: it's like HostedFile--a special resource type that we don't want to document [18:44] Is there an example in the live docs? [18:45] no, it doesn't show up. i'm just making 100% sure it doesn't show up in the future [18:46] okay [18:46] Can we flag it with @hidden or something that makes WADL generation skip it? [18:46] leonardr: r=me. I noted that the copyright needs updating [18:47] StevenK: we want it in the WADL, we just don't want it in the HTML docs [18:47] it's a resource that the end-user never sees [18:47] @undocumented ? :-) [18:49] i don't think it's the server side's job to make that kind of distinction [18:52] sinzui: so MailmanTestCase doesn't actually isolate from the real proxy? [18:53] I agree with leonardr. We want the xslt public so users can see how we interpret it, but other's may want to make their own interpretation of the data [18:53] jcsackett: it does. I was stumped by the impossible timeouts when I sketched out the test you are reading. [18:54] sinzui: okay. [18:54] reading XMLRPCRunner.__init__, I realised it already has a reference to the real proxy [19:00] jcsackett: https://code.launchpad.net/~leonardr/launchpadlib/remove-xslt/+merge/46167 [19:01] sinzui: shouldn't the isolation you do in your test be part of the MailmanTestcase setup? [19:01] sinzui: not asking you to make that change, but if it's something that should be done, perhaps file a bug? [19:01] i acknowledge i don't have a full understanding of what we want in a mailman test, but unplugging from the proxy seems correct. [19:04] jcsackett: which setup. I think the test setup id DRY [19:05] sinzui: nevermind, i was misunderstanding what was being done. [19:05] sinzui: i thought that you were patching around something the base class for your test should have done. that is not the case. [19:06] yeah, get_mailing_list_api_test_proxy() looks like duplication, but it is changing an instance, where as super() is changing the module [19:06] reset_log and get_mark may move to the base class. I am pondering a test in a new branch that may need them [19:08] jcsackett: when you are done this my branch, I want to to talk about the implementation of my current bug, the one you looked at this morning [19:08] sinzui: sure. [19:13] sinzui: r=me, with two minor things. [19:13] notes are on on your diff, but it's basically some docs/comments stuff. [19:14] leonardr: your MP shows zero lines diff. [19:14] hmm... [19:14] jcsackett: I will follow up on you comments [19:15] sinzui: sounds good. feel free to ping me when you want to mumble re: your current branch. [19:15] leonardr: the file you wanted to remove (as i understand it) is still in your branch. http://bazaar.launchpad.net/%7Eleonardr/launchpadlib/remove-xslt/files/head%3A/src/launchpadlib/ === jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: jtv || queue: [leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [19:16] jcsackett: i didn't commit anything. new version is pushed [19:19] jcsackett: can we mumble for a few minutes? [19:28] jcsackett: http://pastebin.ubuntu.com/553725/ [19:56] Can someone take a look at https://code.launchpad.net/~pcjc2/launchpad/fix-tag-search-bug-501945/+merge/46075 for me? === leonardr is now known as leonardr-afk === leonardr-afk is now known as leonardr === jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: jtv || queue: [leonardr, pcjc2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [20:20] pcjc2: you're on the queue. [20:20] pcjc2: you're a new contributor? [20:39] jtv: r=me, but it looks lifeless hit it a bit earlier. i won't request follow up from sinzui since lifeless already approved it. [20:39] leonardr: r=me. [20:39] newish - this is my second patch === jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: pcjc2 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [20:40] pcjc2: cool. so, in the channel info here there is frequently an on call reviewer, who you can usually ping to get into the queue. [20:40] in this instance, that's me. :-) [20:40] I hashed out some of this with lifeless (He got the SQL tested), but he's a bit busy at the moment to review it [20:41] Thanks - I've not been here before [20:41] The other patch I did was with gmb helping, and it related to bug import, not an actual part of the web service. [20:58] * pcjc2 approves of Peter B's config API [20:58] Might want to consider the possibility of freeze and thaw for the callbacks [20:58] Also, possibly add save APIs? [20:59] export either a given context, or a "flattened" (borrowing from gimp layers) context ? [21:00] (OOPS: -E_WRONG_WINDOW) [21:09] EdwinGrubbs: thumper: can either of you mentor jcsackett review of my branch? https://code.launchpad.net/~sinzui/launchpad/mailman-heartbeat-0/+merge/46160 [21:09] sinzui: I can do it === jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [21:23] pcjc2: there are comments on your MP. [21:24] thanks [21:24] * pcjc2 checsk [21:24] sinzui: it seems strange to log "--MARK--" since syslogd can be configured to add that to a logfile automatically. Of course, when the xmlrpcrunner does it, it would be preceded by a timestamp. [21:25] "You should flip those back; by convention we put the expected value first in assertEqual statements." [21:25] correct fix is then to swap the parameters passed to each caller then [21:25] (and swap the assert back) [21:25] EdwinGrubbs: I was following the suggestion of the losa. [21:25] * sinzui looks at mailman's own logging lib again [21:25] pcjc2: that may be correct, my comment was limited to the scope of your MP. [21:26] I swapped them because every test had the parameters reversed, I didn't know there was a convention [21:26] EdwinGrubbs: mailman does not use python batteries. I think it still thinks we live with Python 2.1 [21:26] RE: comment - a docstring at the start of the function ok to explain the SQL? [21:27] pcjc2: that should work, yes. please remember that the first line of a docstring should be one line quick explanation of the method, then a blank line, then the longer details. [21:27] and thanks for making these changes. :-) [21:27] like a git commit ;). NP [21:28] exactly like a git commit. :-) [21:28] pcjc2: ping me when you have pushed the changes, i can approve the MP and alert my mentor to follow up. [21:29] even if i'm in the middle of another review i can do that quickly. [21:29] ok, will do. Am merging some git conflicts another project right now, but should look at it tonight [21:30] pcjc2: ok. [21:36] sinzui: I just checked, and rsyslog puts the timestamp in front of its autogenerated "-- MARK --", so I think it is a bad idea to use that as your heartbeat text. If the losas insist on that, I'll agree to it, but I'm guessing they didn't think about the confusion it could cause. [21:37] EdwinGrubbs: I think we want a timestamp so that nagios nows when the heatbeat took place [21:37] * StevenK hands sinzui a 'k' [21:38] sinzui: yes, but if nagios is grepping for "-- MARK --" with a timestamp, and syslogd is configured to add "-- MARK --" entries every few minutes, and they also have a timestamp, the xmlrpcrunner could be broken and you would never know it. [21:38] I am getting rid of silent letters just after I get rid of c, j, q, x [21:39] You can't QQ without q [21:40] EdwinGrubbs: I do not know what syslogd has to do with this. It is not managing Mailman's logs [21:40] Why would the losas request that we add --MARK-- to the queue's log if they could do it themselves [21:41] q is a dipthong for kw [21:43] I think asking a losa is a good idea at this point :) === matsubara is now known as matsubara-afk [21:45] lifeless: Mailman's Syslog puts a write lock on the log. As my test shows, some insider knowedege and access is needed to acquire access, [21:45] sinzui: what is on the receiving end of the syslog() call in the xmlrpcrunner then? Configuring syslogd to log MARK just shows that syslog is working, it doesn't show that the programs using that logfile are working. [21:46] And be assured I toppled mailman several times learning the art of stealing the logs [21:46] wheeee [21:47] EdwinGrubbs: I think the point is that we don't know if xmlrpcrunner is hung or not [21:47] Edwin Mailman.logging.syslog is a dict-like object that manages Mailman.logging.StampedLoggers. Each logger manages a file with 'a+b' [21:48] Edwin each object in the chain ensures the file is closed if you were to call del on the instance [21:49] EdwinGrubbs: Mailman really thinks about logging like it was the 10 years ago. It does not even have bools [21:50] sinzui: ok, that is bizarro [21:50] But it would life to use with `open(ccc) as log:` Mailman 3 might do that [21:50] EdwinGrubbs: I believe mailman requires Python2.1 to run [21:52] EdwinGrubbs: I was looking for barry to asking about SHUNTING. I will bring up the log issue with him [21:52] when I find him [21:58] sinzui: you can keep using MARK if you want, since mailman's syslog() doesn't log to syslogd. This reminds me of the time a friend created a readFoo() function that had a parameter to turn on writing. [21:58] I'll approve the mp [21:58] * sinzui is still looking for barry [22:00] EdwinGrubbs: thanks. I really want to talk to barry. I an reading an old log and I makes me wonder if his and mthaddon's analysis is complete. I see log entries that imply my change will work by accident, in some cases [22:03] sinzui: I can find Barry in Dallas if you wish [22:03] (And tempt him onto IRC) [22:03] please do [22:03] sinzui: Aye [22:05] sinzui: be with you in a few minutes [22:05] fab [22:05] \o/ [22:05] jcsackett: thanks [22:09] sinzui, okay, i'm all yours [22:10] Looking at a mailman error log. I see that messages were shunted because of [22:10] XMLROC proxy errors in LPHandler. I do not think I want the message shunted [22:10] since Runner implies IncomingRunner or LPHandler should have done something [22:10] itself. [22:10] I will wrap the proxy call to catch and raise oopses when xmlrpc goes down, but I think I want the message reenqueued [22:12] i can't think of a good way to prevent Runner._oneloop() from shunting when _onefile() throws an exception [22:12] short of hacking the code, or catching the exception in _onefile() [22:12] yuck [22:13] well, actually, maybe you can set self._shunt to the retry queue [22:13] I hoped for a way to reenque from the handler, or raise a specific error that tell IncomingRunner to do it [22:13] * barry thinks [22:14] How will a shunted message ever be processed. No one manages mailman [22:14] it has to be done manually via bin/unshunt [22:14] * barry shudders to think how big the shunt queue is [22:14] I think then we have several years in there [22:14] you almost certainly don't want to unshunt the whole queue ;) [22:15] How can we see how large the queue is? [22:15] barry: hi! [22:15] So as I was thinking. We just need to pause the queue until connectivity is back. just loop over the message [22:15] thumper, hi! [22:15] StevenK, ls queue/shunt [22:15] er, queues/shunt [22:16] note tho that the queue files have a timestamp encoded in the file names so you could potentially decode that and only unshunt the ones from whatever date you care about [22:17] not sure what you mean by "until connectivity is back" [22:17] barry, what would you do if you LPHandler has no connectivity to complete its task. this is like a lock on a list. I see we try to return 1 to imply try again later [22:18] sinzui, right. just leave the message in the queue and it will be picked up the next time 'round [22:18] or, enqueue it to queues/retry, but all that does is requeue the message every so often [22:18] so it's about equivalent [22:19] barry: Sometimes the xmlrpc request times out, but the next try works. But on Dec 1, it went down and mailman stopped trying. [22:19] sinzui, any idea why it stopped trying? [22:20] another idea... [22:20] barry: Log looks like Mailman got board shunting [22:20] restart worked fine [22:20] ;) [22:20] so, the .pck files have two objects in them: a dict (the message metadata) and the message object [22:20] My branch to add a heartbeat will warn us in the future [22:21] barry: how would I signal/do an enqueue from a handler [22:21] it would be a bit slow but you can unpickle the .pck file and look in the dict for tell-tale signs that LPHandler is where the failure occurred [22:22] barry: I can see where the failure happen. There are only two calls in out monkeypatches that do not try/except arround the proxy. I am sure we can log this. I just want solve how to keep the message alive [22:23] sinzui, you just instantiate the queue you want and call .enqueue() on it. note tho that that makes a copy of the message, so absent anything else, it continues to get processed by the current runner [22:23] barry: from inside the handers except clause? That sounds like recursion [22:24] sinzui, so i think you catch the exception, instantiate the retry queue, enqueue the message, then reraise the exception (or raise DiscardMessage) [22:24] just be sure we're running the retry runner (i don't remember and i don't have the lp code in front of me) [22:24] StevenK, do you have the lp code handy? [22:25] barry: Yes [22:25] ahh, good point. If I raise the exception, it get shunted. raising discard just removes the copy in in the queue? [22:25] sinzui, yep [22:25] look at IncomingRunner.py [22:25] barry: You'll need to come to me, my battery is 4% or so [22:25] StevenK, what room are you in? [22:25] barry: I know how to check that and I know how to make it run [22:25] barry: Desktop [22:25] StevenK,sec... [22:29] barry: we are running the retryrunner. I wrote a test to ensure it is. Thanks I really appreciate your help [22:29] np. looking at the code now [22:29] it's been a while ;) [22:30] sinzui, which call is failing? [22:30] proxy = proxy = ? ;) [22:30] File "/srv/lists.launchpad.net/production/launchpad-rev-9972/lib/lp/services/mailman/monkeypatches/lphandler.py", line 49, in process [22:30] is_member = proxy.isRegisteredInLaunchpad(sender) [22:31] barry: there that is one of two places we call proxy that we do not wrap in try/except [22:31] sinzui, yeah, for some reason i think the preceding comment is a lie ;) [22:31] barry: I was going to reuse the except rules an log_exception from xmlrpcrunner on it [22:32] * barry thinks [22:32] barry: This is one of many entries in the log http://pastebin.ubuntu.com/553793/ [22:33] yeah, it's not a lie because xmlrpcrunner isn't incoming runner [22:35] This line and the one in lpstanding is just an oversight. All other calls are safe to use, but this one cold lose the message [22:43] * pcjc2 struggles to think of concise words to describe the SQL statements reasoning in his docstrings [22:44] sinzui, so yeah, i think the best you can do is to catch the exception, enqueue the message to retry, and raise DiscardMessage. then the retry runner should requeue it, and when it's re-processed by incoming runner it should start back up at the LaunchpadMembers handler [22:44] sinzui, write some tests, but i think that should work ;) [22:49] pcjc2: "Because the final clause only checks the existence of results of the query, subqueries can select true rather than an actual entity, speeding up the process." [22:49] maybe. [22:49] it's a little clunky. :-P [22:49] I'm moving the EXISTS keyword into the build_tag_set_query [22:49] function [22:49] then it returns a TRUE or FALSE, and it is cleaner - as well as easier to explain [22:50] might not even NEED to explain the SQL in detail if that change is made? [22:52] http://pastebin.com/EfHtBNdE [22:52] ? [22:52] THink I will drop the comment about where it is used [22:52] that could get out of date fast [22:52] Will add one about expecting the surrounding SQL to ensure "Bug.id" is in scope [22:55] what about http://pastebin.com/QswKQ2Lg ? [23:00] See the diff: http://pastebin.com/9iViJh6Y [23:00] * pcjc2 re-runs tests to be sure === jcsackett changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [23:06] pcjc2: i would put quick comments above the string format statements, since you end up with things like "%s %s %s". [23:06] comment to what effect? [23:06] at some point, one has to read the context of the code ;) [23:07] pcjc2: that's an excellent question, actually. [23:07] yeah, this is just an inherently complicated bit. [23:07] looks weird, but you're right, it doesn't need anything more. [23:07] test passing? [23:07] "%s %s NOT %s" is a bit strange, I conceed [23:07] give it a month or two to run... [23:07] yeah, it's odd, but i think it's alright. [23:07] lol. [23:08] (just wanted to check the "testbug.py" test, as it compares SQL output - had to make sure I'd not shifted the way the SQL was braketed [23:08] I'm not going to re-run the other tests as the SQL is the same.. [23:09] * jcsackett nods. [23:10] Will need some manual QA though, as I'm not sure how good the "life" test coverage is [23:11] ? [23:11] which test are you referring to by "life" test? [23:11] "live" I meant [23:11] with a big data-set [23:11] it aims to fix a problem which needs a production sized DB to reproduce [23:13] return "%s" % include_clause [23:13] pcjc2: we qa all such things ;) [23:13] should become "return include_clause" [23:13] pcjc2: on qastaging.launchpad.net [23:13] perhaps [23:13] return str(include_clause) [23:14] include_clause is assembled from strings [23:14] or even unicode(include_clause) [23:14] we don't prefix the return "(%s %s NOT %s)" % ... case with u" [23:14] pcjc2: sure, explicit can be good sometimes (also its a noop if the type matches) [23:15] sinzui: https://code.launchpad.net/~thumper/launchpad/daily-build-page-text/+merge/46207 for your UI approval :) [23:15] okay [23:15] lifeless - name your preference... unicode, str ? [23:16] meh - actually, it is only the one "NOP" return "%s" % include_clause [23:16] the other cases around it need that syntax, so it is best we keep them consistent [23:16] even if "%s" % ... is a NOOP [23:17] what is the SQL'esque way of fixing this comment... [23:17] This SQL is designed to be a sub-query where the parent SQL defines Bug.id. [23:17] "defines" seems wrong [23:18] JOINs/includes/contains/? [23:18] pcjc2: i have to run for the evening. i'll check the MP in the morning for the changes. ttyl. [23:18] "parent query scope includes Bug.id" ? [23:19] @jcsackett: Thanks for the review - with luck the MP will be in good order by tomorrow [23:19] pcjc2: sounds good. [23:19] pcjc2: I would leave it as is [23:19] as it was? [23:19] pcjc2: it looked ok at a glance [23:20] as was, yes [23:20] ok, will push that diff [23:20] jcsackett's two comments were good suggestions [23:20] Do I need to mark the MP as needing further review? [23:20] Review comments have been taken into account, new push coming... [23:26] sinzui: Hi. Would you be able to review a CSS bug fix sometime (bug #684911: https://code.launchpad.net/~huwshimi/launchpad/broken-calendar-684911/+merge/46208)? [23:27] <_mup_> Bug #684911: calendar overlay widget style broken after lazr-js 191 upgrade

< https://launchpad.net/bugs/684911 > [23:27] * sinzui hates that widget [23:27] you cruel man. [23:27] sinzui: Is that a no then? :P [23:28] ah, so the hack to for the yui-3 skin was lost in the last update? [23:29] sinzui: Looks like something like that. In which case, me fixing the styles this way would only be a temporary fix [23:29] I don't really understand how all the lazr-js stuff works yet but maybe the real fix is something to do with that [23:30] We wanted a yui-3 widget, but one does not exist. We could make a lazr one that handles NOW [23:30] this is good to land. Thanks huw. [23:31] Yeah ok that makes sense. Thanks [23:32] Oh, the fonts are wrong [23:32] Ah yes [23:32] huwshimi: I will find the correct font-family string [23:32] I remembered that we had to hack the fonts as well as the skin 18 months ago [23:34] @jcsackett: My MP should be ready to go when you get to it tomorrow - thanks [23:36] huw: do you have a strong understand of how browsers (last 5 year) handle font-size. YUI requires us to use % to guarantee consistent rendering, but that also means the engineer never knows what the actual font-size will be when all content and rules are mixed together [23:37] huw I want to switch back to ems, or even px as suggested by Canonical web guidelines if I could trust consistent sizes [23:43] sinzui: Yeah font sizing gets very confusing very quickly. [23:43] Especially when you won't necessarily have control over third party widgets etc. [23:44] I used to avoid px. I was surprised the Canonical insists on it. I think they may be looking a how browser engines handle px though. My phone shows me pages and I know it is ignoring the px rules [23:45] With the calendar I can either set the font to the Ubuntu one or remove the font-family settings altogether (which will then inherit the correct fonts) [23:45] Any preference [23:45] ? [23:45] YUI is specifically reconciling Window with every other os/basefont. I am not sure I care about windows [23:45] sadly we care [23:45] because our customers have customers and suppliers on windows [23:46] huwshimi: I think we need to set it. Somewhere in the chain, verdana or tahoma gets added [23:47] lifeless: I think the issue windows fonts being a little big is less than the issue that we are not using the font-sizes that the CSS predicts will rendered. % rules are being nested. We cannot avoid that [23:47] sinzui: On a quick test of removing all font-family lines from calendar.css it seems to be working correctly. [23:48] lifeless: http://curtis.hovey.name/2010/12/17/launchpad-font-size-broken-by-design/ [23:48] huwshimi: fab [23:49] sinzui: Should I go ahead and commit without font-family rules then? [23:49] huwshimi: yep [23:49] ok great thanks. [23:50] sinzui: I can see the brain damage on linux too ;) [23:51] sinzui: would love it if we fix this. [23:52] lifeless: I am hoping for a short brain storm session + a spike at the thunderdome to find a compromise that will let me close a UI bug [23:59] sinzui: When I try and land the branch it complains that the branch is not approved. On the MP the status is approved. Do I need to do anything else to get it to land?