[03:56] <stub> https://code.edge.launchpad.net/~stub/launchpad/read-only-launchpad/+merge/26033
[06:27] <rockstar> stub, can I get you to do a review for me?
[07:03] <stub> yo
[07:05] <rockstar> stub, https://code.edge.launchpad.net/~rockstar/launchpad/sourcepackagerecipe-name-contstraint/+merge/26122
[07:10] <stub> rockstar: Is using a propertly like that how we do this elsewhere?
[07:15] <stub> rockstar: Normally, we don't do this sort of check except in the UI code.
[07:15] <stub> rockstar: And the database exception gets raised if something is broken and lets through invalid data
[07:15] <rockstar> stub, well, I did the pre-imp with abentley, and that was his suggested method, which I liked.
[07:16] <stub> You 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:17] <stub> That is a lot of database queries to protect against input that should never arrive, and will be caught at the database layer.
[07:17] <stub> I can see the point for an emergency cherry pick to edge, but I don't like it for long term
[07:17] <rockstar> stub, so you're saying this should be implemented in the browser code only?
[07:17] <stub> Yes, like all our other unique inputs.
[07:18] <rockstar> stub, okay.  I'll look into that in the morning then.
[07:18] <rockstar> stub, thanks for the review.  Please disapprove it.
[08:16] <noodles775> Hi stub, I've just added a comment on your MP - I can't see anywhere where was_read_only is set initially?
[08:18] <noodles775> Also, 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:35] <stub> noodles775: 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 initialization
[08:36] <noodles775> stub: yes, I understood the getattr, but afaics, it is *never* set?
[08:36]  * noodles775 looks again
[08:37] <stub> It is set at the end of that if statement
[08:37] <stub>             self.thread_locals.was_read_only = is_read_only()
[08:37] <noodles775> stub: yes, but that if statement is only executed if was_read_only is not None?
[08:37] <stub> point.
[08:37]  * stub wonders how his tests work at all
[08:39] <noodles775> Ah, 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:40] <stub> There are tests. I'm fixing the last one of them now ;)
[08:40] <noodles775> Great :)
[09:01] <stub> noodles775: All pushed. There are other readonly mode tests too, but they are for bits I didn't touch
[09:02] <stub> (trying to keep this minimal since we don't have much time to test, and I'm lazy)
[09:02] <noodles775> stub: great, I'll take another look. Also, did you see my question about dogfood vs. production minutes above?
[09:05] <stub> Dogfood 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:06] <stub> We don't really have a metric for dogfood. Staging we tend to double the times (faster machines, but three replicas to apply to)
[09:07] <noodles775> stub: 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:08] <stub> I can get a timing... I'll kick it off now so we will know.
[09:08] <noodles775> Ah, great!
[09:09] <noodles775> stub: you've got some staging database you can trash afterwards have you? or are you just not committing the transaction?
[09:09] <stub> I'll skip the commit
[09:12] <stub> Might make staging whOOPS a bit, but whatever.
[09:16] <stub> noodles775: Is this landing this cycle?
[09:18] <noodles775> We 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:19] <noodles775> stub: it's expected that test_publication passed both with and without your changes right?
[09:22] <stub> Yes. I'm fixing a race condition that is common under load but that we have no tests for.
[09:22] <noodles775> OK, r=me
[10:01] <stub> noodles775: It took 17 minutes on staging, so maybe 35-40 mins to apply to production.
[10:01] <noodles775> stub: thanks. I assume that's way too long? What is acceptable?
[10:02] <stub> Thats up to the release manager and losas
[10:02] <noodles775> OK.
[10:10] <stub> noodles775: This version should be identical and takes 8 minutes, so 16-20 mins on production: http://paste.ubuntu.com/440285/
[10:11] <wgrant> noodles775: 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] <noodles775> Thanks 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] <noodles775> wgrant: sure.
[10:12] <wgrant> noodles775: Thanks.
[10:39] <wgrant> win 34
[10:39] <wgrant> Argh.
[10:39]  * noodles775 is glad it's not just him who does that ;)
[11:51] <bigjools> why don't you leave the 80s behind and get a graphical IRC client ;)
[11:51] <noodles775> heh, you use vim too right? ;)
[11:56] <maxb> bigjools: Find me a graphical irc client I can run in screen :-)
[11:56] <maxb> Actually, I do run a graphical IRC client, I just connect it to my irssi in proxy mode :-)
[12:00] <jelmer_> hi deryck
[12:01] <deryck> Hi jelmer
[12:04] <deryck> jelmer, thanks for the review!
[12:05] <deryck> jelmer, 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:23] <bigjools> ah, the right tool for the job I say :)
[12:38] <deryck> bigjools, is that in reply to my question?
[12:38] <bigjools> deryck: ah no, I was replying to others before you
[12:39] <bigjools> deryck: but FWIW flush() is always preferred to commit() if it works!
[12:39] <bigjools> commit() makes tests *really* slow
[12:43] <wgrant> Unless you're like bigjools and have a ridiculous SSD?
[12:43] <bigjools> s/ridiculous/absolutely gorgeous/
[12:43] <deryck> bigjools, 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] <bigjools> correct
[12:43] <bigjools> well, flushes from the cache
[12:44] <deryck> gotcha.  I think I need commit in this case then, but will test and confirm.
[12:44] <wgrant> flush() causes Storm to execute pending SQL.
[12:44] <wgrant> You should rarely need to do that manually.
[12:45] <deryck> Right
[12:45] <wgrant> It probably (well, hopefully) won't hit disk.
[12:46] <deryck> ah, right.  I see the distinction, thanks.  My mind is a bit fuzzy today.
[13:41] <StevenK> jelmer: I've pushed a fix for the -config issue
[13:47] <jelmer_> StevenK: cool - I've already approved that one, just waiting for Michael to review
[13:47]  * jelmer_ wished Launchpad allowed Tweak votes like BundleBuggy used to
[13:47] <noodles775> StevenK, jelmer_ : I just approved - with a note about lp_production_configs.
[13:51] <wgrant> noodles775: Thanks -- both landed.
[13:52] <noodles775> wgrant: np.
[14:49] <jelmer_> hi Curtis
[14:49] <jelmer_> thanks for the non-Soyuz branches :-)
[15:00] <noodles775> sinzui: when is PQM closing? I remember seeing an announcement, but can't find the email.
[15:00] <sinzui> noodles775, I am confirming that now for an email. I asked for 22:00 UTC
[15:00] <noodles775> sinzui: on which day?
[15:00] <sinzui> today
[15:03] <noodles775> sinzui: 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:04] <sinzui> ouch. yes I am
[15:04] <noodles775> Thanks.
[15:11] <sinzui> noodles775, 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 variability
[15:12] <noodles775> Thanks sinzui.
[15:13] <StevenK> sinzui: I have a branch that just hit ec2
[15:14] <StevenK> But I so don't want to stay up and babysit it
[15:14] <sinzui> okay, I think that will be fine
[15:15] <noodles775> jelmer_: 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:16] <sinzui> noodles775, I think that means you want to rename the method to be public now
[15:16] <sinzui> noodles775, I will refrain from suggesting a uses the rename hack since exposing a method explicitly makes it public
[15:17] <jelmer_> noodles775: Maybe, I haven't actually checked...
[15:17] <jelmer_> noodles775: I'll have a look now.
[15:18] <noodles775> sinzui: 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:22] <noodles775> jelmer_: 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:24] <jelmer_> noodles775: Originally I had it return the exception string and None when uploading was allowed.
[15:25] <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 user
[15:25] <jelmer_> noodles775: there are different exception classes so in theory you could have different behaviour based on that.
[15:27] <noodles775> jelmer_: 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:29] <jelmer_> noodles775: ok
[15:29] <noodles775> jelmer_: in fact, there's already an InsufficientUploadRights exception defined there - it might be what you're after.
[15:30] <noodles775> jelmer_: 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:31] <jelmer_> noodles775: The feature request doesn't require that though, so I'm happy to change it to a boolean.
[15:32] <noodles775> jelmer_: in which case, would either returning True, or raising a descriptive webservice_error be better?
[15:34] <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] <noodles775> jelmer_: great. So if they can upload, return True, otherwise raise a descriptive webservice error.
[15:35] <jelmer_> noodles775: ok
[15:39] <jtv> hi jelmer!  Can I chuck one on your queue?
[15:39] <jelmer_> jtv: Hee Jeroen! Yeah, go ahead.
[15:39] <jtv> dankuwel :)
[15:49] <jelmer_> noodles775: thanks for the review
[15:56] <noodles775> jelmer_: np!
[15:58] <sinzui> gary_poster, ping
[15:58] <noodles775> sinzui: 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_poster> sinzui: hi
[15:58] <sinzui> gary_poster, unping, noodles775 just offered
[15:58] <gary_poster> ok
[15:58] <sinzui> or I am confused
[15:58] <noodles775> sinzui: ? I was saying I *can't* review it now...
[15:58] <noodles775> :)
[15:59] <sinzui> gary_poster, can you review https://code.launchpad.net/~sinzui/launchpad/cache-tweaks-0/+merge/26167
[16:00] <gary_poster> sinzui, 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 away
[16:00] <sinzui> ^ gary_poster your tales insight would be appreciated since I had to enter macro territory to make cache times variable
[16:00] <gary_poster> cool.  my tales insight is questionable, but will give it a whirl
[16:01] <sinzui> I am somewhat concerned that I may be the leading tales/metal developer on the Launchpad team. I blame attrition.
[16:08] <gary_poster> LOL " Pre-implementation: no one, I really am guessing, really"
[16:20] <stub> https://code.edge.launchpad.net/~stub/launchpad/page-performance-report/+merge/26017 <- I need to land the extra logging at a minimum
[16:23] <stub> jtv: ^^
[16:23] <jtv> stub: it'll cost you a beer
[16:24] <jtv> Oh, it's the one I looked at before
[16:24] <stub> You only get Leo for this one ;)
[16:24] <jtv> stub: grr...  just make sure there's a bug for the missing tests.
[16:25] <jtv> Leo < Lao
[16:25] <stub> I can revert the changes for the report - it needs to be rewritten anyway
[16:25] <jtv> which are those?
[16:25] <stub> Although that means it will certainly be broken, rather than untested.
[16:25] <stub> pageperformancereport.py
[16:26] <stub> The important bit to land is publication.py
[16:26] <jtv> Since this is ongoing work I'd rather see it land as-is, with a bug filed for missing tests.
[16:28] <stub> Bug #586421
[16:28] <mup> Bug #586421: page-performance-report.py is untested <Launchpad Foundations:New> <https://launchpad.net/bugs/586421>
[16:29] <jtv> stub: I've updated the MP
[16:35] <jtv> jelmer: I'm off before the military seals off the city.  Hope you find all well with my branch!
[16:35] <jtv> stub: still curfew tonight, right?
[16:36] <stub> think so - midnight
[16:36] <jtv> :(
[16:37] <jelmer> jtv: Yeah, I think so
[16:37] <jelmer> jtv: Have safe and fun evening
[16:37] <jelmer> *a
[16:37] <jtv> Thank you.  :-)
[17:01] <gary_poster> sinzui: approved, with a few thouhts
[17:01] <gary_poster> thoughts
[17:01] <sinzui> thank, I am sure thoughts are needed
[17:01] <gary_poster> :-)
[17:28] <jelmer_> sinzui: sorry, I've had to put your branch back in the queue
[17:28] <jelmer_> Hopefully rockstar should be able to review it.
[17:28] <sinzui> thanks. I do not intend to land it for 10.05 so I was not concerned.
[17:29] <jelmer_> ah, great.
[17:29] <rockstar> sinzui, I probably will not be taking my OCR day today.  I have things that need to get into 10.05
[17:29] <sinzui> rockstar, understood. ping me if you think you need a pre-emptive rc
[17:30] <jelmer_> sinzui: I need one actually..
[17:31] <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:35] <jelmer_> sinzui: Would you perhaps be able to re-review that branch?
[17:35] <sinzui> okay
[17:47] <sinzui> jelmerI 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:48] <sinzui> jelmer why do we *need* a leading underscore?
[17:50] <jelmer_> sinzui: there already is a checkUpload method that takes real objects and returns exceptions
[17:50] <jelmer_> sinzui: the _checkUpload method is a wrapper around that with semantics better suitable for the web service
[17:52] <sinzui> So this export also blocks the other method from being exported by accident
[17:55] <jelmer_> yeah
[17:57] <sinzui> jelmer r=me, rc=me
[17:57] <jelmer_> sinzui: thanks!
[18:21] <rockstar> sinzui, when is PQM closing?
[18:21] <sinzui> 4.5 hours
[18:21] <rockstar> sinzui, oh shit...
[18:22] <rockstar> sinzui, I'll probably need a few RCs.
[18:22] <rockstar> sinzui, why so early?
[18:22] <sinzui> you can ask for an RC *now* better ask early than wait 10 hours
[18:22] <sinzui> US and UK are off on monday
[18:22] <rockstar> sinzui, oh crap...  I forgot.
[18:23] <rockstar> sinzui, is it safe to assume you might be around on the weekend
[18:23] <sinzui> I certainly will
[18:24] <sinzui> I will send out an email about contacting me after I finish testing my RC: hack to send sms messages to my phone
[18:30] <rockstar> sinzui, can you look at this really quick. Just needs code review.
[18:31] <sinzui> sure
[19:21] <sinzui> rockstar, am I correct in seeing you have been dupped into landing bug data changes?
[19:30] <sinzui> rockstar you have my RC for the ec2 landing.
[19:34] <sinzui> rockstar, I agree with BjornT regarding the test in https://code.edge.launchpad.net/~rockstar/launchpad/no-duplicate-recipe-names/+merge/26080
[19:35] <rockstar> sinzui, do you have a suggestion?  BjornT is apparently not around.
[19:35] <sinzui> I am going to look
[19:35] <rockstar> I couldn't see another way around that.
[19:47] <rockstar> sinzui, I'm happy to say it's a temporary test.
[19:47] <rockstar> sinzui, basically, the patch needs to get in.
[19:49] <sinzui> rockstar, we have 100's of UNIQUE constraints, and non are tested. drop the test
[19:49] <sinzui> I 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 constraint
[19:51] <rockstar> sinzui, okay.
[20:05] <rockstar> sinzui, test is gone.
[20:05] <sinzui> faboo
[20:06] <rockstar> sinzui, were you going to do the code-review for https://code.edge.launchpad.net/~rockstar/launchpad/no-recipe-dupes/+merge/26212 ?
[20:11] <sinzui> rockstar, can you really do model work in the UI without making the import fascist angry
[20:12] <rockstar> sinzui, I didn't notice any errors.
[20:12] <sinzui> rockstar, this implementation is not safe for API.
[20:12] <sinzui> I expect a set object to have a lookup method that API and browser can use
[20:14] <rockstar> sinzui, yes, I proposed a fix in the model, and stub disapproved it.
[20:18] <sinzui> what was his reason?
[20:19] <rockstar> sinzui, because it was requiring a select query on every change.
[20:19] <sinzui> There are no uses of store.find under browser/ this implementation gives the view more knowledge of the schema than the model has
[20:19] <rockstar> sinzui, so, the plan is to get the db patch in, fix the UI, and then go about fixing the mutators for the API.
[20:20] <rockstar> sinzui, so, how about if I move the store.find to be a staticmethod of the SourcePackageRecipe model?
[20:20] <rockstar> And then I just call it from the browser code.
[20:20] <rockstar> sinzui, I figured I'd have to do that anyway.
[20:20] <sinzui> move your store core to the Set or utility object. Views lookup duplicate instances all the time
[20:21] <rockstar> sinzui, okay.
[20:23] <sinzui> rockstar, 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 instances
[20:24] <rockstar> sinzui, okay.
[20:24] <sinzui> ISourcePackageRecipeSource.exists() ?
[20:27] <rockstar> sinzui, yeah, that's probably a better plan.
[20:27]  * rockstar goes to work.
[21:08] <rockstar> sinzui, fixed: https://code.edge.launchpad.net/~rockstar/launchpad/no-recipe-dupes/+merge/26212
[21:09]  * sinzui looks
[21:12] <sinzui> rockstar, this is what I expected :) I also expected a doc test to demonstrate how it is used. there is no doc test for recipes though
[21:12] <sinzui> rockstar, what do I read to learn how to use the api?
[21:13] <rockstar> sinzui, nothing.  We need to shift where our tests are, and make documentation.
[21:13] <rockstar> Right now, there are unit tests for the API, but not docs.
[21:14] <sinzui> TestSourcePackageRecipe ?
[21:14] <sinzui> I see the creation test there
[21:15] <sinzui> rockstar, 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:16] <rockstar> sinzui, 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] <sinzui> yes, and I will give it to you for all your troubles
[21:19] <rockstar> sinzui, did you see my other two branches with a rc request?
[21:20] <sinzui> I think I gave them to you.
[21:20]  * sinzui looks again
[21:22] <rockstar> sinzui, test added, pushing now.
[21:22] <sinzui> I was negligent. I see I did not do as I said I would
[21:22] <rockstar> sinzui, I don't think negligent is the right word there.  :)  Busy, scatterbrained, harassed, not negligent.
[21:23] <sinzui> If I do a bad job here, no one will ask me to be the release manager again@
[21:23] <sinzui> !
[21:25] <sinzui> I see my SMS forwarder needs some python 2.6 love
[21:28] <sinzui> rockstar, I think we are done. three rc and the reviews are approved
[21:28] <rockstar> sinzui, awesome, thanks.
[21:28] <rockstar> sinzui, is PQM closed now then?
[21:28] <sinzui> It closes in 90 minutes
[21:28] <thumper> sinzui: are you RM this time?
[21:29] <sinzui> I am.
[21:29] <sinzui> As I said, If I cock this up, I will not be asked to do this again :)
[22:11] <thumper> sinzui: can I chat briefly about a couple of branche?
[22:11] <sinzui> yes
[22:11] <thumper> sinzui: skype?
[22:12] <sinzui> I am ready