/srv/irclogs.ubuntu.com/2012/06/06/#juju-dev.txt

* niemeyer pops up01:14
niemeyerOkay.. time for some sleep01:37
niemeyerNight all01:37
Arammorning.05:02
davecheneyhowdy doodie06:23
davecheneyputting my head in the lions mouth, http://codereview.appspot.com/6297048/06:23
fwereademornings06:46
fwereadeTheMue, ping10:20
TheMuefwereade: pong10:20
fwereadeTheMue, just thought I'd update you quickly re service/unit relations10:20
fwereadeTheMue, I have something that I think looks good10:21
TheMuefwereade: I'm listening.10:21
fwereadeTheMue, but it's going to be a fair number of branches, which will roughly go:10:21
fwereadeadd state.watcher.VersionWatcher10:21
fwereadeadd state.RelatedUnitWatcher, which watches both the presence node and the settings node of a given unit relation10:22
fwereadeadd state.RelatedUnitsWatcher, which watches a set of unit relations and maintains a RelatedUnitWatcher for each10:23
fwereadeadd state.UnitRelation, the tricky bit of which is WatchRelated (which depends on all the foregoing)10:24
fwereadeadd state.ServiceRelation10:24
fwereadephew10:24
TheMuefwereade: Hehe, good list.10:24
TheMuefwereade: ServiceRelation is already started.10:24
fwereadeTheMue, there's a struct without methods, right?10:25
TheMuefwereade: UnitRelation is on the list.10:25
fwereadeTheMue, I've basically written all that code10:25
fwereadeTheMue, needs far better tests, and to be broken into that nice clean structure10:26
fwereadeTheMue, but I wanted to check we weren't duplicating too much10:26
TheMuefwereade: Currently it has the first few accessor methods, but more are planned (matching the Py impl.).10:26
fwereadeTheMue, ah yeah, that's right, sorry10:26
fwereadeTheMue, that was what I was starting from10:26
TheMuefwereade: No problem, it's not yet started.10:27
fwereadeTheMue, cool; just wanted to reassure you that it was coming, and check you didn't need it imminently10:27
TheMuefwereade: I had two smaller branches I'm working at before going on.10:27
TheMuefwereade: So it fits.10:27
fwereadeTheMue, cool10:27
TheMuefwereade: One orthogonal issue is the harmonization of error handling in the state code.10:28
fwereadeTheMue, I'm +1 on the general concept, but not really sure what we're planning10:29
fwereadeTheMue, the thing is, I do often appreciate the details of the naked errors that bubble up, so I kinda favour a nested error type... would something like that work for you?10:30
TheMuefwereade: Right now too many low level errors pop up to the user, like errors containing internal keys or ZK messages.10:30
fwereadeTheMue, although I'd be willing to lose that so long as we log the errors whenever we transform them10:31
fwereadeTheMue, actually, I think I like that idea10:32
niemeyerGood morning folks10:32
fwereadeniemeyer, heyhey10:32
TheMuefwereade: In private I'm using specific error types like e.g. ServiceNotFoundError. As a struct it can contain further infos as fields, like keys. And Error() returns the message. It's a bit more effort, but testing and analyzing is simpler this way. Also later I18N.10:33
fwereadeTheMue, SGTM10:33
TheMueniemeyer: Moin.10:33
niemeyerfwereade: https://codereview.appspot.com/6248076 is reviewed10:40
fwereadeniemeyer, what's the problem with TestAddRelation?10:41
niemeyerfwereade: Hmm10:44
niemeyerfwereade: Sorry, nevermind..10:44
niemeyerfwereade: I got tricked by codereview showing me as being added in a revision you pushed10:45
fwereadeniemeyer, I can agree that state_test.go is getting a little unwieldy though :)10:45
niemeyerfwereade: If I compare against the base it goes away10:45
niemeyerfwereade: It is indeed10:45
niemeyer 29 For cs:~user/series/mysql10:49
niemeyer 30   cs:~user/mysql10:49
niemeyerfwereade: Shouldn't this say "precise" rather than "series"?10:49
fwereadeniemeyer, oh hell10:49
fwereadesorry10:49
niemeyerfwereade: np10:49
fwereadeniemeyer, I'll propose a trivial in a mo10:49
niemeyerfwereade: Cool.. feel free to propose+submit directly as a trivial10:50
fwereadeniemeyer, ok, will do10:50
fwereadecheers10:50
niemeyerfwereade: Thanks10:50
niemeyerAram: ping11:53
Arampong niemeyer11:55
Aramwhat's up.11:55
hazmatlooks like the ec2 archives are working again12:03
niemeyerAram: Heya12:08
Aramhey12:08
niemeyerAram: That's what I was about to ask too :-)12:08
niemeyerhazmat: Sweet12:09
niemeyerAram: Haven't heard much after the sprint.. how're things moving there?12:09
niemeyerTheMue: You've got a review on https://codereview.appspot.com/6268049/12:09
TheMueniemeyer: Just seen the notification, thanks.12:09
Aramniemeyer: working on mgokeeper right now, haven't really worked *at* it until today, I played with mongo in all kinds of scenarios to understand it better.12:10
niemeyerAram: Sweet12:13
niemeyerAram: As usual, it works best if changes are pushed in small solid increments, rather than as a big chunk12:13
niemeyerchunks12:13
niemeyerAram: No change is too small.. even one-liners are fine if they look sensible12:13
Aramabsolutely, I don't intend to come with just one big commit :).12:13
niemeyerAram: I know.. no body intends, but it often happens nevertheless :-)12:14
niemeyerAram: So it's good to keep it in mind so that breakpoints are more easily detected12:14
Aramyes.12:14
niemeyerAram: I promise we'll actually integrate them, rather than being trashed as might have been seen in some other project ;-D12:15
Aramheh, indeed.12:16
TheMueLunchtime.12:16
Aramenjoy12:17
niemeyerTruesome12:23
niemeyerTheMue: https://codereview.appspot.com/6250076/ is dirty again12:25
niemeyerOMG.. is it lunch time yet... hmmm.. nope, 3h to go.  The body gets pretty confused when I wake up so early.12:38
niemeyerTheMue: You've just reproposed a branch that has already been submitted: https://codereview.appspot.com/6120045/12:49
niemeyerTheMue: Probably because of the vague branch name ("go")12:49
hazmatniemeyer, i always  listen to the body in those situations12:50
TheMueniemeyer: Yes, just wondered about the message in the notification too. Strange,12:50
niemeyerhazmat: Hehe, wise :)12:50
TheMueniemeyer: Check again, should have been the cleaning of the RemoveRelation() branch.12:50
niemeyerTheMue: You've reproposed the same branch.. nothing really surprising I guess12:50
niemeyerTheMue: Nothing up for review12:51
TheMueniemeyer: No, please ignore it, has been my fault.12:51
TheMueniemeyer: The right one will come in a few moments.12:52
niemeyerTheMue: Sounds good, thanks12:52
TheMueniemeyer: Ah, looks cleaner now.12:56
niemeyerTheMue: Super, thanks.. I'll just finish the subordinate units branch from fwereade and then will go back to yours12:56
TheMueniemeyer: Thx,12:57
niemeyerfwereade: https://codereview.appspot.com/6268050/ looks great.. just sent a suggestion for the API13:17
niemeyerfwereade: Please let me know how you feel abou tit13:17
niemeyerabout it13:17
fwereadeniemeyer, thanks, will do13:17
niemeyerfwereade: Cheers13:17
niemeyerAlso, if someone is in the mood for a quick review: https://codereview.appspot.com/6295048/13:17
niemeyerTheMue: Reviewed13:32
TheMueniemeyer: Great, thx.13:32
niemeyerTheMue: np13:32
TheMueniemeyer: I'm just writing down some thoughts about error creation and handling.13:33
niemeyerTheMue: Awesome, looking forward to seeing it13:33
TheMueniemeyer: Will send it to our list for discussion.13:34
niemeyerTheMue: Brilliant13:34
niemeyerTheMue, fwereade: We're *almost* zeroing out on branches up for review.. this is probably a close to a good time to do the switch over to the juju-core project in Launchpad13:41
TheMueniemeyer: Sounds good13:42
fwereadeniemeyer, there's never going to be a good time; so yeah, let's do it now ;p13:43
niemeyerCool, I'll do the whole dance.. there's still time to merge these last few branches, though13:44
niemeyerAfter you're done with that, we can settle on a no-more-merges approach13:44
TheMueHooray, got my list of branches with prerequisites in. The current open and the new started are directly based on trunk. Phew.13:45
niemeyerTheMue: Awesome.. unfortunately there will be some extra pain just now, as you'll have to rebase the branch on the new tree13:46
niemeyerTheMue: But that shouldn't be too much touble hopefully13:46
niemeyertrouble13:46
TheMueniemeyer: Think so too.13:46
niemeyerTheMue: Ok, so all of your impending branches are in?13:48
TheMueniemeyer: There's one proposal open, the new branch is in a very early state and not yet proposed.13:49
niemeyerTheMue: Okay, can you please get the open proposal in then13:49
niemeyerTheMue: So I can do the switch in a less painful way for you13:49
fwereadeniemeyer, one thing about the formatting I just thought of13:49
fwereadeniemeyer, the overridden format_smart methods don't produce yaml13:50
niemeyerfwereade: Ditto for you.. there's at least one almost-ready-to-merge branch.. if you can get that in now, I'll do the switch over shortly afterwards13:50
niemeyerfwereade: What does that mean in practice?13:50
TheMueniemeyer: Directly submit it?13:50
niemeyerTheMue: How do you mean?13:50
niemeyerTheMue: What's the branch?13:50
fwereadeniemeyer, they print individual list elements directly13:51
TheMueniemeyer: It's the changed service/relation mapping in topology.13:51
niemeyerfwereade: Well, that sounds like an issue.. it should be yaml as well13:51
fwereadeniemeyer, this is not a problem if we're not saying that "smart formatting implies yaml"13:51
fwereadeniemeyer, indeed so :)13:51
niemeyerTheMue: That's reviewed already13:51
niemeyerfwereade: There's no such thing as "smart formatting" as far the API towards the user goes13:52
niemeyerfwereade: That smart formatting is a pretty dumb idea, if you pardon the pun13:52
TheMueniemeyer: You had two remarks, not yet a LGTM. But both a simple.13:52
fwereadeniemeyer, there's "--format=smart", it just happens to be a default13:52
TheMues/a/are/13:52
fwereadeniemeyer, yeah, agreed13:52
niemeyerTheMue: Okay, so the comment I was making is for us to focus on getting these branches in13:52
niemeyerTheMue: If you address the points, I'll provide a timely review, and you can submit13:53
TheMueniemeyer: OK, come in in a few moments.13:53
niemeyerfwereade: We want yaml for the output.. if it prints anything else such as a Python object or whatever, it is a bug13:53
niemeyerfwereade: smart was an internal implementation that leaked, in a horrible way unfortunately13:54
fwereadeniemeyer, heh, such is life... but that then means that fixing it will break more than we anticipated... ie relation-ids and relation-list :/13:56
niemeyerfwereade: Hmm.. how do you mean?13:56
fwereadeniemeyer, they're the ones that output strainght \n-separated lists atm13:56
niemeyerfwereade: Oh, I think those are fine13:57
fwereadeniemeyer, ok, but they're not outputting yaml; if this is an inconsistency we can live with them I'm ok with that13:57
niemeyerfwereade: Their values can't really contain anything but a list of identifiers separated by newlines13:57
niemeyerfwereade: Which is a handy API for shell scripts to deal with13:58
fwereadeniemeyer, agreed, but not providing a yaml option across the board seems a little weak13:59
niemeyerfwereade: Or do I miss some more complex case that we have?13:59
fwereadeniemeyer, we could just add an explicit --yaml option, which should be trivial13:59
niemeyerfwereade: We can, but I suggest not worrying about this for now14:00
niemeyerfwereade: The shell use case is so strong that it justifies the inconsistency, and doing s.split("\n") is trivial on any language14:01
fwereadeniemeyer, I think that the complete implementation is "def format_json(self, result, stream):\n    print >>stream, yaml.safe_dump(result)"14:01
TheMueniemeyer: It's in for review, and sorry, has been the validation improvement.14:01
fwereadeniemeyer, but anyway: I like the AddUnitSubordinateTo(principal) idea, and I'm working on that now14:02
niemeyerfwereade: Okay, I'm happy with adding that if you'd prefer to have it now.. --output yaml?14:03
niemeyerfwereade: Is that the syntax we have everywhere else?14:03
fwereadeniemeyer, "--format yaml"14:03
niemeyerfwereade: Ah, cool.. I'm happy with whatever we're using elsewhere14:03
niemeyerandrewsmedina: Is Francisco Souza with you guys as well?14:07
andrewsmedinaniemeyer: yes14:09
niemeyerandrewsmedina: Cool.. just found an unanswered mail from him on the list re. CentOS and was wondering about it14:10
niemeyerWill provide some feedback14:10
andrewsmedinaniemeyer: he is my co-worker and my friend14:10
andrewsmedinaniemeyer: we ported juju to work on centos14:11
niemeyerandrewsmedina: This is awesome14:11
andrewsmedinaniemeyer: this are working fine now14:11
niemeyerandrewsmedina: Really? Ho ho.. this is cool14:12
andrewsmedinaniemeyer: we need thinkin how to provide to juju this compatibility without use a fork14:12
andrewsmedinaniemeyer: if juju support a custom cloud init14:13
niemeyerandrewsmedina: Sounds good.. I'll provide some feedback on the list and we can move from there14:15
niemeyerfsouza: Timely ;)14:17
fsouzaniemeyer: andrewsmedina summoned me14:20
niemeyerfsouza, andrewsmedina: Mail sent14:22
fsouzaniemeyer: great, I hope to give it back to you soon :)14:37
fwereadeniemeyer, https://codereview.appspot.com/6268050/14:43
niemeyerfwereade: Looking14:45
fwereadeniemeyer, cheers14:47
niemeyerfwereade: Done14:48
niemeyerfwereade: I think you got the last bit of the comment reversed14:48
niemeyerfwereade: LGTM with that addressed14:48
niemeyerfwereade: That bit, specifically: "Adding a non-subordinate unit on a subordinate service or the other way around should fail in a nice way, though. People may do that with the UI (add-unit foo)."14:49
fwereadeniemeyer, surely it's impossible to add a principal unit as a subordinate via the UI?14:50
niemeyerfwereade: juju add-unit subordinate-service14:50
fwereadeniemeyer, add-unit will call AddUnit, surely?14:50
fwereadeniemeyer, AddUnitSubordinateTo should only be ever called by us as a side-effect of addition of some other unit14:51
fwereadeniemeyer, and we should know what we're doing from the POV of program logic alone14:51
niemeyerfwereade: You're right, but the case that is panicking is simply the counterpart of that14:52
niemeyerfwereade: Which can plausibly happen as a valid user interaction of some kind14:52
fwereadeniemeyer, there's no panic in AddUnit14:53
niemeyerfwereade: The case that is panicking in AddUnitSubordinateTo is the counterpart of a case that errors in AddUnit14:53
niemeyer 109         if ch.Meta().Subordinate {14:53
niemeyer 110                 return nil, fmt.Errorf("cannot directly add units to subordinate14:53
niemeyer      service %q", s.name)14:53
niemeyer 111         }14:53
niemeyer 122         if !ch.Meta().Subordinate {14:53
niemeyer 123                 panic("cannot make a principal unit subordinate to another unit"14:53
niemeyer     )14:53
niemeyerThis is a sensible panic:14:55
niemeyer 125         if principal == nil {14:55
niemeyer 126                 panic("a subordinate unit must be added to a principal unit")14:55
niemeyer 127         }14:55
niemeyerEverything else there should be an error14:55
fwereadeniemeyer, I still don't see how I can panic unless the state is already screwed up badly enough that we started a subordinate relation between the wrong kinds of services14:56
niemeyerfwereade: A panic is a serious logical mistake, or system problem14:56
niemeyerfwereade: Having two valid units at hand, and doing an operation that is not supported for them, is an error, not a reason to blow the process up14:57
fwereadeniemeyer, fair enough... I still consider the above to represent a serious logical error that can only result from other code acting flat-out wrong; but I can see there's a distinction there14:58
niemeyerfwereade: My proof that this isn't the case is that you correctly spotted that panicking on AddUnit would be bad14:58
fwereadeniemeyer, ha, yeah14:59
fwereadeniemeyer, a thought; is there any point panicing explicitly in the first place? the next operation is to call principal.IsPrincipal() which will panic on nil anyway15:03
niemeyerfwereade: I'd be fine with dropping the explicit check15:03
jimbakerfwereade, thanks for the review, some great refactoring comments!15:05
fwereadejimbaker, cheers, hope I haven't missed some subtlety somewhere ;)15:06
jimbakerfwereade, the only subtlety, if we will call that, is just the preservation of the json serialization. i'm just trying to preserve any existing warts, since that's the point of adding format: 215:09
jimbakeralso i do agree with format_yaml, it's too bad white space (including \n) in yaml just simply is a white space separated string and doesn't imply a list15:11
fwereadejimbaker, I'm pretty sure that yaml can serialize anything json can; so if we're using it internally, why not go with the more expressive format that is often needed anyway, and dispense with all the extra logic in the protocol?15:12
fwereadeniemeyer, that's in now15:13
niemeyerfwereade: Sweet, thanks15:13
niemeyerfwereade: Would you mind to have a quick look at https://codereview.appspot.com/6295048/?15:13
niemeyerfwereade: Should be a trivial15:14
fwereadeniemeyer, on it15:14
fwereadeniemeyer, LGTM15:14
niemeyerfwereade: Cheers!15:16
niemeyerOh, TheMue had already reviewed it too15:16
niemeyerCool15:16
jimbakerfwereade, yaml can serialize anything json can do. but again there are subtle changes that the json path introduces that using charms can observe and therefore depend on. in particular, the u"foobar" stuff in output15:17
fwereadejimbaker, where does it hit output? all the communications stuff in protocol.py is 100% internal, isn't it?15:18
jimbakerfwereade, sure, but it's in the communication that the conversion occurs15:18
fwereadejimbaker, that is, everything we serialize to return is immediately deserialized atthe other end before being converted to the final format15:18
jimbakerfwereade, so info is being added that is unexpected, and then leaks out15:19
niemeyerTheMue: LGTM on https://codereview.appspot.com/6268049/15:19
niemeyerWill do the switch over once that's in, after lunch15:19
TheMueniemeyer: Great, thanks15:19
jimbakerfwereade, previously i didn't maintain two separate code paths, and it was 90% (or whatever pareto limit) consistent with the old format15:20
fwereadejimbaker, what is the problem with converting object=>yaml=>object=>json, compared to object=>json=>object=>json?15:20
niemeyerLunch!15:20
fwereadejimbaker, (ok, the whole idea is silly in the first place, but...)15:21
jimbakerfwereade, it's the fundamental reason why this work was done in the first place15:21
fwereadejimbaker, ah, hold on, I see now15:22
jimbakerso you do relation-get - with format v1 and you get {u"public-address": u"ec2-1-2-3-4.compute-1.amazonaws.com"}15:23
jimbakerfwereade, now it's quite possible to avoid this particular case, there's nothing wrong with unicode per se. but then other things start changing15:24
jimbakerso keeping the two code paths ensures we get old buggy behavior, as was agreed upon15:24
jimbakerand it's the *same* buggy behavior, and the buggy behavior even now has tests :)15:25
fwereadejimbaker, yep, consider those objections withdrawn; I had thought we'd duplicate the bugs, but we don't15:25
fwereadejimbaker, thanks for bearing with me :)15:25
jimbakerfwereade, no worries, and thanks for the good tips again15:26
fwereadejimbaker, a pleasure :)15:26
Aramniemeyer: am I correct in assuming that in mgo the only way to search for things matching a prefix is with a bson.RegEx?15:36
fwereadehazmat, btw, did you have an opinion on https://code.launchpad.net/~julian-edwards/juju/maas-provider-non-mandatory-port/+merge/107577 ? it's been hanging a while16:06
fwereadehazmat, it still LGTM16:07
niemeyerAram: Yeah, but when searching for simple prefixes (/^foo/) MongoDB optimizes that away and uses indexes etc16:13
Aramyes, yes, I know.16:13
niemeyerAram: Cool :)16:13
hazmatfwereade, yeah.. its on my list for today to merge16:14
niemeyerWood is running out.. I'll run out to buy some and be back soon to get trunk transitioned16:19
niemeyerWoohay.. wood is burning, stocks replenished17:17
fwereadeniemeyer, how's the transition going?18:58
niemeyerfwereade: Heya19:04
niemeyerfwereade: Translating the paths now19:05
niemeyerfwereade: Why, do you have something impending?19:05
niemeyerOur tests are getting pretty slow..19:15
niemeyerThe environment ones, mainly19:15
fwereadeniemeyer, not quite, but I have a longish pipeline that's pretty well mapped out in my mind; but then, hmm: tomorrow is a public holiday19:20
niemeyerfwereade: Oh, really? It's a public holiday here as well19:20
niemeyerfwereade: Either way, the transition is done19:20
niemeyerfwereade: lp.net/juju-core/juju is live19:20
niemeyerKilling the old trunk right now19:20
fwereadeniemeyer, and I don't-entirely-depend-on-it-but-boy-howdy-it's-convenient rog's unit node branch19:21
fwereadeniemeyer, that doesn't need to come in for a while19:21
niemeyerfwereade: I'll review it today still19:21
niemeyerfwereade: I didn't bother because rog wasn't around19:21
fwereadeniemeyer, but I should be able to dash off an easy branch or two without it all the same19:21
fwereadeniemeyer, yeah, quite right19:21
niemeyerI'm actually pondering if I should take this rare chance to actually stop for a couple of days19:22
niemeyer(being a national holiday tomorrow and all)19:22
fwereadeniemeyer, sounds sensible :)19:23
mrammniemeyer: rest up now while you still can19:23
mrammand don't burn out!19:23
fwereadetue/thu national holidays should generally be taken advantage of :)19:23
niemeyerfwereade:+1 :-)19:23
niemeyermramm: Yeah, two sprints in a row, and another conf in a couple of weeks.. fun19:24
mrammniemeyer: you have been traveling a lot19:24
niemeyermramm: Hey, but I need the miles.. ;-)19:25
mrammhaha19:25
niemeyerIt's actually a bit sad.. my wife and I have diverging ideas of what holidays should look like.. she wants to go out on some kind of trip to relax, and I want to stay at home and *not travel* for a change19:26
mrammniemeyer: yea, I know how you feel19:27
mrammvacation from traveling is nice19:27
niemeyerIt's been fun, though, honestly.. the amount of progress we've been doing on a daily basis feels so great.19:29
niemeyerAaaand.. I've committed the tombstone on the previous branch.19:29
niemeyerWe're officially moved!19:29
niemeyerA long life to juju-core19:30
* niemeyer mails people19:30
niemeyerfwereade: Forgot to update .lbox.. in case you've branched, please repull in a moment19:38
niemeyerWoohoo!20:16
niemeyerAll Go branches were dealt with, and the move is now done20:17
mrammniemeyer: Awesome!20:18
niemeyerhttps://code.launchpad.net/juju/+activereviews20:19
niemeyerNo Go branches there20:19
niemeyerhttps://code.launchpad.net/juju-core/+activereviews20:20
niemeyerNothing there either!20:20

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