[01:14] * niemeyer pops up [01:37] Okay.. time for some sleep [01:37] Night all [05:02] morning. [06:23] howdy doodie [06:23] putting my head in the lions mouth, http://codereview.appspot.com/6297048/ [06:46] mornings [10:20] TheMue, ping [10:20] fwereade: pong [10:20] TheMue, just thought I'd update you quickly re service/unit relations [10:21] TheMue, I have something that I think looks good [10:21] fwereade: I'm listening. [10:21] TheMue, but it's going to be a fair number of branches, which will roughly go: [10:21] add state.watcher.VersionWatcher [10:22] add state.RelatedUnitWatcher, which watches both the presence node and the settings node of a given unit relation [10:23] add state.RelatedUnitsWatcher, which watches a set of unit relations and maintains a RelatedUnitWatcher for each [10:24] add state.UnitRelation, the tricky bit of which is WatchRelated (which depends on all the foregoing) [10:24] add state.ServiceRelation [10:24] phew [10:24] fwereade: Hehe, good list. [10:24] fwereade: ServiceRelation is already started. [10:25] TheMue, there's a struct without methods, right? [10:25] fwereade: UnitRelation is on the list. [10:25] TheMue, I've basically written all that code [10:26] TheMue, needs far better tests, and to be broken into that nice clean structure [10:26] TheMue, but I wanted to check we weren't duplicating too much [10:26] fwereade: Currently it has the first few accessor methods, but more are planned (matching the Py impl.). [10:26] TheMue, ah yeah, that's right, sorry [10:26] TheMue, that was what I was starting from [10:27] fwereade: No problem, it's not yet started. [10:27] TheMue, cool; just wanted to reassure you that it was coming, and check you didn't need it imminently [10:27] fwereade: I had two smaller branches I'm working at before going on. [10:27] fwereade: So it fits. [10:27] TheMue, cool [10:28] fwereade: One orthogonal issue is the harmonization of error handling in the state code. [10:29] TheMue, I'm +1 on the general concept, but not really sure what we're planning [10:30] TheMue, 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] fwereade: Right now too many low level errors pop up to the user, like errors containing internal keys or ZK messages. [10:31] TheMue, although I'd be willing to lose that so long as we log the errors whenever we transform them [10:32] TheMue, actually, I think I like that idea [10:32] Good morning folks [10:32] niemeyer, heyhey [10:33] fwereade: 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] TheMue, SGTM [10:33] niemeyer: Moin. [10:40] fwereade: https://codereview.appspot.com/6248076 is reviewed [10:41] niemeyer, what's the problem with TestAddRelation? [10:44] fwereade: Hmm [10:44] fwereade: Sorry, nevermind.. [10:45] fwereade: I got tricked by codereview showing me as being added in a revision you pushed [10:45] niemeyer, I can agree that state_test.go is getting a little unwieldy though :) [10:45] fwereade: If I compare against the base it goes away [10:45] fwereade: It is indeed [10:49] 29 For cs:~user/series/mysql [10:49] 30 cs:~user/mysql [10:49] fwereade: Shouldn't this say "precise" rather than "series"? [10:49] niemeyer, oh hell [10:49] sorry [10:49] fwereade: np [10:49] niemeyer, I'll propose a trivial in a mo [10:50] fwereade: Cool.. feel free to propose+submit directly as a trivial [10:50] niemeyer, ok, will do [10:50] cheers [10:50] fwereade: Thanks [11:53] Aram: ping [11:55] pong niemeyer [11:55] what's up. [12:03] looks like the ec2 archives are working again [12:08] Aram: Heya [12:08] hey [12:08] Aram: That's what I was about to ask too :-) [12:09] hazmat: Sweet [12:09] Aram: Haven't heard much after the sprint.. how're things moving there? [12:09] TheMue: You've got a review on https://codereview.appspot.com/6268049/ [12:09] niemeyer: Just seen the notification, thanks. [12:10] niemeyer: 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:13] Aram: Sweet [12:13] Aram: As usual, it works best if changes are pushed in small solid increments, rather than as a big chunk [12:13] chunks [12:13] Aram: No change is too small.. even one-liners are fine if they look sensible [12:13] absolutely, I don't intend to come with just one big commit :). [12:14] Aram: I know.. no body intends, but it often happens nevertheless :-) [12:14] Aram: So it's good to keep it in mind so that breakpoints are more easily detected [12:14] yes. [12:15] Aram: I promise we'll actually integrate them, rather than being trashed as might have been seen in some other project ;-D [12:16] heh, indeed. [12:16] Lunchtime. [12:17] enjoy [12:23] Truesome [12:25] TheMue: https://codereview.appspot.com/6250076/ is dirty again [12:38] OMG.. is it lunch time yet... hmmm.. nope, 3h to go. The body gets pretty confused when I wake up so early. [12:49] TheMue: You've just reproposed a branch that has already been submitted: https://codereview.appspot.com/6120045/ [12:49] TheMue: Probably because of the vague branch name ("go") [12:50] niemeyer, i always listen to the body in those situations [12:50] niemeyer: Yes, just wondered about the message in the notification too. Strange, [12:50] hazmat: Hehe, wise :) [12:50] niemeyer: Check again, should have been the cleaning of the RemoveRelation() branch. [12:50] TheMue: You've reproposed the same branch.. nothing really surprising I guess [12:51] TheMue: Nothing up for review [12:51] niemeyer: No, please ignore it, has been my fault. [12:52] niemeyer: The right one will come in a few moments. [12:52] TheMue: Sounds good, thanks [12:56] niemeyer: Ah, looks cleaner now. [12:56] TheMue: Super, thanks.. I'll just finish the subordinate units branch from fwereade and then will go back to yours [12:57] niemeyer: Thx, [13:17] fwereade: https://codereview.appspot.com/6268050/ looks great.. just sent a suggestion for the API [13:17] fwereade: Please let me know how you feel abou tit [13:17] about it [13:17] niemeyer, thanks, will do [13:17] fwereade: Cheers [13:17] Also, if someone is in the mood for a quick review: https://codereview.appspot.com/6295048/ [13:32] TheMue: Reviewed [13:32] niemeyer: Great, thx. [13:32] TheMue: np [13:33] niemeyer: I'm just writing down some thoughts about error creation and handling. [13:33] TheMue: Awesome, looking forward to seeing it [13:34] niemeyer: Will send it to our list for discussion. [13:34] TheMue: Brilliant [13:41] TheMue, 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 Launchpad [13:42] niemeyer: Sounds good [13:43] niemeyer, there's never going to be a good time; so yeah, let's do it now ;p [13:44] Cool, I'll do the whole dance.. there's still time to merge these last few branches, though [13:44] After you're done with that, we can settle on a no-more-merges approach [13:45] Hooray, got my list of branches with prerequisites in. The current open and the new started are directly based on trunk. Phew. [13:46] TheMue: Awesome.. unfortunately there will be some extra pain just now, as you'll have to rebase the branch on the new tree [13:46] TheMue: But that shouldn't be too much touble hopefully [13:46] trouble [13:46] niemeyer: Think so too. [13:48] TheMue: Ok, so all of your impending branches are in? [13:49] niemeyer: There's one proposal open, the new branch is in a very early state and not yet proposed. [13:49] TheMue: Okay, can you please get the open proposal in then [13:49] TheMue: So I can do the switch in a less painful way for you [13:49] niemeyer, one thing about the formatting I just thought of [13:50] niemeyer, the overridden format_smart methods don't produce yaml [13:50] fwereade: 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 afterwards [13:50] fwereade: What does that mean in practice? [13:50] niemeyer: Directly submit it? [13:50] TheMue: How do you mean? [13:50] TheMue: What's the branch? [13:51] niemeyer, they print individual list elements directly [13:51] niemeyer: It's the changed service/relation mapping in topology. [13:51] fwereade: Well, that sounds like an issue.. it should be yaml as well [13:51] niemeyer, this is not a problem if we're not saying that "smart formatting implies yaml" [13:51] niemeyer, indeed so :) [13:51] TheMue: That's reviewed already [13:52] fwereade: There's no such thing as "smart formatting" as far the API towards the user goes [13:52] fwereade: That smart formatting is a pretty dumb idea, if you pardon the pun [13:52] niemeyer: You had two remarks, not yet a LGTM. But both a simple. [13:52] niemeyer, there's "--format=smart", it just happens to be a default [13:52] s/a/are/ [13:52] niemeyer, yeah, agreed [13:52] TheMue: Okay, so the comment I was making is for us to focus on getting these branches in [13:53] TheMue: If you address the points, I'll provide a timely review, and you can submit [13:53] niemeyer: OK, come in in a few moments. [13:53] fwereade: We want yaml for the output.. if it prints anything else such as a Python object or whatever, it is a bug [13:54] fwereade: smart was an internal implementation that leaked, in a horrible way unfortunately [13:56] niemeyer, 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] fwereade: Hmm.. how do you mean? [13:56] niemeyer, they're the ones that output strainght \n-separated lists atm [13:57] fwereade: Oh, I think those are fine [13:57] niemeyer, ok, but they're not outputting yaml; if this is an inconsistency we can live with them I'm ok with that [13:57] fwereade: Their values can't really contain anything but a list of identifiers separated by newlines [13:58] fwereade: Which is a handy API for shell scripts to deal with [13:59] niemeyer, agreed, but not providing a yaml option across the board seems a little weak [13:59] fwereade: Or do I miss some more complex case that we have? [13:59] niemeyer, we could just add an explicit --yaml option, which should be trivial [14:00] fwereade: We can, but I suggest not worrying about this for now [14:01] fwereade: The shell use case is so strong that it justifies the inconsistency, and doing s.split("\n") is trivial on any language [14:01] niemeyer, I think that the complete implementation is "def format_json(self, result, stream):\n print >>stream, yaml.safe_dump(result)" [14:01] niemeyer: It's in for review, and sorry, has been the validation improvement. [14:02] niemeyer, but anyway: I like the AddUnitSubordinateTo(principal) idea, and I'm working on that now [14:03] fwereade: Okay, I'm happy with adding that if you'd prefer to have it now.. --output yaml? [14:03] fwereade: Is that the syntax we have everywhere else? [14:03] niemeyer, "--format yaml" [14:03] fwereade: Ah, cool.. I'm happy with whatever we're using elsewhere [14:07] andrewsmedina: Is Francisco Souza with you guys as well? [14:09] niemeyer: yes [14:10] andrewsmedina: Cool.. just found an unanswered mail from him on the list re. CentOS and was wondering about it [14:10] Will provide some feedback [14:10] niemeyer: he is my co-worker and my friend [14:11] niemeyer: we ported juju to work on centos [14:11] andrewsmedina: This is awesome [14:11] niemeyer: this are working fine now [14:12] andrewsmedina: Really? Ho ho.. this is cool [14:12] niemeyer: we need thinkin how to provide to juju this compatibility without use a fork [14:13] niemeyer: if juju support a custom cloud init [14:15] andrewsmedina: Sounds good.. I'll provide some feedback on the list and we can move from there [14:17] fsouza: Timely ;) [14:20] niemeyer: andrewsmedina summoned me [14:22] fsouza, andrewsmedina: Mail sent [14:37] niemeyer: great, I hope to give it back to you soon :) [14:43] niemeyer, https://codereview.appspot.com/6268050/ [14:45] fwereade: Looking [14:47] niemeyer, cheers [14:48] fwereade: Done [14:48] fwereade: I think you got the last bit of the comment reversed [14:48] fwereade: LGTM with that addressed [14:49] fwereade: 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:50] niemeyer, surely it's impossible to add a principal unit as a subordinate via the UI? [14:50] fwereade: juju add-unit subordinate-service [14:50] niemeyer, add-unit will call AddUnit, surely? [14:51] niemeyer, AddUnitSubordinateTo should only be ever called by us as a side-effect of addition of some other unit [14:51] niemeyer, and we should know what we're doing from the POV of program logic alone [14:52] fwereade: You're right, but the case that is panicking is simply the counterpart of that [14:52] fwereade: Which can plausibly happen as a valid user interaction of some kind [14:53] niemeyer, there's no panic in AddUnit [14:53] fwereade: The case that is panicking in AddUnitSubordinateTo is the counterpart of a case that errors in AddUnit [14:53] 109 if ch.Meta().Subordinate { [14:53] 110 return nil, fmt.Errorf("cannot directly add units to subordinate [14:53] service %q", s.name) [14:53] 111 } [14:53] 122 if !ch.Meta().Subordinate { [14:53] 123 panic("cannot make a principal unit subordinate to another unit" [14:53] ) [14:55] This is a sensible panic: [14:55] 125 if principal == nil { [14:55] 126 panic("a subordinate unit must be added to a principal unit") [14:55] 127 } [14:55] Everything else there should be an error [14:56] niemeyer, 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 services [14:56] fwereade: A panic is a serious logical mistake, or system problem [14:57] fwereade: 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 up [14:58] niemeyer, 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 there [14:58] fwereade: My proof that this isn't the case is that you correctly spotted that panicking on AddUnit would be bad [14:59] niemeyer, ha, yeah [15:03] niemeyer, 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 anyway [15:03] fwereade: I'd be fine with dropping the explicit check [15:05] fwereade, thanks for the review, some great refactoring comments! [15:06] jimbaker, cheers, hope I haven't missed some subtlety somewhere ;) [15:09] fwereade, 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: 2 [15:11] also 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 list [15:12] jimbaker, 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:13] niemeyer, that's in now [15:13] fwereade: Sweet, thanks [15:13] fwereade: Would you mind to have a quick look at https://codereview.appspot.com/6295048/? [15:14] fwereade: Should be a trivial [15:14] niemeyer, on it [15:14] niemeyer, LGTM [15:16] fwereade: Cheers! [15:16] Oh, TheMue had already reviewed it too [15:16] Cool [15:17] fwereade, 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 output [15:18] jimbaker, where does it hit output? all the communications stuff in protocol.py is 100% internal, isn't it? [15:18] fwereade, sure, but it's in the communication that the conversion occurs [15:18] jimbaker, that is, everything we serialize to return is immediately deserialized atthe other end before being converted to the final format [15:19] fwereade, so info is being added that is unexpected, and then leaks out [15:19] TheMue: LGTM on https://codereview.appspot.com/6268049/ [15:19] Will do the switch over once that's in, after lunch [15:19] niemeyer: Great, thanks [15:20] fwereade, previously i didn't maintain two separate code paths, and it was 90% (or whatever pareto limit) consistent with the old format [15:20] jimbaker, what is the problem with converting object=>yaml=>object=>json, compared to object=>json=>object=>json? [15:20] Lunch! [15:21] jimbaker, (ok, the whole idea is silly in the first place, but...) [15:21] fwereade, it's the fundamental reason why this work was done in the first place [15:22] jimbaker, ah, hold on, I see now [15:23] so you do relation-get - with format v1 and you get {u"public-address": u"ec2-1-2-3-4.compute-1.amazonaws.com"} [15:24] fwereade, now it's quite possible to avoid this particular case, there's nothing wrong with unicode per se. but then other things start changing [15:24] so keeping the two code paths ensures we get old buggy behavior, as was agreed upon [15:25] and it's the *same* buggy behavior, and the buggy behavior even now has tests :) [15:25] jimbaker, yep, consider those objections withdrawn; I had thought we'd duplicate the bugs, but we don't [15:25] jimbaker, thanks for bearing with me :) [15:26] fwereade, no worries, and thanks for the good tips again [15:26] jimbaker, a pleasure :) [15:36] niemeyer: am I correct in assuming that in mgo the only way to search for things matching a prefix is with a bson.RegEx? [16:06] hazmat, 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 while [16:07] hazmat, it still LGTM [16:13] Aram: Yeah, but when searching for simple prefixes (/^foo/) MongoDB optimizes that away and uses indexes etc [16:13] yes, yes, I know. [16:13] Aram: Cool :) [16:14] fwereade, yeah.. its on my list for today to merge [16:19] Wood is running out.. I'll run out to buy some and be back soon to get trunk transitioned [17:17] Woohay.. wood is burning, stocks replenished [18:58] niemeyer, how's the transition going? [19:04] fwereade: Heya [19:05] fwereade: Translating the paths now [19:05] fwereade: Why, do you have something impending? [19:15] Our tests are getting pretty slow.. [19:15] The environment ones, mainly [19:20] niemeyer, not quite, but I have a longish pipeline that's pretty well mapped out in my mind; but then, hmm: tomorrow is a public holiday [19:20] fwereade: Oh, really? It's a public holiday here as well [19:20] fwereade: Either way, the transition is done [19:20] fwereade: lp.net/juju-core/juju is live [19:20] Killing the old trunk right now [19:21] niemeyer, and I don't-entirely-depend-on-it-but-boy-howdy-it's-convenient rog's unit node branch [19:21] niemeyer, that doesn't need to come in for a while [19:21] fwereade: I'll review it today still [19:21] fwereade: I didn't bother because rog wasn't around [19:21] niemeyer, but I should be able to dash off an easy branch or two without it all the same [19:21] niemeyer, yeah, quite right [19:22] I'm actually pondering if I should take this rare chance to actually stop for a couple of days [19:22] (being a national holiday tomorrow and all) [19:23] niemeyer, sounds sensible :) [19:23] niemeyer: rest up now while you still can [19:23] and don't burn out! [19:23] tue/thu national holidays should generally be taken advantage of :) [19:23] fwereade:+1 :-) [19:24] mramm: Yeah, two sprints in a row, and another conf in a couple of weeks.. fun [19:24] niemeyer: you have been traveling a lot [19:25] mramm: Hey, but I need the miles.. ;-) [19:25] haha [19:26] It'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 change [19:27] niemeyer: yea, I know how you feel [19:27] vacation from traveling is nice [19:29] It's been fun, though, honestly.. the amount of progress we've been doing on a daily basis feels so great. [19:29] Aaaand.. I've committed the tombstone on the previous branch. [19:29] We're officially moved! [19:30] A long life to juju-core [19:30] * niemeyer mails people [19:38] fwereade: Forgot to update .lbox.. in case you've branched, please repull in a moment [20:16] Woohoo! [20:17] All Go branches were dealt with, and the move is now done [20:18] niemeyer: Awesome! [20:19] https://code.launchpad.net/juju/+activereviews [20:19] No Go branches there [20:20] https://code.launchpad.net/juju-core/+activereviews [20:20] Nothing there either!