| davecheney | niemeyer: new revision of SetConfig incoming | 00:34 |
|---|---|---|
| davecheney | you may want to see more caching | 00:34 |
| davecheney | niemeyer: ready for review, https://codereview.appspot.com/6229046 | 00:41 |
| niemeyer | davecheney: Review on first one delivered | 01:16 |
| niemeyer | davecheney: "Patch set 8 incorporates https://codereview.appspot.com/6244053." | 01:17 |
| niemeyer | davecheney: I guess it shouldn't? Otherwise what's the point of having the other CL? | 01:17 |
| davecheney | niemeyer: that was a cryptic way of saying i rolled that change up into setconfig | 01:17 |
| davecheney | but given your comments on the makeBucket branhc | 01:17 |
| davecheney | that will turn out to be a bigger piece of work | 01:18 |
| niemeyer | davecheney: The comment remains.. what's the point of having a second CL if it's included in another one | 01:18 |
| davecheney | i'm finding it hard to manage branches that have multiple parents | 01:18 |
| davecheney | in the future I won't merge the like patch #8 | 01:19 |
| davecheney | i'll wait til the upstream passes review | 01:19 |
| niemeyer | davecheney: Ideally things are either independent, or you use -pre-req | 01:19 |
| niemeyer | davecheney: Yeah, that's a good plan in general | 01:19 |
| davecheney | so, talking about https://codereview.appspot.com/6244053 for a second | 01:19 |
| davecheney | you belive this is a bug introduced recently | 01:20 |
| davecheney | ? | 01:20 |
| niemeyer | Ok | 01:20 |
| niemeyer | davecheney: Yeah, definitely.. this logic was worked on by rog last week or so | 01:20 |
| davecheney | ah, ok | 01:20 |
| davecheney | i'll review the commit history and try a different approach | 01:20 |
| davecheney | i'll put SetConfig back into WIP | 01:21 |
| niemeyer | davecheney: I'm having a look at it just now | 01:21 |
| davecheney | if you could give it a cursory view that would be good | 01:21 |
| niemeyer | davecheney: I'm a bit unhappy with the strategy that is spreading through there | 01:22 |
| niemeyer | return &storage{bucket: e.s3().Bucket(config.bucket)} | 01:22 |
| niemeyer | davecheney: This is creating a storage+S3+bucket every single time we need to use the bucket | 01:22 |
| niemeyer | davecheney: Imagine what happens in a trivial loop like this: | 01:23 |
| niemeyer | for { | 01:23 |
| niemeyer | ... whatever ... | 01:23 |
| davecheney | yup, understood | 01:23 |
| niemeyer | environ.PutFile(file) | 01:23 |
| niemeyer | davecheney: *tons* of garbage | 01:23 |
| davecheney | I am not overly concerned with performance at this stage | 01:23 |
| davecheney | as this is talking to a network service | 01:24 |
| davecheney | but I will add caching | 01:24 |
| niemeyer | davecheney: For no great reason.. we got into this situation after trying to avoid the caching, which wasn't a big deal at all | 01:24 |
| davecheney | understood | 01:24 |
| niemeyer | davecheney: I'm not *concerned* either | 01:24 |
| niemeyer | davecheney: I just feel bad that we're giving up on both simplicity *and* performance at the same time | 01:25 |
| davecheney | i'm trying for something that is correct with respect to SetConfig | 01:25 |
| davecheney | i hope that caching could be added later | 01:25 |
| davecheney | going back to makeBucket for a second, 'cos that is now a blocker | 01:25 |
| davecheney | I can't see there the bug was introduced | 01:25 |
| davecheney | not in the last week at least | 01:25 |
| niemeyer | davecheney: THe makeBucket issue was created while avoiding the caching as well | 01:26 |
| niemeyer | davecheney: Well, some of the issues, maybe not makeBucket | 01:26 |
| niemeyer | davecheney: The race from the last review, for example | 01:26 |
| davecheney | niemeyer: i'm concerned that making the environs reloadable is becoming a blocker | 01:28 |
| davecheney | I understand it is important for the final product | 01:29 |
| davecheney | but would it be possible to propose a provisioning agent that doesn't reload it's configuration after starting | 01:29 |
| niemeyer | davecheney: No, we have spent a lot of time on this, let's not throw all the work and discussion now that it's pretty much done | 01:30 |
| niemeyer | davecheney: Hindsight is good, but let's not ignore what was done now that we have it | 01:30 |
| davecheney | i am concerned that the adding reloadability to environs is a moving target | 01:31 |
| davecheney | i'm wondering if it would be better to address then after we have a PA that we can use to bring up a trial juju on ec2 | 01:32 |
| niemeyer | davecheney: It wasn't a moving target.. I suggested not changing caching, for example, because it worked, and then you found it a better approach.. that broke things further.. and that had to be fixed.. and ... | 01:32 |
| davecheney | exactly | 01:32 |
| davecheney | so lets put all that to once side | 01:32 |
| davecheney | one side | 01:32 |
| niemeyer | davecheney: Yeah, let's *fix the problems* and not change further | 01:32 |
| niemeyer | davecheney: If you're feeling overwhelmed, I can do that myself | 01:32 |
| davecheney | niemeyer: not overwhelmed, just concerned that all the changes that the PA needs to environs/Environ keeps pushing back the PA itself | 01:33 |
| niemeyer | davecheney: Yeah, so ignore the madeBucket stuff, if that's what you mean | 01:33 |
| niemeyer | davecheney: Just drop it, and we'll put on someone else's todo list | 01:33 |
| niemeyer | davecheney: That sounds fine | 01:34 |
| davecheney | i'm suggesting a middle ground of delivering a PA that doesnt' try to respond to changes in /environment, so has no requirement for Environ.SetConfig | 01:34 |
| niemeyer | davecheney: But I do want to see caching added back, and all that extra stuff removed | 01:34 |
| niemeyer | davecheney: Because that's part of the "never ending roll" | 01:34 |
| davecheney | niemeyer: yes, i want to get out of this spiral | 01:35 |
| davecheney | and I can do that if the PA can skip having to reload it's environ once it's up and running | 01:35 |
| davecheney | then when the rest of the PA is working | 01:35 |
| niemeyer | davecheney: Again, you spent days on this, and I spend hours reviewing it.. I'm not happy with trashing both your and my time | 01:35 |
| davecheney | reloading environ configs can be tackled as seperate branch | 01:35 |
| davecheney | i'm not saying throw it awwy | 01:35 |
| davecheney | just put it on hold for a while | 01:35 |
| niemeyer | davecheney: That doesn't work.. if it stays out, it will be broken next week | 01:36 |
| niemeyer | davecheney: Let's fix it, and move on | 01:36 |
| davecheney | ok, understood | 01:36 |
| niemeyer | davecheney: Happy to drop make/madeBucket, though | 01:37 |
| niemeyer | davecheney: Didn't happen in this branch.. | 01:37 |
| davecheney | ok, so to review, I will continue to work on SetConfig. I will restore all the cached values like ec3Unlocked and friends | 01:41 |
| davecheney | and makes changes to environ.config atomic with changes to {public,}storage.bucket | 01:41 |
| davecheney | /s/ec3/ec2/ | 01:42 |
| niemeyer | davecheney: Heh, madeBucket was born broken | 01:42 |
| davecheney | niemeyer: thought so | 01:42 |
| niemeyer | davecheney: Sorry about that | 01:42 |
| niemeyer | davecheney: I suspect the second problem is a NOOP once you deal with the first | 01:43 |
| niemeyer | davecheney: You got into the second issue because replacing the bucket was done with s3(), which needed a lock | 01:43 |
| niemeyer | davecheney: Once you have the cached values, it's all done within the lock | 01:43 |
| davecheney | yup, that'll work | 01:43 |
| davecheney | seriously, i should stop listening to roger's advice | 01:44 |
| davecheney | that was his suggestion :) | 01:44 |
| davecheney | anwywya, i have to go | 01:44 |
| niemeyer | LOL | 01:44 |
| niemeyer | Rog have some great advices, though | 01:44 |
| niemeyer | ''''''''''''''''has | 01:44 |
| davecheney | i know, it's hard to sift one from the other | 01:44 |
| davecheney | anyway, i have to go | 01:44 |
| niemeyer | davecheney: Cheers | 01:44 |
| davecheney | will submit another round tonihgt | 01:45 |
| davecheney | thank you for your time | 01:45 |
| niemeyer | davecheney: My pleasure | 01:48 |
| andrewsmedina | niemeyer: are you working 24/7 ? | 01:50 |
| andrewsmedina | :-p | 01:50 |
| niemeyer | andrewsmedina: Nah.. :-) | 01:51 |
| andrewsmedina | :D | 01:52 |
| andrewsmedina | niemeyer: I submitted a new proposal | 01:52 |
| niemeyer | andrewsmedina: Thanks! | 01:52 |
| niemeyer | andrewsmedina: This was probably the last one.. with those minors, it should be good to go in | 01:52 |
| andrewsmedina | niemeyer: thanks :D | 01:54 |
| andrewsmedina | niemeyer: Rodrigo Senra will be here tomorrow | 01:54 |
| niemeyer | andrewsmedina: Wow, really? | 02:30 |
| niemeyer | andrewsmedina: This is awesome.. small world | 02:31 |
| niemeyer | andrewsmedina: Please deliver him a hug from me :) | 02:31 |
| andrewsmedina | niemeyer: ok, I will | 02:32 |
| niemeyer | andrewsmedina: aaaaand your branhc is going in | 02:32 |
| andrewsmedina | niemeyer: nice! | 02:32 |
| andrewsmedina | niemeyer: thanks | 02:32 |
| niemeyer | andrewsmedina: Thank *you* for keeping up with it | 02:32 |
| andrewsmedina | niemeyer: now I will work on local provider environ interface =D | 02:33 |
| niemeyer | andrewsmedina: Sweeeeet | 02:33 |
| niemeyer | andrewsmedina: Small increments is the key | 02:34 |
| niemeyer | andrewsmedina: Don't feel bad to push half baked stuff | 02:34 |
| niemeyer | andrewsmedina: As long as it's a correct step forward, it's fine to go in | 02:34 |
| niemeyer | andrewsmedina: E.g. a type that implements all interface methods returning errors in all of them is a correct step forward | 02:34 |
| niemeyer | andrewsmedina: the implementation of a single one of these methods is a correct step forward to | 02:35 |
| niemeyer | too | 02:35 |
| niemeyer | error: Failed to load data for branch bzr+ssh://bazaar.launchpad.net/~andrewsmedina/juju/go-local-network/: Get https://api.launchpad.net/devel/branches?ws.op=getByUrl&url=bzr%2Bssh%3A%2F%2Fbazaar.launchpad.net%2F~andrewsmedina%2Fjuju%2Fgo-local-network%2F: dial tcp 91.189.89.224:443: connection timed out | 02:35 |
| niemeyer | error: Failed to load data for bran | 02:35 |
| niemeyer | Ah, dave isn't here | 02:35 |
| andrewsmedina | niemeyer: Why this error?? | 02:36 |
| niemeyer | andrewsmedina: NEvermind.. I was going to point to Dave that it wasn't just on him that it was blowing up :) | 02:36 |
| andrewsmedina | niemeyer: I will sent small proposes | 02:36 |
| niemeyer | andrewsmedina: Cheers! | 02:37 |
| niemeyer | andrewsmedina: It's in! | 02:44 |
| niemeyer | and the queue is zeroed! | 02:45 |
| niemeyer | Ah, the feeling of mission accomplished.. well, at least for a few hours. | 02:45 |
| niemeyer | Will take the chance to have a nice sleep | 02:45 |
| niemeyer | Night all! | 02:45 |
| andrewsmedina | niemeyer: good night! | 02:45 |
| davecheney | rogpeppe: whoop whoop! | 09:05 |
| rogpeppe | davecheney: haroo! | 09:06 |
| fwereade | davecheney, rogpeppe: heyhey | 09:08 |
| rogpeppe | fwereade: yo! | 09:08 |
| fwereade | davecheney, btw, what awful time of day is it for you? | 09:08 |
| davecheney | fwereade: 7pm | 09:09 |
| davecheney | rogpeppe: this is what i'm thinking http://paste.ubuntu.com/1006126/ | 09:09 |
| fwereade | davecheney, ok, not so awful then :) | 09:09 |
| rogpeppe | fwereade: you might want to fix this bug BTW: cmd/jujuc/server/util_test.go:37: not enough arguments in call to st.AddCharm | 09:09 |
| davecheney | not aweful at all | 09:09 |
| fwereade | rogpeppe, oof, thanks | 09:09 |
| davecheney | s/awe/aw/g | 09:10 |
| rogpeppe | davecheney: that won't work, sadly | 09:10 |
| davecheney | poop, why not | 09:10 |
| rogpeppe | davecheney: look at the implementation of deleteAll | 09:10 |
| * davecheney looks | 09:10 | |
| rogpeppe | davecheney: it wouldn't be good to have the bucket changing underfoot during the execution of that method | 09:10 |
| rogpeppe | davecheney: hence my "immutable" suggestion. | 09:11 |
| rogpeppe | davecheney: i thought that's what you meant when you said it was "a bit harder" BTW | 09:12 |
| davecheney | rogpeppe: good point, but consider why the bucket would be changing | 09:12 |
| rogpeppe | davecheney: if we change the bucket, does it make sense to have an ongoing delete method suddenly start to remove objects that were listed in one bucket from another bucket? | 09:12 |
| * davecheney thinks | 09:13 | |
| rogpeppe | davecheney: i think that an "immutable" method gives a nice understandable semantic - if you've started an operation with one set of settings, it will complete with the same. | 09:13 |
| davecheney | rogpeppe: if only it were that easy, for environ.SetConfig doesn't create a new storage, but alters it's bucket value | 09:14 |
| davecheney | however, that can be fixed | 09:15 |
| rogpeppe | davecheney: that's fine, i hadn't intended that it did | 09:15 |
| fwereade | rogpeppe, trivial: http://paste.ubuntu.com/1006133/ | 09:15 |
| rogpeppe | davecheney: did you look at my immutable method? | 09:15 |
| davecheney | yup, you look to be taking another reference to the *storage | 09:15 |
| rogpeppe | fwereade: oh cool, i thought maybe it needed a real sha hash | 09:15 |
| fwereade | rogpeppe, it may do at some stage | 09:16 |
| rogpeppe | davecheney: no, i'm copying the contents of the storage. | 09:16 |
| fwereade | rogpeppe, but we're not at that stage yet :) | 09:16 |
| rogpeppe | davecheney: well, the contents of the storage struct anyway | 09:16 |
| * davecheney reads again | 09:16 | |
| davecheney | 7~oooooooooh | 09:17 |
| davecheney | wow, i never considered that | 09:17 |
| davecheney | mainly 'cos i've never used a lanugage that could do that | 09:17 |
| rogpeppe | davecheney: it does mean we've got another allocation each time, but i think that's irrelevant, as mentioned earlier | 09:18 |
| davecheney | i agree about allocations | 09:18 |
| davecheney | i don't think allocation counting is important when we're talking to a slow arse ec2 provisioning service | 09:18 |
| rogpeppe | davecheney: and actually i think probably the compiler can tell that the value never escapes, so it might not even do one. | 09:19 |
| rogpeppe | davecheney: absolutely | 09:19 |
| rogpeppe | davecheney: particular when PutReader allocates 1700 times! | 09:19 |
| rogpeppe | s/ar/arly | 09:19 |
| davecheney | yeah, but gustavo had a point that I was touching too much code | 09:21 |
| davecheney | rogpeppe: what if the exported Storage/PublicStorage methods always returned an immutable *storage ? | 09:22 |
| davecheney | then DeleteAll would just work | 09:22 |
| fwereade | rogpeppe, btw, can I just submit that directly please? | 09:24 |
| davecheney | hmm, that might need a little more finesse, deleteAll isn't part of the public Storage API | 09:24 |
| rogpeppe | fwereade: i think you should make a CL, i'll LGTM it and you submit it immediately. that way the commit log looks consistent. | 09:24 |
| fwereade | rogpeppe, SGTM | 09:25 |
| rogpeppe | davecheney: i don't think that would work | 09:25 |
| davecheney | rogpeppe: would you say that deleteAll is the exception that breaks the rule | 09:25 |
| rogpeppe | davecheney: because there's nothing stopping someone holding on to a Storage object, but i guess we want it to change when the settings change. | 09:25 |
| rogpeppe | davecheney: ... or maybe not. i wonder. | 09:25 |
| davecheney | rogpeppe: there are arguments for an against | 09:26 |
| fwereade | rogpeppe, https://codereview.appspot.com/6243054 | 09:26 |
| rogpeppe | fwereade: you've checked that all tests pass? | 09:27 |
| fwereade | rogpeppe, heh, I'll do a full run through | 09:27 |
| rogpeppe | fwereade: i think it's worth it :-) | 09:27 |
| davecheney | rogpeppe: which do you think gustavo had in mind ? | 09:27 |
| fwereade | rogpeppe, quite so :) | 09:27 |
| rogpeppe | davecheney: i dunno. as you say, arguments for and against. | 09:28 |
| davecheney | if we call Environ.SetConfig, should that apply to any Storage that was requested, or should the Storage you previously asked for remain valid ? | 09:28 |
| davecheney | given that the reason for calling SetConfig is most likely to update some credentials | 09:28 |
| rogpeppe | davecheney: exactly. that's the question | 09:28 |
| davecheney | it's likely that your previous Storage would be inoperable | 09:28 |
| fwereade | rogpeppe, huh, strange things are going on elsewhere; I'll poke around and see if I can figure out what's up | 09:29 |
| davecheney | actually, SetConfig can _only_ change credentials, or the bucket | 09:29 |
| davecheney | as those are the only configuration values consumed | 09:29 |
| rogpeppe | davecheney: if we're changing credentials, to avoid screwing things up, we'll have to change the credentials *before* deprecating the old credentials | 09:31 |
| davecheney | rogpeppe: how about this, SetConfig never changes the value of storage.bucket, that is immutable once created | 09:31 |
| rogpeppe | davecheney: that won't work if we want a Storage to work long term | 09:32 |
| davecheney | it simply replaces (and safely publishes) new storage and publicStorage references | 09:32 |
| davecheney | right, so then the opposite is the better chioce | 09:32 |
| davecheney | the mutex guards storage.bucket, and changes to that are visible immediately | 09:32 |
| rogpeppe | davecheney: i think the crux is deleteAll - do we treat that as an "external" method, so the bucket can change underfoot as it's progressing? | 09:37 |
| rogpeppe | davecheney: what i'm concerned about is someone changing not just the security credentials (but leaving them pointing to the same original AWS account) but the name of the bucket too. | 09:38 |
| rogpeppe | s/(but /c/(/ | 09:38 |
| davecheney | so, what if I implement 'immutable' inside deleteAll, as it is the special case | 09:39 |
| davecheney | or at least use it in just the deleteAll case | 09:39 |
| rogpeppe | davecheney: tbh i think we might be better for all concerned if we make storage not change with SetConfig | 09:39 |
| davecheney | rogpeppe: i would be more than happy with that | 09:40 |
| rogpeppe | davecheney: and we can document that SetConfig works only on the Environ and not on storage objects returned from it | 09:40 |
| davecheney | what is your feeling about gustavo's view on this ? | 09:40 |
| rogpeppe | davecheney: then anything that calls Storage gets a nice coherent view | 09:40 |
| rogpeppe | davecheney: i *think* he'd be ok with that. | 09:40 |
| davecheney | i've seen you raise the point a few times | 09:40 |
| rogpeppe | davecheney: i have mixed feelings - there are trade-offs all over | 09:41 |
| TheMue | morning | 09:41 |
| rogpeppe | davecheney: but my current feeling is that this is the sweet spot | 09:41 |
| davecheney | hey | 09:41 |
| rogpeppe | TheMue: hiya! | 09:41 |
| davecheney | rogpeppe: i'm happy to float that | 09:41 |
| davecheney | rogpeppe: in your extimation, who are these long term holders of Storage references ? | 09:41 |
| rogpeppe | davecheney: i don't think there are any | 09:42 |
| rogpeppe | davecheney: that's the reason for my current thoughts | 09:42 |
| davecheney | rogpeppe: then why suggest that storages' not change with SetCOnfig ? | 09:45 |
| rogpeppe | davecheney: because even though there are no long term holders of Storage references, there will be *concurrent* holders of Storage references | 09:46 |
| rogpeppe | davecheney: so we can provide them with a coherent view | 09:46 |
| rogpeppe | davecheney: by letting each call to Storage return a new *storage instance. | 09:46 |
| davecheney | rogpeppe: yup, i'd support that | 09:47 |
| rogpeppe | davecheney: in fact it's easier than that | 09:47 |
| rogpeppe | davecheney: we just alloc a new storage on each SetConfig | 09:47 |
| rogpeppe | davecheney: and return that from Storage | 09:48 |
| rogpeppe | davecheney: easy peasy | 09:48 |
| rogpeppe | davecheney: (changing the type of the storage field in environ to be a pointer, of course) | 09:48 |
| rogpeppe | davecheney: then the current storage implementation stays identical to current. | 09:49 |
| davecheney | let rogpeppe it's slightly less easy, 'cos we have to publish that reference properly | 09:49 |
| rogpeppe | doh, s/identical to current/the same/ | 09:49 |
| rogpeppe | davecheney: that reference is inside the environ instance, no? | 09:49 |
| davecheney | i'd vote for constructing a new storage for each call to environ.Storage() using the current values of config() | 09:49 |
| rogpeppe | davecheney: i don't think there's a need to publish it | 09:50 |
| rogpeppe | davecheney: we will get the new instance when we call Storage again | 09:50 |
| davecheney | publish across goroutines | 09:50 |
| rogpeppe | davecheney: why do we need to do that? | 09:50 |
| rogpeppe | davecheney: we've said that there are no long-term users of Storage instances, right? | 09:50 |
| rogpeppe | davecheney: which means that when the current storage operations are complete, the goroutines will call Storage again and get the new version. | 09:51 |
| rogpeppe | davecheney: there, published. | 09:51 |
| davecheney | if goroutine X calls SetCOnfig, we need a mutex to ensure the value of environ.storage is properly published | 09:51 |
| rogpeppe | davecheney: yeah, obviously we still need the mutex on the environ | 09:51 |
| rogpeppe | davecheney: but we make sure that any Storage returned from Environ is immutable. | 09:52 |
| davecheney | rogpeppe: yup, i can do that | 09:58 |
| rogpeppe | davecheney: it's important, then, that environ doesn't call deleteAll directly on the storage instance inside the environ. it should call Storage() first, and then cast .(*storage). | 09:59 |
| davecheney | i'll add a comment block if appropraite | 10:02 |
| davecheney | rogpeppe: this it looking like a much smaller diff | 10:23 |
| davecheney | storage.go is almost untouched | 10:23 |
| rogpeppe | davecheney: that's what i hoped | 10:23 |
| davecheney | # launchpad.net/juju/go/environs/ec2_test | 10:24 |
| davecheney | ./live_test.go:281: undefined: "launchpad.net/juju/go/environs/ec2".DeleteStorageContent | 10:24 |
| davecheney | o_O | 10:24 |
| davecheney | this is against trunk | 10:24 |
| davecheney | let me check I haven't delted that by accident | 10:24 |
| rogpeppe | davecheney: trunk works for me | 10:25 |
| davecheney | ignore that error, ny fault | 10:25 |
| davecheney | my fault | 10:25 |
| TheMue | *rofl* | 10:25 |
| fwereade | rogpeppe, ok, I just plain cannot figure out what is up with the store tests... I think I've nuked everything and started perfectly clean, and I still get 1 pass, 2 fixture panic, 25 missed | 10:42 |
| fwereade | rogpeppe, have you seen this before? | 10:42 |
| rogpeppe | fwereade: yeah, i see it every so often. i'll try again and see if i can reproduce it | 10:42 |
| davecheney | rogpeppe: https://codereview.appspot.com/6256050, tell me what you think | 10:48 |
| davecheney | specifically around immutable storages | 10:49 |
| rogpeppe | davecheney: LGTM | 10:54 |
| rogpeppe | davecheney: and i think this shows the goodness of the approach actually | 10:54 |
| davecheney | i'm relieved that storage.go is virtually unaffected | 10:54 |
| rogpeppe | davecheney: it means that we can still continue to use environs.EmptyStorage when there's no public storage | 10:54 |
| rogpeppe | davecheney: otherwise that wouldn't work. | 10:54 |
| davecheney | coolcoolcool | 10:55 |
| davecheney | so, from the POV of someone who wrote Storage, are you happy with the tradeoffs there ? | 10:55 |
| davecheney | ie, Storage() gives you an immutable reference | 10:55 |
| rogpeppe | davecheney: yeah, i think this works nicely | 10:55 |
| rogpeppe | davecheney: it means that someone external can write the equivalent of deleteAll without worrying about things changing underfoot | 10:56 |
| davecheney | rogpeppe: the only time I had to step outside the API was in instance.Destroy, but it was only two lines | 10:56 |
| * rogpeppe didn't notice that. one mo, while i look. | 10:56 | |
| rogpeppe | davecheney: oh, you mean e.Storage().(*storage) ? | 10:57 |
| davecheney | yup | 10:57 |
| davecheney | e.Storage() gives you your own reference, the cast it too (*storage) | 10:57 |
| rogpeppe | davecheney: yeah, i think that's inevitable, unless we provide a Destroy method on environs.Storage | 10:58 |
| davecheney | this is just so you go through the mutex to get the properly published reference to environ.storageUnlocked | 10:59 |
| rogpeppe | davecheney: because the bucket's existence isn't something dealt with in the storage API | 10:59 |
| rogpeppe | davecheney: exactly; i think i mentioned this case above. | 10:59 |
| rogpeppe | [11:00:00] <rogpeppe> davecheney: it's important, then, that environ doesn't call deleteAll directly on the storage instance inside the environ. it should call Storage() first, and then cast .(*storage). | 10:59 |
| davecheney | jynx | 10:59 |
| davecheney | i'll fix the typos | 11:00 |
| davecheney | whoa, how did that change to storage slip into NewConfig | 11:02 |
| davecheney | right - time for a break, and probably a scotch | 11:12 |
| fwereade | rogpeppe, after much messing around it appears that the only thing that makes any difference is adding a ludicrously long sleep after starting the server in MgoSuite | 12:30 |
| fwereade | rogpeppe, by "ludicrously", I mean that sometimes 10s isn't long enough but it hasn't yet failed with 20s | 12:30 |
| rogpeppe | fwereade: hmm. | 12:30 |
| rogpeppe | fwereade: how is it failing again? | 12:31 |
| fwereade | rogpeppe, failing to dial | 12:31 |
| hazmat | fwereade, the server hasn't started? | 12:31 |
| fwereade | rogpeppe, no reachable servers | 12:31 |
| fwereade | hazmat, yeah | 12:31 |
| rogpeppe | fwereade: maybe the dial should retry? | 12:31 |
| fwereade | rogpeppe, reading the docs it appears it should anyway | 12:32 |
| fwereade | rogpeppe, I'll try DialWithTimeout again (10s is usual I think; I tried 20s, but I should probably try 30s to match the always-succeed-with-sleep rate) | 12:32 |
| rogpeppe | fwereade: sounds like a plan | 12:33 |
| rogpeppe | fwereade: what is it with these servers that forever to come up? | 12:33 |
| fwereade | rogpeppe, I have no idea, I really wasn't expecting that | 12:34 |
| fwereade | rogpeppe, I've spent all morning poking around at entirely unrelated things :/ | 12:34 |
| rogpeppe | fwereade: i know the feeling! | 12:34 |
| rogpeppe | fwereade: hmm, having fixed the bug in madeBucket, one of my old tests fails. it *should* fail given the new semantics, but i'm not sure now if they're right. | 12:35 |
| fwereade | rogpeppe, heh :( | 12:36 |
| rogpeppe | fwereade: the test scenario is we have two Environs, both pointing to the same remote environ, e and e2; we do this: e.Bootstrap(); e2.Destroy(); e.Bootstrap() | 12:36 |
| rogpeppe | fwereade: do we care about that working or not? | 12:37 |
| fwereade | rogpeppe, tbh that feels somewhat edgey to me | 12:37 |
| rogpeppe | fwereade: me too. when are we ever gonna call Bootstrap twice on the same Environ, right? | 12:37 |
| rogpeppe | Aram: yo! | 12:37 |
| Aram | hi. | 12:38 |
| fwereade | rogpeppe, indeed; if reported the response should probably be "don't do that please" | 12:38 |
| rogpeppe | fwereade: yeah. i think i'll change the test. | 12:38 |
| rogpeppe | fwereade: and perhaps the docs: "When Destroy has been called, all Environs referring to the same remote environment may become invalid", or something. | 12:40 |
| fwereade | rogpeppe, yeah, sounds pretty clear | 12:41 |
| rogpeppe | fwereade: you gonna submit that AddCharm fix? | 12:43 |
| fwereade | rogpeppe, yeah, I got distracted with trying to make all tests pass :/ | 12:45 |
| fwereade | rogpeppe, I'll submit that one separately right now | 12:45 |
| rogpeppe | fwereade: there are still more that fail too. i get sporadic StateSuite.TestUnitWatchPorts failures. | 12:45 |
| fwereade | rogpeppe, I saw that one once, indeed | 12:45 |
| rogpeppe | fwereade: i think TheMue has it in hand | 12:46 |
| fwereade | rogpeppe, cool | 12:46 |
| TheMue | rogpeppe, fwereade: Not yet really, only the idea of a race condition. | 12:47 |
| TheMue | rogpeppe: We talked about inserting a little sleep. But it could be a similar flaky behavior in production environment. So I've got to analyze your laste change deeper and how to avoid this behavior directly in the content watcher. | 12:50 |
| rogpeppe | TheMue: no need to insert a sleep. | 12:50 |
| rogpeppe | TheMue: i can see the problem, and i know the fix. | 12:50 |
| TheMue | rogpeppe: Oh, so feel free. I've got two branches in queue before. | 12:51 |
| TheMue | rogpeppe: What is your idea now? | 12:51 |
| rogpeppe | TheMue: i think! i haven't tested it yet, but i've just written the code (one line) | 12:51 |
| TheMue | rogpeppe: Small and beautiful ;) | 12:51 |
| rogpeppe | TheMue: insert a null op as the first test in unitWatchPortsTests | 12:52 |
| rogpeppe | TheMue: expecting an empty ports array | 12:52 |
| TheMue | rogpeppe: It's not only the port test. | 12:52 |
| TheMue | rogpeppe: There are more failing, all based on the content watcher. | 12:52 |
| rogpeppe | TheMue: the same would apply to any similar test | 12:52 |
| TheMue | rogpeppe: But so you fix the test, but not the nehavior. | 12:53 |
| rogpeppe | TheMue: the tests need to take account of the fact that the watcher *always* sends an initial value on the channel | 12:53 |
| rogpeppe | TheMue: i think that's reasonable behaviour | 12:53 |
| rogpeppe | TheMue: if we don't think that, then we can fix the behaviour | 12:53 |
| TheMue | rogpeppe: The idea of always sending is ok. But the fact, that we sometimes get errors and sometimes not. | 12:54 |
| rogpeppe | TheMue: but that behaviour was deliberately made that way; i don't mind if we change it to be different. | 12:54 |
| rogpeppe | TheMue: that's a bug in the tests. | 12:54 |
| rogpeppe | TheMue: they're creating the watcher and setting the content immediately after. there's a race there. | 12:54 |
| TheMue | rogpeppe: And what do you do when this happens in production? | 12:55 |
| rogpeppe | TheMue: there's no race unless you're trying to predict what the events are coming on to the channel, as the tests are. | 12:55 |
| rogpeppe | TheMue: each of those events is a valid representation of the contents at some point in the past. | 12:56 |
| rogpeppe | TheMue: and that's all we want from the contents watcher | 12:56 |
| TheMue | rogpeppe: I'll take a look at your change. | 12:56 |
| rogpeppe | TheMue: i'll push it in a mo | 12:57 |
| TheMue | fwereade: I've got to change the endpoint related methods to not separate between provider/requirer and peer. Instead I shall change it to …(endpoints …RelationEndpoint). OK, less functions, but more complex code imho. | 12:59 |
| fwereade | TheMue, yeah, I think that's a shame, IMO it makes it much harder to see what's going on | 13:00 |
| fwereade | TheMue, but hey ho | 13:00 |
| fwereade | TheMue, sorry for the noise, I guess :( | 13:00 |
| TheMue | fwereade: np ;) | 13:01 |
| rogpeppe | TheMue: there's an issue with ResolvedWatcher | 13:08 |
| rogpeppe | TheMue: when the content is empty, parseResolvedMode fails. | 13:09 |
| TheMue | rogpeppe: Will have to look at the change history. IMHO I've tested it. | 13:09 |
| rogpeppe | TheMue: yes, but before we weren't seeing that initial blank value. i guess it's being created blank, but perhaps it should be created with a valid resolved content. | 13:10 |
| rogpeppe | TheMue: ah, no! i guess that it just didn't exist before. | 13:11 |
| TheMue | rogpeppe: Yep. | 13:11 |
| rogpeppe | TheMue: so the first value on the content-changed channel is "no node existing" | 13:11 |
| rogpeppe | TheMue: what should the value of "resolved" be then? | 13:12 |
| rogpeppe | TheMue: maybe it should just be ResolvedNone if there's no node. | 13:14 |
| TheMue | rogpeppe: Sorry, I'm in a totally different context currently. IMHO the resolve change is relative new even for the Python version. Maybe fwereade can help. | 13:15 |
| TheMue | rogpeppe: I have to take some to to see the change history here too and look why it now fails. | 13:17 |
| TheMue | some time | 13:17 |
| rogpeppe | TheMue: i think it's quite straightforward - if you do WatchResolved on a unit without a resolved node, what should you read from the channel? | 13:18 |
| rogpeppe | TheMue: ah! the answer is there in Unit.Resolved | 13:19 |
| rogpeppe | TheMue: ResolvedNone is correct. | 13:19 |
| TheMue | rogpeppe: Didn't the original implementation did its first shot when the node is created, not before? | 13:20 |
| rogpeppe | TheMue: yes, but in keeping with the current thinking on watchers, we want to send the current state as the first event on the channel. | 13:21 |
| rogpeppe | TheMue: which in this case is ResolvedNone. | 13:22 |
| TheMue | rogpeppe: Yes, that's ok. But I'm a bit astonished that the change is in the code based but dependencies havn't been changed so far. | 13:23 |
| TheMue | code base | 13:23 |
| rogpeppe | TheMue: which dependencies? | 13:23 |
| TheMue | rogpeppe: You now see several follow-up, like in the tests. | 13:24 |
| rogpeppe | TheMue: sorry, i don't understand | 13:24 |
| TheMue | rogpeppe: The checkin of the changed watcher has been too early. The same branch IMHO has to change the tests or the problem with the parseResolvedMode. | 13:25 |
| rogpeppe | TheMue: i haven't checked in anything | 13:25 |
| rogpeppe | TheMue: i'm fixing those problems currently. | 13:26 |
| rogpeppe | TheMue: and the fact that ChildWatcher doesn't always send a first event. | 13:26 |
| TheMue | rogpeppe: The behavior of the content watcher changed. That's what I'm talking about. | 13:27 |
| rogpeppe | TheMue: ah yes. that's true. the problem was that the tests usually passed! | 13:28 |
| rogpeppe | TheMue: so i'm fixing that now. | 13:28 |
| TheMue | rogpeppe: Yip, here we have to find a proper way, like a continous integration server running all tests after each submit. | 13:28 |
| rogpeppe | TheMue: i think it's fine. as niemeyer said recently, we just need to make sure we run the tests ourselves before submitting. | 13:29 |
| rogpeppe | TheMue: ok, i have all tests passing reliably (i think!) now. | 13:30 |
| TheMue | rogpeppe: Do you run all tests? Off each package? | 13:30 |
| TheMue | rogpeppe: Great, cheers. | 13:30 |
| rogpeppe | TheMue: just the state package currently. | 13:31 |
| TheMue | rogpeppe: The cheers is for the fixing. ;) | 13:31 |
| TheMue | rogpeppe: I would like some kind of running all (!) tests at least once per day. | 13:31 |
| rogpeppe | TheMue: i'm gonna fix the watcher tests too. | 13:31 |
| rogpeppe | TheMue: we all run the tests at least once a day, i hope! | 13:32 |
| TheMue | rogpeppe: Do you have a script for it or do you go into each package manually? | 13:32 |
| rogpeppe | TheMue: go test launchpad.net/juju/go/... | 13:33 |
| rogpeppe | TheMue: or just go test ./... | 13:33 |
| rogpeppe | from the root of the go hierarchy | 13:33 |
| TheMue | rogpeppe: Oh, the elipses works? I didn't know, great, that helps a lot. | 13:33 |
| rogpeppe | the go tool is lovely like that | 13:33 |
| TheMue | rogpeppe: Wow, thx for that hint. | 13:33 |
| rogpeppe | TheMue: the ellipses are a wildcard - you can do go test .../state | 13:34 |
| Aram | rogpeppe: do you use bzr manually or do you use niemeyer's wrapper? | 13:34 |
| rogpeppe | or go install .../test/.../foo | 13:34 |
| rogpeppe | Aram: i use niemeyer's cobzr wrapper | 13:34 |
| Aram | ok, I'll look into it. | 13:34 |
| rogpeppe | Aram: it makes the branches work well with $GOPATH | 13:34 |
| rogpeppe | Aram: otherwise it's a right pain | 13:34 |
| rogpeppe | TheMue: https://codereview.appspot.com/6245053 | 13:44 |
| TheMue | *click* | 13:44 |
| rogpeppe | TheMue, fwereade: i'm going to be away for an hour or two soon. | 13:46 |
| TheMue | rogpeppe: OK | 13:46 |
| fwereade | rogpeppe, np, I'm finishing in a mo too | 13:46 |
| fwereade | rogpeppe, might manage to push a couple of CLs before I do | 13:47 |
| rogpeppe | fwereade: cool. will have a look later. | 13:47 |
| TheMue | Btw, I've got a public holiday on Monday. So don't wonder if you don't reach me. | 13:47 |
| rogpeppe | TheMue: ah, ok. sometime we're going to have to try to do a meeting! | 13:48 |
| rogpeppe | TheMue: you know Aram is starting on monday, right? | 13:48 |
| TheMue | rogpeppe: Yep, good news. ;) | 13:48 |
| rogpeppe | TheMue: indeed. maybe we should have a meeting on tuesday? | 13:49 |
| TheMue | rogpeppe: Would be fine to me. | 13:49 |
| rogpeppe | fwereade: ? | 13:49 |
| rogpeppe | fwereade: i.e. would you be up for a meeting on tues? | 13:50 |
| fwereade | rogpeppe, tuesday would be fine | 13:50 |
| rogpeppe | fwereade: probs around 8pm oz time | 13:50 |
| fwereade | rogpeppe, is there only one time zone there? | 13:50 |
| rogpeppe | fwereade: which i think was what we agreed before | 13:50 |
| rogpeppe | fwereade: no idea. i mean sydney time, of course. | 13:50 |
| fwereade | rogpeppe, ok, so my midday I guess; that sounds great | 13:52 |
| rogpeppe | fwereade: not so good for niemeyer i think | 13:52 |
| TheMue | rogpeppe: Yep, midday is ok, lunch is later. :) | 13:52 |
| rogpeppe | fwereade: maybe he should call the time. | 13:53 |
| TheMue | rogpeppe: It's 7:00 then for him. | 13:53 |
| rogpeppe | TheMue: exactly. | 13:53 |
| rogpeppe | TheMue: maybe 9pm and 8am might be better | 13:53 |
| fwereade | rogpeppe, TheMue: I'm easy, but as you say niemeyer should probably call it | 13:53 |
| TheMue | rogpeppe: Isn't he in UK next week? | 13:54 |
| rogpeppe | TheMue: that's true | 13:54 |
| Aram | what timezone is dave cheney in? | 13:54 |
| rogpeppe | Aram: sydney, australia | 13:55 |
| Aram | ah. | 13:55 |
| TheMue | rogpeppe: LGTM | 13:58 |
| rogpeppe | TheMue: thanks | 13:58 |
| rogpeppe | niemeyer: yo! | 13:59 |
| Aram | hi ni | 13:59 |
| Aram | niemeyer: | 13:59 |
| niemeyer | Hello! | 13:59 |
| rogpeppe | niemeyer: you've arrived just as i'm leaving for a couple of hours | 13:59 |
| rogpeppe | niemeyer: got a couple more CLs for you to look at though. just bug fixes. | 13:59 |
| niemeyer | rogpeppe: Super | 14:00 |
| niemeyer | rogpeppe: Thanks | 14:00 |
| TheMue | niemeyer: Moin | 14:02 |
| niemeyer | Heya | 14:03 |
| rogpeppe | see y'all in a bit | 14:04 |
| TheMue | niemeyer: Just for info, the earring is back. ;) | 14:46 |
| niemeyer | TheMue: Hah, awesome :) | 14:46 |
| TheMue | niemeyer: One of the side jobs: plumber. | 14:47 |
| niemeyer | TheMue: Hehe, I know how it feels :) | 14:47 |
| Aram | meh, vmware website is getting worse every day. | 15:01 |
| TheMue | niemeyer: Could you please take a look at topology.py line 567 ff? | 15:02 |
| niemeyer | TheMue: Local or branch? | 15:03 |
| TheMue | niemeyer: Current trunk (python). | 15:03 |
| niemeyer | TheMue: Ah, .py | 15:03 |
| TheMue | niemeyer: There the interface of the actual relation in the loop is tested only against the interface of endpoint 0. | 15:04 |
| niemeyer | TheMue: There | 15:04 |
| niemeyer | TheMue: Ok | 15:04 |
| niemeyer | TheMue: ? | 15:04 |
| TheMue | niemeyer: When doing a test now with two endpoints with different interfaces, but the rest is ok, it seems to return a key instead of an error. | 15:05 |
| TheMue | niemeyer: Is that ok? | 15:05 |
| niemeyer | TheMue: How could this possibly happen? | 15:05 |
| niemeyer | TheMue: That would mean there's a relation with two different interfaces on it? | 15:06 |
| TheMue | niemeyer: No, adding is tested ok. But the user of the function may test two invalid endpoints (I've done so in my unit test, extra). | 15:06 |
| TheMue | niemeyer: IMHO a test that all passed endpoints share the same interface could be added. | 15:07 |
| niemeyer | TheMue: Yeah, that sounds fine | 15:08 |
| TheMue | niemeyer: OK, will add it, the rest of the new function RelationKey() has been changed according to your review. That's the last point. | 15:09 |
| niemeyer | TheMue: Super, thanks! | 15:09 |
| TheMue | niemeyer: For the NoRelationError string I've tried %+v, which is really good due to the field names, but long, and %v, which only shows the values of the endpoints. What do you prefer, short or long? | 15:10 |
| niemeyer | TheMue: Neither.. that's an error message.. should be oriented towards the user, not with types and debugging information | 15:11 |
| niemeyer | TheMue: An endpoint is uniquely identified by service name + relation name | 15:12 |
| TheMue | niemeyer: Hehe, that's why I wrote a RelationEndpoint.String() where you told me that it's not needed, instead %v. | 15:12 |
| niemeyer | TheMue: There can't be two different relations between two services with the same (service name, relation name) => (service name, relation name) characteristics | 15:13 |
| niemeyer | TheMue: Yeah, because you were *already* printing debugging information there | 15:13 |
| niemeyer | TheMue: In a way that is not understandable to a user nor to a developer | 15:13 |
| TheMue | niemeyer: OK, what about a method Id() string for RelationEndpoint returning exactly this with a colon? | 15:13 |
| niemeyer | TheMue: That's what I was against | 15:13 |
| TheMue | niemeyer: And I would use that id in the error generation. | 15:14 |
| niemeyer | TheMue: foo:bar:baz:baco:bar is not readable | 15:14 |
| TheMue | error message | 15:14 |
| niemeyer | TheMue: Yeah, sounds fine in principle | 15:15 |
| niemeyer | TheMue: Not sure I have the whole picture right now, but we can try it | 15:15 |
| niemeyer | I'll head to lunch and be back in a bit | 15:16 |
| TheMue | niemeyer: The message would be like ("… between %q and %q", ep1.Id(), ep2.Id()) , where %q later is "wordpress:blog" etc. | 15:16 |
| niemeyer | TheMue: I suggest using String() | 16:03 |
| niemeyer | TheMue: So you can say between %s and %s | 16:03 |
| niemeyer | , ep1, ep2, | 16:03 |
| === wrtp is now known as rogpeppe | ||
| rogpeppe | niemeyer, TheMue, fwereade: yo! | 16:08 |
| niemeyer | rogpeppe: Welcome back! | 16:10 |
| * niemeyer sips a delicious post-lunch coffee | 16:10 | |
| rogpeppe | niemeyer: looks out over the sunny Tyne river | 16:11 |
| * rogpeppe looks out over the sunny Tyne river :) | 16:11 | |
| rogpeppe | i just took a little time to see a final year recital from a friend who's a folk degree students here. t'was amazing. now using the free wi-fi at the Sage, Gateshead. | 16:14 |
| niemeyer | rogpeppe: Where's that? | 16:14 |
| rogpeppe | niemeyer: http://en.wikipedia.org/wiki/The_Sage_Gateshead | 16:15 |
| rogpeppe | niemeyer: am currently sitting just inside the glass just above the smaller of the two boats you can see in that picture. and the sky is just as blue. | 16:16 |
| rogpeppe | niemeyer: it's a fantastic building | 16:16 |
| rogpeppe | niemeyer: have you got any suggestions for the best way to do the putFakeTools thing? | 16:25 |
| niemeyer | rogpeppe: Woah | 16:25 |
| niemeyer | rogpeppe: This is amazing | 16:25 |
| rogpeppe | niemeyer: what is? | 16:25 |
| niemeyer | rogpeppe: That building | 16:26 |
| rogpeppe | niemeyer: it certainly is | 16:26 |
| rogpeppe | niemeyer: although some have compared it to a sea slug :-) | 16:26 |
| niemeyer | rogpeppe: It reminds me of the rubber that protects a car's wheel transmission | 16:28 |
| niemeyer | rogpeppe: Has the same shape | 16:28 |
| rogpeppe | :-) | 16:28 |
| niemeyer | rogpeppe: But I digress wildly :) | 16:28 |
| rogpeppe | niemeyer: i'm sitting just near to where the woman on the left is in this picture: http://en.wikipedia.org/wiki/File:Newcastle_the_sage_innen.jpg | 16:28 |
| niemeyer | rogpeppe: Regarding the putFakeTools, if you want to keep the tools fake, sounds fine, but I still see that as a lot of logic for what is basically "List + Get" | 16:29 |
| rogpeppe | niemeyer: and that arc you can see outside above her head is also an amazing piece of engineering, the millenium bridge | 16:29 |
| rogpeppe | niemeyer: well, yes. | 16:29 |
| rogpeppe | niemeyer: i'm mildly inclined to export ToolsPath from environs | 16:29 |
| niemeyer | rogpeppe: Sounds sensible too | 16:29 |
| rogpeppe | niemeyer: which would simplify the logic a lot, but slightly clutter the public API for the sake of testing. | 16:30 |
| rogpeppe | niemeyer: which i don't like doing. | 16:30 |
| niemeyer | rogpeppe: I'd prefer that we had at least a couple of variables on it, to make it explicit what it is depending upon | 16:30 |
| rogpeppe | niemeyer: ? variables on what? | 16:31 |
| niemeyer | rogpeppe: environs.ToolsPath(version, arch, "12.04") | 16:31 |
| rogpeppe | niemeyer: definitely. i think that function already might exist. | 16:32 |
| rogpeppe | niemeyer: just not exported | 16:32 |
| rogpeppe | 12.04 or precise? :-) | 16:32 |
| rogpeppe | niemeyer: BTW, i caught this piece of your conversation last night with davecheney: http://paste.ubuntu.com/1006651/ | 16:33 |
| rogpeppe | niemeyer: i beg to disagree :) | 16:33 |
| niemeyer | rogpeppe: Yeah, good question, I kind of put the literal to talk about it :) | 16:33 |
| rogpeppe | niemeyer: do you have any idea how many allocations PutFile does? | 16:33 |
| niemeyer | rogpeppe: I don't, and I see no reason to increase it whatever that number looks like | 16:34 |
| rogpeppe | niemeyer: 1780 | 16:34 |
| rogpeppe | more than | 16:34 |
| niemeyer | rogpeppe: Nice, let's make it larger then! Not. | 16:34 |
| rogpeppe | niemeyer: 3 allocations is lost in the noise | 16:34 |
| rogpeppe | niemeyer: (literally - it's within the standard deviation) | 16:34 |
| niemeyer | rogpeppe: Yeah, sounds great. Let's do caching please, it's simpler. | 16:35 |
| rogpeppe | niemeyer: the code is more complex actually. i don't mind too much, but i just had to take a little issue with the reasoning. | 16:35 |
| rogpeppe | niemeyer: anyway, "precise" vs "12.04" | 16:36 |
| niemeyer | rogpeppe: No, it's not, and we don't have to discuss this because we have evidence.. two different bugs were already introduced on the way of removing caching.. | 16:36 |
| rogpeppe | niemeyer: i like the former because it's obviously an ubuntu release name. i like the latter because it's more amenable to program manipulation. | 16:37 |
| niemeyer | rogpeppe: Well, let's have "precise" then.. it's already around the code base, and we can actually manipulate it too now that I think of it. | 16:38 |
| niemeyer | rogpeppe: Still curious about what happens after 'z', but I'm sure *some* form of the logic will remain | 16:38 |
| rogpeppe | niemeyer: yeah, i'm slightly concerned about that. | 16:38 |
| rogpeppe | (if the code's simpler, i'm all for it in general - it was just the "it allocates more therefore let's do it differently" reasoning i wasn't keen on. | 16:39 |
| rogpeppe | ) | 16:39 |
| niemeyer | rogpeppe: If you read properly, you'll see in the context that this is just yet another argument among the others | 16:41 |
| niemeyer | rogpeppe: and that argument is *also* true. | 16:41 |
| niemeyer | fwereade: ping | 16:47 |
| rogpeppe | niemeyer: fair enough | 16:53 |
| niemeyer | fwereade, rogpeppe, TheMue, Aram: Folks, just as a reminder, next week I'll be in London | 16:55 |
| rogpeppe | niemeyer: do you think you'll be able to make a meeting on Tues? | 16:55 |
| niemeyer | I'm not sure about how the flow of the sprint will look like | 16:56 |
| niemeyer | But I'll try to keep on top of reviews and communication as usual | 16:56 |
| TheMue | niemeyer: *envy* | 16:56 |
| Aram | btw niemeyer, do you think I'll be able to access the wiki by monday? | 16:56 |
| Aram | I don't have permission right now :). | 16:56 |
| niemeyer | Aram: I'm hoping so | 16:56 |
| TheMue | niemeyer: I only have been there on the airport so far. Would like to see more. | 16:56 |
| niemeyer | Aram: There's a "nice" list of tasks for you to go through after starting | 16:56 |
| niemeyer | TheMue: I won't be able to see much more than the airport, and the office, I'm afraid :) | 16:57 |
| niemeyer | TheMue: Well, the office is still in a fantastic spot, though.. | 16:57 |
| niemeyer | That's changing soon unfortunately | 16:57 |
| niemeyer | Aram: That said, hmm | 16:57 |
| niemeyer | Aram: DOn't be super enthusiastic about it.. As far as juju goes, it's pretty much all public | 16:57 |
| TheMue | niemeyer: The new one seems to be nice too. Only not such a good outlook. | 16:58 |
| niemeyer | rogpeppe: Most probably, re. the meeting | 16:58 |
| rogpeppe | niemeyer: i'm thinking that it would be nice if we could attempt at least one face-to-face with Aram next week. | 16:58 |
| niemeyer | rogpeppe: Wasn't it Monday, btw? | 16:58 |
| niemeyer | rogpeppe: Yeah, that sounds like a great idea actually | 16:58 |
| rogpeppe | niemeyer: yeah, but TheMue has a public holiday on mon | 16:59 |
| rogpeppe | niemeyer: how about early one morning? | 16:59 |
| niemeyer | rogpeppe: Ah, sounds good then | 16:59 |
| niemeyer | rogpeppe, Aram: What about Friday? | 16:59 |
| rogpeppe | niemeyer: then it'll be ok for davecheney | 16:59 |
| Aram | niemeyer: next friday? as in one feek from now? | 16:59 |
| Aram | sure. | 16:59 |
| Aram | anytime. | 17:00 |
| niemeyer | Aram: Yeah, in London | 17:00 |
| niemeyer | :) | 17:00 |
| niemeyer | rogpeppe: Sounds good.. good chance given we're all closer in terms of timing | 17:00 |
| rogpeppe | niemeyer: BTW by "face to face" i meant G+... | 17:00 |
| niemeyer | rogpeppe: ROTFL | 17:01 |
| niemeyer | rogpeppe: That's what face-to-face has turned into? :-) | 17:01 |
| niemeyer | rogpeppe: I actually mean *real* face-to-face.. :) | 17:01 |
| rogpeppe | niemeyer: more so than IRC :-) | 17:01 |
| niemeyer | Aram: Can you get to London on Friday? | 17:01 |
| rogpeppe | niemeyer: actually next fri would work for me | 17:01 |
| niemeyer | Aram: How troublesome would it be for you? | 17:01 |
| rogpeppe | niemeyer: seems like a long way to come for a short time though. | 17:02 |
| niemeyer | rogpeppe: It pays off.. | 17:03 |
| niemeyer | rogpeppe: The next several weeks will be a lot nicer for Aram after having met people personally | 17:03 |
| Aram | niemeyer: I guess I could come, have to check up flights and whatnot, never bought tickets from here. | 17:03 |
| rogpeppe | niemeyer: FWIW i can get to london in about 3h15m | 17:05 |
| rogpeppe | niemeyer: yeah, i agree with that | 17:05 |
| rogpeppe | niemeyer: when do you fly back? | 17:05 |
| niemeyer | rogpeppe: Friday too | 17:05 |
| niemeyer | rogpeppe: EOD | 17:06 |
| niemeyer | Maybe Thursday would be better | 17:06 |
| niemeyer | Aram: Which city are you in again? | 17:06 |
| Aram | vienna | 17:07 |
| niemeyer | Aram: I'll send a probe to the agency that books flights for us | 17:08 |
| niemeyer | Aram: Will CC you | 17:08 |
| Aram | please, thanks | 17:08 |
| rogpeppe | looks like flights from vienna to heathrow are ok | 17:09 |
| rogpeppe | niemeyer, Aram: http://www.skyscanner.net/flights/vie/lond/120530/120602/airfares-from-vienna-to-london-in-may-2012-and-june-2012.html | 17:09 |
| * Aram has to leave for a few hours, sorry. | 17:10 | |
| Aram | I'll be back. | 17:10 |
| rogpeppe | Aram: see you monday! | 17:11 |
| niemeyer | Mail sent | 17:11 |
| niemeyer | rogpeppe: Yeah, not to bad | 17:12 |
| niemeyer | too | 17:12 |
| niemeyer | Aram: Cheers | 17:12 |
| rogpeppe | niemeyer: i've just proposed a new version of https://codereview.appspot.com/6245048 with simplified putFakeTools | 17:13 |
| niemeyer | rogpeppe: Looking | 17:15 |
| rogpeppe | niemeyer: i've got to head off now. will poke my head in later. | 17:20 |
| rogpeppe | niemeyer, TheMue, Aram, fwereade: have a great w/e | 17:20 |
| TheMue | rogpeppe: Thx, you too. | 17:20 |
| niemeyer | rogpeppe: Review delivered | 17:21 |
| niemeyer | rogpeppe: Thanks for the quick turn around | 17:22 |
| TheMue | niemeyer: Btw, changed Id() to String() of the RelationEndpoint too. | 17:28 |
| niemeyer | TheMue: Sweet, is it ready for another round? | 17:49 |
| TheMue | niemeyer: Yep, it's in the box. ;) | 17:49 |
| niemeyer | TheMue: Checking | 17:49 |
| niemeyer | TheMue: Diff looks bogus | 17:54 |
| niemeyer | TheMue: https://codereview.appspot.com/6198055/ | 17:54 |
| TheMue | niemeyer: Ooops, but I set the prerequisite. | 17:55 |
| niemeyer | TheMue: Yeah, this is a bug in lbox that I still haven't fixed | 17:55 |
| TheMue | niemeyer: Hmm, but that is now in the trunk. May that be the reason? | 17:55 |
| niemeyer | TheMue: The problem is that the pre-req has been merged | 17:56 |
| niemeyer | TheMue: But lbox is still comparing against the pre-req | 17:56 |
| niemeyer | TheMue: I'm sorry about that | 17:56 |
| niemeyer | TheMue: To fix it, merge trunk on the pre-req, and push it | 17:56 |
| niemeyer | TheMue: (with bzr push only) | 17:56 |
| niemeyer | TheMue: Then, re-propose the follow up | 17:56 |
| niemeyer | TheMue: This will get the diff clean again | 17:56 |
| niemeyer | Ouch, forgot to buy the new battery | 17:59 |
| TheMue | niemeyer: OOK, wait a moment, will try. | 18:01 |
| TheMue | niemeyer: Ah, great, thx, looks cleaner now. | 18:02 |
| niemeyer | TheMue: Wow, yeah :) | 18:12 |
| niemeyer | TheMue: Review delivered | 18:25 |
| niemeyer | TheMue: The logic there is not quite right | 18:25 |
| TheMue | niemeyer: In which way? | 18:27 |
| niemeyer | TheMue: Please check the review and let me know if you need further details | 18:27 |
| TheMue | niemeyer: Done, that's why I ask. ;) | 18:28 |
| niemeyer | TheMue: Ok.. so you don't know that a *relation name* and a *service name* are not the same thing? | 18:28 |
| TheMue | niemeyer: I took line 571 in topology.py as example | 18:30 |
| TheMue | niemeyer: There is service["name"] != endpoint.relation_name | 18:31 |
| niemeyer | TheMue: The code is misleading indeed, but it's important to keep an eye on what you are actually doing | 18:32 |
| niemeyer | TheMue: What is inside that "service" dictionary? | 18:32 |
| niemeyer | TheMue: Have a look at assign_service_to_relation | 18:32 |
| TheMue | niemeyer: Ah, the relation name of the service. | 18:35 |
| niemeyer | TheMue: Do you have a good mental model of what a relation name is? | 18:37 |
| niemeyer | TheMue: That's not a trick question.. if you don't, let's talk about it | 18:38 |
| TheMue | niemeyer: It's a part of the relation identifier, but I find the name "name" mislieading. | 18:39 |
| niemeyer | TheMue: Ok.. so let's pick a metadata.yaml to look at | 18:39 |
| * niemeyer looks | 18:39 | |
| TheMue | niemeyer: One moment, have to help my wife. | 18:40 |
| niemeyer | TheMue: https://juju.ubuntu.com/docs/charm.html#the-metadata-file | 18:40 |
| niemeyer | TheMue: At the end of the doc there are some samples | 18:40 |
| niemeyer | TheMue: The relation name is that "db" under provides or requires | 18:41 |
| niemeyer | TheMue: Now, imagine that I have an application called mywiki that takes a couple of mongodbs | 18:41 |
| niemeyer | TheMue: One of them is for caching, and the other contains the actual app data | 18:42 |
| niemeyer | TheMue: Both of these relations will have the "mongodb" interface, for example | 18:42 |
| niemeyer | TheMue: But we need to disambiguate them so we can refer to them | 18:42 |
| niemeyer | TheMue: So we might call one of them "data" and the other "cache" | 18:42 |
| niemeyer | TheMue: Both with interface "mongodb" | 18:42 |
| niemeyer | TheMue: So when I'm adding a relation, I could say something like | 18:43 |
| niemeyer | TheMue: juju add-relation mywiki:data themongoservice | 18:43 |
| niemeyer | For example.. | 18:43 |
| niemeyer | TheMue: TO connect that specific relation | 18:43 |
| niemeyer | TheMue: So it is indeed a "relation name" | 18:43 |
| niemeyer | TheMue: Makes sense? | 18:43 |
| TheMue | niemeyer: Ah, ok, thank you, makes it more clear. | 18:49 |
| niemeyer | TheMue: np | 18:49 |
| TheMue | niemeyer: OK, so I'll change it (with a glas of whine beside me) | 18:50 |
| TheMue | niemeyer: For the interface error I used a plain error to return informations about the interface name. | 18:50 |
| niemeyer | TheMue: Will keep that in mind when reviewing.. LOL | 18:50 |
| TheMue | niemeyer: But indeed, I can change it to a NoRelationError too. | 18:51 |
| TheMue | niemeyer: Hehe, only one wine (or two or three *hicks*) | 18:51 |
| niemeyer | ;-) | 18:53 |
| TheMue | niemeyer: One question, coult you please take a look at https://codereview.appspot.com/6200044/diff/3001/state/topology.go. | 19:26 |
| TheMue | niemeyer: This has been my first approach before UDS. In line 50 and 55 you see that I used a type to contain the name information. | 19:27 |
| niemeyer | TheMue: Checking | 19:28 |
| niemeyer | TheMue: Ok? | 19:28 |
| TheMue | niemeyer: After the discussion with William I switched to a map of role to service key. Our idea has been that the name relates to the service name and that the key is better here. A wrong assumption, as you now showed to me. | 19:28 |
| TheMue | niemeyer: So now I would like to reintroduce this type, but containing the serviceKey (like today the sting in the map) and the relationName. What do you think? | 19:29 |
| niemeyer | TheMue: Sounds reasonable | 19:29 |
| TheMue | niemeyer: Fine, thanks. | 19:30 |
| niemeyer | TheMue: Thank you! | 19:30 |
| niemeyer | go-environs-setconfig is ready to go.. OMG.. so happy | 19:36 |
| andrewsmedina | niemeyer: nice! | 20:22 |
| niemeyer | Woohoo.. clean queue again | 20:42 |
| niemeyer | I'm heading out for some exercising | 20:42 |
| TheMue | niemeyer: I have to disappoint you. Just reproposed the latests branch. ;) | 20:57 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!
