/srv/irclogs.ubuntu.com/2010/05/27/#launchpad-reviews.txt

stubhttps://code.edge.launchpad.net/~stub/launchpad/read-only-launchpad/+merge/2603303:56
=== _thumper_ is now known as thumper
rockstarstub, can I get you to do a review for me?06:27
stubyo07:03
rockstarstub, https://code.edge.launchpad.net/~rockstar/launchpad/sourcepackagerecipe-name-contstraint/+merge/2612207:05
stubrockstar: Is using a propertly like that how we do this elsewhere?07:10
stubrockstar: Normally, we don't do this sort of check except in the UI code.07:15
stubrockstar: And the database exception gets raised if something is broken and lets through invalid data07:15
rockstarstub, well, I did the pre-imp with abentley, and that was his suggested method, which I liked.07:15
stubYou are issuing a database query every time name or owner is set, despite the fact nothing should ever get that far with values that cause a problem.07:16
stubThat is a lot of database queries to protect against input that should never arrive, and will be caught at the database layer.07:17
stubI can see the point for an emergency cherry pick to edge, but I don't like it for long term07:17
rockstarstub, so you're saying this should be implemented in the browser code only?07:17
stubYes, like all our other unique inputs.07:17
rockstarstub, okay.  I'll look into that in the morning then.07:18
rockstarstub, thanks for the review.  Please disapprove it.07:18
noodles775Hi stub, I've just added a comment on your MP - I can't see anywhere where was_read_only is set initially?08:16
noodles775Also, do you have a metric for dogfood minutes/production minutes? (see my last comment at: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-changes-build-generalisation-new/+merge/25594 )08:18
=== noodles775 changed the topic of #launchpad-reviews to: On Call: noodles775 || reviewing: stub || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
stubnoodles775: It isn't set initially - that is why I use getattr(locals, 'was_read_only', None) instead of locals.was_read_only. There is nowhere sane to initialize it because there is no per-thread initialization08:35
noodles775stub: yes, I understood the getattr, but afaics, it is *never* set?08:36
* noodles775 looks again08:36
stubIt is set at the end of that if statement08:37
stub            self.thread_locals.was_read_only = is_read_only()08:37
noodles775stub: yes, but that if statement is only executed if was_read_only is not None?08:37
stubpoint.08:37
* stub wonders how his tests work at all08:37
noodles775Ah, there were tests? Can you update the MP with the test command... I only requested another foundations person because I assumed there were no tests for this functionality.08:39
stubThere are tests. I'm fixing the last one of them now ;)08:40
noodles775Great :)08:40
stubnoodles775: All pushed. There are other readonly mode tests too, but they are for bits I didn't touch09:01
stub(trying to keep this minimal since we don't have much time to test, and I'm lazy)09:02
noodles775stub: great, I'll take another look. Also, did you see my question about dogfood vs. production minutes above?09:02
stubDogfood isn't great for timings. Staging timings are better.  If we think it is too slow, or we can't wait to land on staging to be sure, I'll need to refactor the migration.09:05
stubWe don't really have a metric for dogfood. Staging we tend to double the times (faster machines, but three replicas to apply to)09:06
noodles775stub: OK. Unfortunately we can't land it on staging just to find out (as it needs to land together with over 9k of code changes).09:07
stubI can get a timing... I'll kick it off now so we will know.09:08
noodles775Ah, great!09:08
noodles775stub: you've got some staging database you can trash afterwards have you? or are you just not committing the transaction?09:09
stubI'll skip the commit09:09
stubMight make staging whOOPS a bit, but whatever.09:12
stubnoodles775: Is this landing this cycle?09:16
noodles775We were hoping to... we've got it on dogfood at the moment, but haven't yet managed to push through a few hundred packages. At the moment its not looking likely, unless we get lots of packages through df today.09:18
noodles775stub: it's expected that test_publication passed both with and without your changes right?09:19
stubYes. I'm fixing a race condition that is common under load but that we have no tests for.09:22
noodles775OK, r=me09:22
=== jelmer_ changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: stub || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== jelmer_ changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: stub, deryck || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
stubnoodles775: It took 17 minutes on staging, so maybe 35-40 mins to apply to production.10:01
noodles775stub: thanks. I assume that's way too long? What is acceptable?10:01
stubThats up to the release manager and losas10:02
noodles775OK.10:02
stubnoodles775: This version should be identical and takes 8 minutes, so 16-20 mins on production: http://paste.ubuntu.com/440285/10:10
wgrantnoodles775: When you've a moment, can you please re-EC2 lp:~wgrant/launchpad/diffs-in-queue? I've fixed the failures from a few days ago.10:11
noodles775Thanks stub... I'll update and send it off to ec2... but at this time I don't think I'll be landing it.. there seem to be other issues with teh current db-devel on dogfood that need looking in to.10:11
noodles775wgrant: sure.10:11
wgrantnoodles775: Thanks.10:12
=== jelmer changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: stub, stevenk || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== noodles775 changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: stevenk || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
wgrantwin 3410:39
wgrantArgh.10:39
* noodles775 is glad it's not just him who does that ;)10:39
bigjoolswhy don't you leave the 80s behind and get a graphical IRC client ;)11:51
noodles775heh, you use vim too right? ;)11:51
maxbbigjools: Find me a graphical irc client I can run in screen :-)11:56
maxbActually, I do run a graphical IRC client, I just connect it to my irssi in proxy mode :-)11:56
jelmer_hi deryck12:00
deryckHi jelmer12:01
deryckjelmer, thanks for the review!12:04
deryckjelmer, so I used transaction.commit because that used in other tests.  I feel dumb not knowing, but is Store.of().flush() better to use here?12:05
=== mrevell is now known as mrevell-lunch
bigjoolsah, the right tool for the job I say :)12:23
deryckbigjools, is that in reply to my question?12:38
bigjoolsderyck: ah no, I was replying to others before you12:38
bigjoolsderyck: but FWIW flush() is always preferred to commit() if it works!12:39
bigjoolscommit() makes tests *really* slow12:39
wgrantUnless you're like bigjools and have a ridiculous SSD?12:43
bigjoolss/ridiculous/absolutely gorgeous/12:43
deryckbigjools, right.  And I want to understand too -- flush may or may not commit, or it never commits?  i.e. flush just writes from memory to disk without commit?12:43
bigjoolscorrect12:43
bigjoolswell, flushes from the cache12:43
deryckgotcha.  I think I need commit in this case then, but will test and confirm.12:44
wgrantflush() causes Storm to execute pending SQL.12:44
wgrantYou should rarely need to do that manually.12:44
deryckRight12:45
wgrantIt probably (well, hopefully) won't hit disk.12:45
deryckah, right.  I see the distinction, thanks.  My mind is a bit fuzzy today.12:46
=== mrevell-lunch is now known as mrevell
=== matsubara-afk is now known as matsubara
=== StevenK changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: stevenk || queue: [StevenK*3] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== jelmer_ changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: stevenk || queue: [StevenK*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
StevenKjelmer: I've pushed a fix for the -config issue13:41
jelmer_StevenK: cool - I've already approved that one, just waiting for Michael to review13:47
* jelmer_ wished Launchpad allowed Tweak votes like BundleBuggy used to13:47
noodles775StevenK, jelmer_ : I just approved - with a note about lp_production_configs.13:47
wgrantnoodles775: Thanks -- both landed.13:51
noodles775wgrant: np.13:52
=== Ursinha-afk is now known as Ursinha
=== sinzui changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: stevenk || queue: [StevenK*2, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== jelmer_ changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: sinzui || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
jelmer_hi Curtis14:49
jelmer_thanks for the non-Soyuz branches :-)14:49
noodles775sinzui: when is PQM closing? I remember seeing an announcement, but can't find the email.15:00
sinzuinoodles775, I am confirming that now for an email. I asked for 22:00 UTC15:00
noodles775sinzui: on which day?15:00
sinzuitoday15:00
noodles775sinzui: I've a branch that includes a schema patch that, according to stubs testing (when he approved it) will take 16-20mins on production. Are you ok with that?15:03
sinzuiouch. yes I am15:04
noodles775Thanks.15:04
sinzuinoodles775, StevenK. If you are concerned about not meeting the PQM deadline in 8 hours, I can look at your branches before they go to ec2 and provide a pre-emptive RC so that you are not penalised by ec2 variability15:11
noodles775Thanks sinzui.15:12
StevenKsinzui: I have a branch that just hit ec215:13
StevenKBut I so don't want to stay up and babysit it15:14
sinzuiokay, I think that will be fine15:14
noodles775jelmer_: are there other examples of wrapping a method for api exposure that prefix with an _?15:15
noodles775(in ref. to your checkUpload branch).15:15
sinzuinoodles775, I think that means you want to rename the method to be public now15:16
sinzuinoodles775, I will refrain from suggesting a uses the rename hack since exposing a method explicitly makes it public15:16
jelmer_noodles775: Maybe, I haven't actually checked...15:17
jelmer_noodles775: I'll have a look now.15:17
noodles775sinzui: in this case it seems it's just to use string args instead of the normal IFaces... it seems strange to prefix with _ imo. (sinzui: https://code.edge.launchpad.net/~jelmer/launchpad/can-upload-webcall/+merge/26141  for reference :) ).15:18
noodles775jelmer_: also, why are you not returning True/False? It also seems strange to not return *anything* to mean yes, and raise an exception otherwise?15:22
jelmer_noodles775: Originally I had it return the exception string and None when uploading was allowed.15:24
jelmer_noodles775: I guess it really depends on whether we would want to expose the reason why the upload would not be possible to the user15:25
jelmer_noodles775: there are different exception classes so in theory you could have different behaviour based on that.15:25
noodles775jelmer_: we should never be expecting a 500, afaik (if you do raise an exception, it needs to use webservice_error(xxx) as in the other examples in interfaces/archive.py)15:27
jelmer_noodles775: ok15:29
noodles775jelmer_: in fact, there's already an InsufficientUploadRights exception defined there - it might be what you're after.15:29
noodles775jelmer_: but I'm still unsure why you're not returning True/False.15:30
* noodles775 checks for other examples.15:30
jelmer_noodles775: With True/False the user doesn't get an idea why the upload wouldn't be allowed (user not in packageset, pocket is release and distroseries is closed, etc)15:30
jelmer_noodles775: The feature request doesn't require that though, so I'm happy to change it to a boolean.15:31
noodles775jelmer_: in which case, would either returning True, or raising a descriptive webservice_error be better?15:32
jelmer_noodles775: I would expect a descriptive webservice error to be better because it gives the user more information that they can use or ignore.15:34
noodles775jelmer_: great. So if they can upload, return True, otherwise raise a descriptive webservice error.15:34
jelmer_noodles775: ok15:35
=== jelmer_ changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
jtvhi jelmer!  Can I chuck one on your queue?15:39
jelmer_jtv: Hee Jeroen! Yeah, go ahead.15:39
jtvdankuwel :)15:39
=== jtv changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: sinzui || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
jelmer_noodles775: thanks for the review15:49
noodles775jelmer_: np!15:56
sinzuigary_poster, ping15:58
noodles775sinzui: I have to run, so assuming you're keen to land your cache branch that jelmer reviewed, can you request a review from the next OCR?15:58
gary_postersinzui: hi15:58
=== matsubara is now known as matsubara-lunch
sinzuigary_poster, unping, noodles775 just offered15:58
gary_posterok15:58
sinzuior I am confused15:58
noodles775sinzui: ? I was saying I *can't* review it now...15:58
noodles775:)15:58
sinzuigary_poster, can you review https://code.launchpad.net/~sinzui/launchpad/cache-tweaks-0/+merge/2616715:59
gary_postersinzui, will be happy to, but may or may not have IS call now.  I'll start looking, and will inform you if I'm dragged away16:00
sinzui^ gary_poster your tales insight would be appreciated since I had to enter macro territory to make cache times variable16:00
gary_postercool.  my tales insight is questionable, but will give it a whirl16:00
sinzuiI am somewhat concerned that I may be the leading tales/metal developer on the Launchpad team. I blame attrition.16:01
gary_posterLOL " Pre-implementation: no one, I really am guessing, really"16:08
stubhttps://code.edge.launchpad.net/~stub/launchpad/page-performance-report/+merge/26017 <- I need to land the extra logging at a minimum16:20
stubjtv: ^^16:23
jtvstub: it'll cost you a beer16:23
jtvOh, it's the one I looked at before16:24
stubYou only get Leo for this one ;)16:24
jtvstub: grr...  just make sure there's a bug for the missing tests.16:24
jtvLeo < Lao16:25
stubI can revert the changes for the report - it needs to be rewritten anyway16:25
jtvwhich are those?16:25
stubAlthough that means it will certainly be broken, rather than untested.16:25
stubpageperformancereport.py16:25
stubThe important bit to land is publication.py16:26
jtvSince this is ongoing work I'd rather see it land as-is, with a bug filed for missing tests.16:26
stubBug #58642116:28
mupBug #586421: page-performance-report.py is untested <Launchpad Foundations:New> <https://launchpad.net/bugs/586421>16:28
jtvstub: I've updated the MP16:29
jtvjelmer: I'm off before the military seals off the city.  Hope you find all well with my branch!16:35
jtvstub: still curfew tonight, right?16:35
stubthink so - midnight16:36
jtv:(16:36
jelmerjtv: Yeah, I think so16:37
jelmerjtv: Have safe and fun evening16:37
jelmer*a16:37
jtvThank you.  :-)16:37
=== matsubara-lunch is now known as matsubara
gary_postersinzui: approved, with a few thouhts17:01
gary_posterthoughts17:01
sinzuithank, I am sure thoughts are needed17:01
gary_poster:-)17:01
=== salgado is now known as salgado-lunch
=== jelmer_ changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: || queue: [jtv, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
jelmer_sinzui: sorry, I've had to put your branch back in the queue17:28
jelmer_Hopefully rockstar should be able to review it.17:28
sinzuithanks. I do not intend to land it for 10.05 so I was not concerned.17:28
jelmer_ah, great.17:29
rockstarsinzui, I probably will not be taking my OCR day today.  I have things that need to get into 10.0517:29
=== jelmer_ changed the topic of #launchpad-reviews to: On Call: || reviewing: || queue: [jtv, sinzui, jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
sinzuirockstar, understood. ping me if you think you need a pre-emptive rc17:29
jelmer_sinzui: I need one actually..17:30
jelmer_sinzui: Michael has asked me to fix some things in https://code.edge.launchpad.net/~jelmer/launchpad/can-upload-webcall/+merge/26141 that have now been adressed; it'd be nice if this change could make it into 10.05.17:31
=== gary_poster is now known as gary-lunch
jelmer_sinzui: Would you perhaps be able to re-review that branch?17:35
sinzuiokay17:35
sinzuijelmerI really do not understand why we have a public private method on the interface. It we expect other objects/callsites to know about it, it should not have a leading underscore.17:47
sinzuijelmer why do we *need* a leading underscore?17:48
jelmer_sinzui: there already is a checkUpload method that takes real objects and returns exceptions17:50
jelmer_sinzui: the _checkUpload method is a wrapper around that with semantics better suitable for the web service17:50
sinzuiSo this export also blocks the other method from being exported by accident17:52
jelmer_yeah17:55
sinzuijelmer r=me, rc=me17:57
=== salgado-lunch is now known as salgado
jelmer_sinzui: thanks!17:57
=== deryck is now known as deryck[lunch]
rockstarsinzui, when is PQM closing?18:21
sinzui4.5 hours18:21
rockstarsinzui, oh shit...18:21
rockstarsinzui, I'll probably need a few RCs.18:22
rockstarsinzui, why so early?18:22
sinzuiyou can ask for an RC *now* better ask early than wait 10 hours18:22
sinzuiUS and UK are off on monday18:22
rockstarsinzui, oh crap...  I forgot.18:22
rockstarsinzui, is it safe to assume you might be around on the weekend18:23
sinzuiI certainly will18:23
sinzuiI will send out an email about contacting me after I finish testing my RC: hack to send sms messages to my phone18:24
=== gary-lunch is now known as gary_poster
rockstarsinzui, can you look at this really quick. Just needs code review.18:30
sinzuisure18:31
=== deryck[lunch] is now known as deryck
sinzuirockstar, am I correct in seeing you have been dupped into landing bug data changes?19:21
sinzuirockstar you have my RC for the ec2 landing.19:30
sinzuirockstar, I agree with BjornT regarding the test in https://code.edge.launchpad.net/~rockstar/launchpad/no-duplicate-recipe-names/+merge/2608019:34
rockstarsinzui, do you have a suggestion?  BjornT is apparently not around.19:35
sinzuiI am going to look19:35
rockstarI couldn't see another way around that.19:35
rockstarsinzui, I'm happy to say it's a temporary test.19:47
rockstarsinzui, basically, the patch needs to get in.19:47
sinzuirockstar, we have 100's of UNIQUE constraints, and non are tested. drop the test19:49
sinzuiI landed a constraint a few months ago to force integrity error so that I could find bad code, but I did not test the unique constraint19:49
rockstarsinzui, okay.19:51
rockstarsinzui, test is gone.20:05
sinzuifaboo20:05
rockstarsinzui, were you going to do the code-review for https://code.edge.launchpad.net/~rockstar/launchpad/no-recipe-dupes/+merge/26212 ?20:06
sinzuirockstar, can you really do model work in the UI without making the import fascist angry20:11
rockstarsinzui, I didn't notice any errors.20:12
sinzuirockstar, this implementation is not safe for API.20:12
sinzuiI expect a set object to have a lookup method that API and browser can use20:12
rockstarsinzui, yes, I proposed a fix in the model, and stub disapproved it.20:14
sinzuiwhat was his reason?20:18
rockstarsinzui, because it was requiring a select query on every change.20:19
sinzuiThere are no uses of store.find under browser/ this implementation gives the view more knowledge of the schema than the model has20:19
rockstarsinzui, so, the plan is to get the db patch in, fix the UI, and then go about fixing the mutators for the API.20:19
rockstarsinzui, so, how about if I move the store.find to be a staticmethod of the SourcePackageRecipe model?20:20
rockstarAnd then I just call it from the browser code.20:20
rockstarsinzui, I figured I'd have to do that anyway.20:20
sinzuimove your store core to the Set or utility object. Views lookup duplicate instances all the time20:20
rockstarsinzui, okay.20:21
sinzuirockstar, I am think about recent integrity issues in Packaging. Packaging does not have a Set. It has a utility object that lets us check for existence and we had to use that in several places to avoid insane instances20:23
rockstarsinzui, okay.20:24
sinzuiISourcePackageRecipeSource.exists() ?20:24
rockstarsinzui, yeah, that's probably a better plan.20:27
* rockstar goes to work.20:27
rockstarsinzui, fixed: https://code.edge.launchpad.net/~rockstar/launchpad/no-recipe-dupes/+merge/2621221:08
* sinzui looks21:09
sinzuirockstar, this is what I expected :) I also expected a doc test to demonstrate how it is used. there is no doc test for recipes though21:12
sinzuirockstar, what do I read to learn how to use the api?21:12
rockstarsinzui, nothing.  We need to shift where our tests are, and make documentation.21:13
rockstarRight now, there are unit tests for the API, but not docs.21:13
sinzuiTestSourcePackageRecipe ?21:14
sinzuiI see the creation test there21:14
sinzuirockstar, I know you do not want to read this. I think we need a short test much like setup in the view test that directly verifies exists()21:15
rockstarsinzui, okay. I can do that in a unit test.  It's not a big deal.  I have to get an RC for it now anyway.21:16
sinzuiyes, and I will give it to you for all your troubles21:16
rockstarsinzui, did you see my other two branches with a rc request?21:19
sinzuiI think I gave them to you.21:20
* sinzui looks again21:20
rockstarsinzui, test added, pushing now.21:22
sinzuiI was negligent. I see I did not do as I said I would21:22
rockstarsinzui, I don't think negligent is the right word there.  :)  Busy, scatterbrained, harassed, not negligent.21:22
sinzuiIf I do a bad job here, no one will ask me to be the release manager again@21:23
sinzui!21:23
sinzuiI see my SMS forwarder needs some python 2.6 love21:25
sinzuirockstar, I think we are done. three rc and the reviews are approved21:28
rockstarsinzui, awesome, thanks.21:28
rockstarsinzui, is PQM closed now then?21:28
sinzuiIt closes in 90 minutes21:28
thumpersinzui: are you RM this time?21:28
sinzuiI am.21:29
sinzuiAs I said, If I cock this up, I will not be asked to do this again :)21:29
thumpersinzui: can I chat briefly about a couple of branche?22:11
sinzuiyes22:11
thumpersinzui: skype?22:11
sinzuiI am ready22:12
=== matsubara is now known as matsubara-afk
=== salgado is now known as salgado-afk

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!