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

=== flaviamissi_ is now known as flaviamissi
* hazmat wonders if he needs to send a mother may i email to the list03:13
fwereaderog, hopefully trivial: https://codereview.appspot.com/578605110:52
fwereaderog, and, blast, I just realised you've got that gozk review still up (right?)10:53
fwereaderog, I checked for *juju* reviews and completely forgot the rest  :/10:53
rogfwereade: yeah, i've been waiting for almost 2 weeks now. sigh.10:53
fwereaderog, I know the feeling10:54
fwereaderog, I think I've passed some sort of milestone now, though: I have more go branches stacked up than I do python ones10:54
rogfwereade: :-)10:54
rogfwereade: i wonder if instead of a new package, whether we might expand the jujutest package a bit to add testing code.10:55
rogfwereade: it could be moved out of environs, and maybe just renamed "test", perhaps.10:56
fwereaderog, doesn't sound like a bad idea, I could follow up with a branch that does that perhaps?10:57
fwereaderog, for some reason I expect things called "test" to contain actual tests10:58
fwereaderog, but I'm not seriously opposed to that name10:58
rogfwereade: given that testutil is new, why not add ZkSetUpEnvironment to jujutest now?10:58
rogfwereade: we could call it "testing"10:58
rogfwereade: i know it matches the name of the std testing package, but i don't think that's a problem10:59
rogimport "launchpad.net/juju/go/testing" sounds reasonable to me10:59
rogfwereade: it's a pity that zookeeper won't work unless it makes its own directory. then ZkSetUpEnvironment could just return a zk.Server.11:02
TheMuerog:  +111:02
rogTheMue: nothing we can do about that though...11:03
TheMuerog: the testing package or the directory part?11:04
rogTheMue: the directory part. were you referring to the testing package?11:04
fwereaderog, so we'd do `import jtesting "launchpad.net/juju/go/testing"` or something?11:04
TheMuerog: yep11:05
rogfwereade: yeah, but the identifier would only be necessary if you were already importing "testing". (probably true most of the time in the zk scenarion)11:05
rogi quite like "scenarion"11:05
TheMuerog, fwereade : as long as our functions in the package have a clean naming it also could be imported with _.11:06
rogTheMue: huh? surely we can't if we want to use it...11:07
rog?11:07
TheMuerog: eh, i meant . like with gocheck11:08
rogTheMue: no no no! no more import "."s! gocheck is one too many as it is!11:09
TheMuerog: what troubles do you have with it?11:09
rogTheMue: namespace pollution11:09
rogTheMue: why don't we import many things with "."?11:10
rogTheMue: answer: because it's nice to see a name and know where it comes from.11:10
rogTheMue: import "." is only there because of it was conventional to write tests as if they were part of the same package as they're testing.11:11
rogTheMue: and sometimes there's a cyclic dependency11:11
TheMuerog:  definitely, but then we need a different package naming then testing. otherwise aliases also can difer over different sources and you get a bad maintenance aspect11:11
rogTheMue: but IMHO it should never be used11:11
rogTheMue: i don't think that's a problem.11:11
rogTheMue: the compiler knows and will tell you very quickly11:12
rogTheMue: we already have two packages with the identifier "ec2" for example11:12
rogTheMue: and it's not a problem.11:12
TheMuerog: i don't talk about the compiler, i talk about people who maintain different sources where the same testing package is imported with different aliases11:12
TheMuerog: and i our tests we would need to import both, so one always has to be aliased11:13
rogTheMue: it's not uncommon to rename a package with a different alias when you import it11:13
rogTheMue: if we have a standard alias in that case, i don't think there's a problem11:13
rogTheMue: for example, when i import "launchpad.net/goamz/ec2" and "launchpad.net/juju/go/environs/ec2", i name the former "amzec2".11:14
rogTheMue: similarly we can import "juju/go/testing" as jujutesting when there's a clash.11:14
TheMuerog: and i talk about this "standard". i've seen enough projects where standards changed with the changing team members. while we are few and fresh there's no prob, but in longer terms11:14
rogTheMue: it doesn't matter too much. it's a variable name - we have preferences but not strict rules.11:15
TheMuerog: that's one of the problems11:16
TheMuerog: it will differ over time11:16
rogTheMue: the package identifier?11:17
TheMuerog: and new code maintainers will stumble about it11:17
TheMuerog:the aliases for the same package in different tests11:17
rogTheMue: i don't think it'll cause more than a moment's pause.11:18
rogTheMue: most of our packages don't use the standard "testing" package.11:18
TheMuerog: the tests do, and they often will import both, the standard and our testiing package11:19
rogTheMue: and it can't give rise to any more problems than a failed compile AFAICS11:19
TheMuerog: the problem is not the compiler run, the problem are the 70% maintenance part of software costs when new people in a few years sit in front of the software and try to understand it11:20
rogTheMue: i don't see that as a problem in this case - a quick glance at the top of the file will immediately resolve any queries11:20
TheMuerog: this look here, that look there, another look at a different time - that sums11:21
TheMuerog: just again, 70% of software costs are maintenance costs11:23
rogTheMue: also, which one you're using will be obvious from the context.11:23
rogTheMue: remember, you were suggesting importing as ".". that's much worse!11:23
TheMuerog: yep, i've got to admit that this has been a bad idea. a reason why i wonna look for a better one11:24
rogTheMue: if i see "testing.Something" i know that it's got to be one of only two packages with that identifier. and the Something will be something juju-related or not.11:25
rogTheMue: all that said, have you got another suggestion for a name?11:26
TheMuerog: not yet, im trying to find one.11:26
TheMuerog: hmm, opposit to testing or gocheck it doesn't provide a testing itself11:29
rogTheMue: no, but it's about testing juju, and that seems ok to me11:29
TheMuerog: it's more that it will contain tools for testing juju11:29
rogTheMue: the other suggestion was "test", which might be ok too11:29
rogTheMue: as it does contain some tests as well as utils11:30
TheMuerog: my ideas are testtools or testenv so far11:30
rogTheMue: it's more than just tools - it has tests. and it's more than testing the environment.11:31
TheMuerog: shall it DO tests or only SUPPORT tests? and i didn't meant to test the environment but to provide an environment for juju testing11:32
rogTheMue: currently it *does* tests. i'm suggesting that it be a catch-all package for common stuff to do with testing juju, including tests and test tools11:33
TheMuerog: how does it relate to the tests inside the packages?11:34
TheMuerog: i would like a package providing the environment and tools but to keep the individual tests inside the packages11:34
rogTheMue: it currently implements provider-independent tests to test an Environ11:34
rogTheMue: the whole point is that those tests are independent of a particular provider, so they can't live in a single package.11:35
rogfwereade: i've just discovered that the current zk behaviour (can't create a server on an existing directory) is entirely my fault. if i fix that, then we don't need a TearDownEnvironment, just srv.Destroy.11:36
TheMuerog: don't the provider packages have a superior pckage for those tests?11:37
rogTheMue: sorry, i don't understand.11:37
fwereaderog, ah, cool11:38
TheMuerog: a package one level above.11:38
rogTheMue: do you mean launchpad.net/juju/go/environs ?11:39
TheMuerog: yes11:39
rogTheMue: i don't want to pollute its exported namespace with testing code11:40
TheMuerog: won't that testing package be exported too?11:40
rogTheMue: hence i use a different package (currently launchpad.net/juju/go/environs/jujutest)11:40
rogTheMue: yes, but that testing package is *just* about testing.11:40
rogTheMue: i don't want to mix testing code and production code - it'll just confuse people.11:41
TheMuerog: yep, that's right11:42
TheMuerog: so far in my case the testing stuff is invisible11:42
rogTheMue: so i was proposing to fwereade that the jujutest package becomes launchpad.net/juju/go/testing11:43
rogTheMue: because i'm not sure we need a proliferation of packages to do with juju testing.11:44
TheMuerog: the movement makes sense, still i've got a naming problem11:44
rogTheMue: ok, well perhaps we'll keep the existing name ("jujutest") for the time being, pending discussion.11:44
TheMuerog:it's that the standard "testing" is a test framework/library while our "testing" would be tests11:45
TheMuerog: "juju" would be redundant there, how simply "tests"11:45
rogi kind of agree with fwereade's earlier comment:11:46
rog[10:58] <fwereade> rog, for some reason I expect things called "test" to contain actual tests11:46
rogTheMue: i don't mind that we're not implementing a testing framework. it's still all about testing, and that's the main thing IMHO.11:46
fwereaderog, the thing is I do see the proposal as creating the very rudimentary beginnings of a testing framework11:47
rogfwereade: +111:47
TheMuefwereade: in this case we should not put this framework and the tests themself into one package11:48
fwereaderog, ok, running a zookeeper server isn't much, but I also expect that we'll want to make it easy to get access to a temporary charm repo we can write to without messing with source control for example11:49
fwereaderog, my perspective is that the proposal moves a small step in a good direction that I can then use to start testing hook contexts11:50
rogTheMue: i don't mind too much if they're in the same package. it's not going to be a large package, and it has reasonable conceptual integrity IMHO11:59
rogfwereade: i'm not entirely sure about the ZkTestingT etc functions. i think we can go simpler without sacrificing much convenience. how about this? http://paste.ubuntu.com/874476/12:09
rogfwereade: i'm assuming a couple of changes to the zk package12:09
rogfwereade: 1) that the server type gets an Addr function that can be used to find out an address for the server12:09
rogfwereade: 2) that the server can be created on an existing directory, as long as it's empty12:10
rogfwereade: then we can potentially combine other similar utils, rather than relying on a single thing to call TestingT for us with the right resources.12:11
rogfwereade: here's another idea, that saves us the error checking each time: http://paste.ubuntu.com/874481/12:13
rogfwereade: (TestingT is implemented both by *testing.T and *gocheck.C)12:13
* hazmat yawns12:50
fwereaderog,sorry, Iwasat lunch, give me a sec to catch up13:14
fwereaderog, tbh I'm entirely unattached to any specific interface13:15
fwereaderog, what I suggested I suggested purely because it looked like it'd work both for state and for my needs13:16
wrtpfwereade: cool. what do you think of, say, my last suggestion?13:21
fwereadewrtp, that looks fine to me13:22
fwereadewrtp, I like the idea of having Addr on Server too13:22
wrtpfwereade: good.13:23
fwereadewrtp, btw, re the zk locking:13:23
fwereadewrtp, I'm comfortable with tests that can theoretically fail under enough load13:23
fwereadewrtp, but that's kinda hard to quantify13:24
wrtpfwereade: yeah, i kind of am too, but it's that creeping "i'm adding just that little bit more flakiness" feeling that concerns me13:24
fwereadewrtp, for example, how low does the tiemout have to be on your machine before you start to see semi-regular failures?13:24
wrtpfwereade: i don't know. wait until we're running the tests on ARM :-)13:25
fwereadewrtp, heh13:25
wrtpniemeyer: yo!13:25
fwereadewrtp, ok, what if you run them 100 times now?13:26
niemeyerGood morning jujuers!13:26
fwereadeniemeyer, heyhey13:26
niemeyerwrtp: Heya13:26
niemeyerfwereade: Alow13:26
fwereadewrtp, what would be the expected failure count?13:26
wrtpfwereade: i'll have a try. am just writing a test for CreateServer currently.13:26
wrtpniemeyer: after discussion this morning, i'm changing zookeeper.CreateServer so that it will succeed if the server directory exists and is empty.13:27
wrtpniemeyer: which means that Server.Destroy is sufficient to delete the tmp directory.13:27
niemeyerwrtp: Hmm.. why is it necessary?13:27
niemeyer(awww... charmstore build broke in the PPA)13:28
wrtpniemeyer: darn. hope you get good logs...13:28
wrtpniemeyer: because fwereade proposed this: https://codereview.appspot.com/5786051/diff/1/testutil/zk.go13:28
wrtpniemeyer: and i thought the complexity is unecessary13:28
wrtpniemeyer: it seems silly that you can create a temp directory and run a zookeeper server in it.13:29
wrtps/can/can't/13:29
wrtpniemeyer: so this was my counter-suggestion: http://paste.ubuntu.com/874481/13:29
wrtpniemeyer: assuming also an Addr method on Server.13:30
niemeyerwrtp: Looks good. Don't we have Addr?13:30
wrtpniemeyer: nope, i don't think so.13:31
wrtpniemeyer: it would just return 127.0.0.1:port13:31
niemeyerwrtp: Yeah, it sounds fine.. how do we even connect to the test server nowadays then/13:32
niemeyer?13:32
wrtpniemeyer: we just assume localhost13:32
wrtpniemeyer: and we specify the port, so we know the address.13:32
niemeyerwrtp: Ah, gotcha13:32
niemeyerwrtp: Yep, +1 on Addr too13:32
wrtpniemeyer: there was also a discussion about where to put this kind of helper function.13:32
wrtpniemeyer: i wondered if it might make sense to make environs/jujutest more general.13:33
wrtpniemeyer: and perhaps rename it to launchpad.net/juju/go/testing13:33
wrtpniemeyer: TheMue wasn't so happy about that name though, because of the clash with the standard testing package13:33
hazmatniemeyer, http://wtf.labix.org has been mia for a few weeks.13:35
hazmatniemeyer, also the enhanced relation support spec is ready for a review (its been revised and scoped appropriately)13:36
hazmatniemeyer, and g'morning13:36
niemeyerhazmat: Morning :-)13:36
niemeyerhazmat: Will check it out13:36
niemeyers/it/them13:36
niemeyerwrtp: Hmm13:37
niemeyerwrtp: jujutest doesn't feel entirely generic13:38
wrtpniemeyer: the name or the stuff in it?13:38
niemeyerwrtp: The stuff in it13:39
wrtpniemeyer: it's only two types. if renamed EnvironTests and EnvironLiveTests, i think they'd live quite happily alongside other more generic testing-related functions13:40
fwereadewrtp, I guess the question is, how many other packages will be using them?13:42
wrtpfwereade: all the environs implementations. that's it, probably.13:42
fwereadewrtp, that does suggest to me that it might live more happily under environs tbh13:42
niemeyerwrtp: Maybe rename go/environs/jujutest as go/environs/envtest, and create a new go/jujutest/?13:43
wrtpniemeyer: yeah, that would work, although i quite like go/testing as a name13:45
niemeyerwrtp: envtesting and testing then?13:45
wrtpniemeyer: sounds good13:45
niemeyerwrtp: I don't mind "testing".. we don't use the stock testing package except for hooking gocheck in anyway13:45
wrtpniemeyer: i agree.13:46
wrtpniemeyer: although, to play devil's advocate, the function to create a new zk server will be used in a context where both "testing" packages are imported.13:47
wrtpniemeyer: i'm happy to import "launchpad.net/juju/go/testing" as gotesting, or "testing" as stdtesting though.13:48
niemeyerwrtp: The latter, please13:48
wrtpniemeyer: yeah. internal gets preference.13:48
niemeyerwrtp: Yeah13:49
wrtpfwereade: do you want to mutate your CL accordingly while i'll make the changes to zk.13:50
wrtps/\./?/13:50
wrtpfwereade: (assuming it sounds good to you, of course)13:51
fwereadewrtp, sure, sounds great13:51
fwereadewrtp, except, sorry, I'm slow13:55
fwereadewrtp, we surely don't actually want to leave the temp dir hanging around, do we?13:55
wrtpfwereade: srv.Destroy removes the zk directory and its contents.13:56
fwereadewrtp, doh sorry13:56
wrtpnp13:56
niemeyerOMG..14:30
niemeyerThe charmstore package is biult14:30
niemeyerbuilt14:30
niemeyerWoohay!14:34
niemeyer"You can now launch 64-bit operating systems on the m1.small and c1.medium instances."14:34
wrtpniemeyer: congrats!14:48
niemeyerwrtp: 8)14:49
niemeyerwrtp: Thanks14:49
fwereadeniemeyer, respectively: cool!; grar, constraints change14:49
wrtpfwereade: i was going to make the zk.CreateServer changes independent of the safe-close branch, but the safe-close branch also did all the changes for the gocheck changes, so i think i'll have to wait until that's ready.14:50
wrtpfwereade: sorry.14:50
fwereadewrtp, no worries, I'll just duplicate the setup stuff for now, it's not too much to carry for a limited time14:50
wrtpfwereade: sounds good14:51
fwereadewrtp, although one note I decided not to bother with was "shouldn't the gocheck changes be independent?"14:51
wrtpfwereade: yes, they should.14:51
niemeyerGetting no-deps binaries out of "dpkg -L <go app>" never gets old14:51
wrtpfwereade: but it takes so long to turn around one branch, i thought it better to do them in the same one. bitten!14:52
fwereadewrtp, yeah, it's a self-reinforcing problem14:52
niemeyerrobbiew: ping14:52
wrtpniemeyer: i don't understand one bit of that remark :-)14:52
fwereadewrtp, slow turnaround -> bigger diffs -> yet slower turnaround14:52
niemeyerwrtp: This command lists the files in a package14:53
wrtpfwereade: yeah. well, the diffs were fairly small for the gocheck changes, hence my merging.14:53
fwereadewrtp, indeed, not an irrational response14:54
wrtpniemeyer: still in the dark "Getting no-deps binaries"? you don't need to explain, BTW :-)14:55
niemeyerwrtp: binaries without dependencies14:55
wrtpah, i understand now. no dynamic library dependencies...14:57
niemeyerwrtp: No additional package dependencies, at least.. it does link against some dynamic libraries14:59
wrtpniemeyer: can you tell that from the output of dpkg -L ?15:00
* wrtp is not versed in the lore of dpkg and friends :-)15:01
robbiewniemeyer: pong15:03
niemeyerrobbiew: Heya15:06
niemeyerrobbiew: I could use that RT push :)15:06
robbiew:)15:06
robbiewniemeyer: ticket #?15:06
niemeyerwrtp: I can tell there are only two binaries15:06
niemeyerrobbiew: 51351.. a pretty number!15:07
robbiewlol15:07
robbiewack15:07
niemeyerrobbiew: I've contacted #is first, but given the silence filed this RT yesterday with the IRC questioning15:07
niemeyerrobbiew: and then enhanced to include more details such as the PPA, package name, etc15:07
niemeyerrobbiew: It'd be great if we managed to find someone in IS that would be indeed interested on mongodb15:08
niemeyerrobbiew: But I suspect it won't be easy, given previous conversations with James15:09
robbiewheh...my powers of influence only expand but so far15:09
niemeyerrobbiew: Yeah, not a matter of power in this case.. I was only expressing a desire to find someone there that would be interested, which of course can't be forced in any way15:11
robbiewright15:11
* robbiew will begin his Jedi mind trick training next week :P15:12
niemeyerrobbiew: LOL15:13
niemeyerand that's lunch time15:43
TheMueSo, agent with watcher is ready for review: https://codereview.appspot.com/578205316:09
niemeyerTheMue: WOOHAY16:38
niemeyerI'm now actually blocked on the store since I depend on IS, so I'm moving back onto reviewing mode16:39
wrtpTheMue: i'm having a look now16:40
wrtpniemeyer: any chance you could start with the oldest ones? :-)16:40
niemeyerwrtp: Sounds like  a plan ;)16:42
niemeyerwrtp: Do you have a preference of ordering to make better use of your remaining time today?16:43
TheMuewrtp: *lol*16:43
wrtpniemeyer: jeeze. let me think.16:43
wrtpniemeyer: go-ec2-robustness is the major one that i'd love to see16:43
niemeyerfwereade_: Btw, can you help me out a bit by providing a map for your branches?16:45
niemeyerfwereade_: I'd prefer to review the tip and then ask you to repropose the ones that have pre-req once the pre-req is merged, so that we have a clean diff to work on16:46
fwereade_niemeyer, go-add-cmd-context and go-add-hook-package are the independent ones16:46
niemeyerfwereade_: Ok.. would you mind to put the rest of them as Work In Progress at https://code.launchpad.net/juju/+activereviews until their diff in Rietveld is clean for review?16:47
fwereade_niemeyer, and kinda go-testutil, but that depends on gozk changes which themselves depend on other gozk changes I think16:47
fwereade_niemeyer, sounds good16:47
niemeyerfwereade_: That's fine.. go-testutil can stay since, even though it has dependencies, they are not polluting the diff itself, right?16:47
fwereade_niemeyer, they're not polluting the diff but they are preventing it fromcompiling, I think it's better to WIP that as well16:48
niemeyerfwereade_: It's fine to keep it in review then, IMO.. but please just don't merge while the pre-req isn't there16:48
fwereade_niemeyer, ok,sounds good :)16:48
wrtpfwereade_: heads up: the zk.Server.Addr method returns (string, error) not just string. just so you know.16:51
fwereade_wrtp, cool, thanks16:51
fwereade_TheMue, btw, I'm a bit concerned that the agent state branch overlaps uncomfortably with the presence-nodes work I was doing a couple of weeks ago16:55
TheMuefwereade_: in which way?16:55
fwereade_TheMue, AIUI the intent was to move away from using ephemeral nodes to signal presence and use presence nodes instead16:56
TheMuefwereade_: didn't know that, sorry. so far i'm doing an almost 1:1 port of the py code.16:57
fwereade_TheMue, I haven't looked too closely at everything in that branch yet so I'mnot sure how widespread the impact will be16:58
fwereade_TheMue, hopefully it will at least make your code a bit smaller16:58
fwereade_TheMue, but I think we've hit a bit of a communications snafu16:58
fwereade_niemeyer, ^^16:58
TheMuefwereade_: do you have something written about it, to help me understanding your idea?16:58
fwereade_TheMue, the CL is here: https://codereview.appspot.com/5672067/16:59
fwereade_TheMue, the description is a little terse but a quick scan of the inline docs should be helpful17:00
TheMuefwereade_: will take a look17:00
fwereade_TheMue, cheers17:00
wrtpTheMue, fwereade_: i think you're right. it seems it was an unfortunate place to start... :-)17:02
niemeyerfwereade_, TheMue: Agreed17:02
niemeyerTheMue: Please check out the IRC logs17:02
wrtpniemeyer: are there any logs of this channel?17:02
niemeyerwrtp, TheMue: http://irclogs.ubuntu.com/2012/03/08/%23juju-dev.html17:03
wrtpniemeyer: cool. which user is doing the logging?17:04
wrtpwe've got a meeting now, right?17:04
niemeyerwrtp: ubuntulog2 sounds suspect17:04
wrtplol17:05
TheMueniemeyer: thx, will do so, but scanning a lot of fluent and overlapping discussions is sometimes painful. don't we have a better way to discuss larger technical changes in a threaded way?17:06
wrtpah, logs have only been for the last two weeks17:06
fwereade_niemeyer, btw, wrtp's gozk locking change is a bit of a heavyweight blocker: rog has one stacked behind it, I have one stacked behind each of those, and TheMue's agent work will likely want to change based on one of *those*17:07
niemeyerTheMue: Sorry, but that's how the discussion took place.. fwereade_, rog and myself have been punching that idea for several days here in this channel17:07
TheMueniemeyer: yes, it can't be changed for the past, no pro. but i'm looking for an idea for the future.17:08
wrtpTheMue: your best bet is to look at fwereade_'s current proposal, i think17:08
TheMuewrtp: as i already wrote i'll do this17:08
fwereade_niemeyer, TheMue: I think we're at the point where we're only going to have more conflicts if we're not aware of exactly what we're working on17:08
niemeyerTheMue: Even in the future, this channel will continue to be the best place to discuss ideas like that live17:09
fwereade_TheMue, for example, I had some vague idea you were working on relation state17:09
wrtpTheMue: and the comments on that proposal are moderately coherent, i think17:09
niemeyerTheMue: Major features will have associated specs and whatnot17:09
niemeyerTheMue: But this one, specifically, was developed very organically..17:10
TheMueniemeyer: for a quick discussion irc is ok, but i'm missing a proper way to gather the conclusions out of thos multiple discussions in different time zones17:10
TheMuefwereade_: i've got to wait with relations. we'll discuss it then.17:10
niemeyerTheMue: Even better than gathering conclusions, in such an area that affects you directly, is paying attention to the channel and participating in it so that you're part of the team solidifying the idea17:10
niemeyerTheMue: The conclusion, though, is available. fwereade_ has a branch, with a short and nice implementation of everything that was debated.17:12
niemeyerTheMue: Please let us know what you think about it.17:12
TheMueniemeyer: yep17:12
wrtphazmat, niemeyer, jimbaker, bcsaller, TheMue, fwereade_, niemeyer: are we doing a meeting, or defer it for a week?17:14
bcsallerwrtp: I'm listening to the charm school webcast still17:15
jimbakerwrtp, i'm good either way17:15
niemeyerwrtp: I'm game to defer it, if no one has important needs to be discussed17:15
niemeyerI'd rather focus on reviews..17:15
wrtpniemeyer: sounds good to me :-)17:16
TheMuewrtp: thx for review17:16
wrtpTheMue: np17:17
niemeyerfwereade_: I'll review the gozk locking now, then17:21
niemeyerI wish Rietveld had a handy "Diff since my last comment" button17:23
niemeyerwrtp: LGTM on safe-close17:24
niemeyerfwereade_: ^17:24
fwereade_niemeyer, sweet17:24
wrtpniemeyer: i usually click on the comment, then adjust the view manually to show that revision and the latest one.17:24
wrtpniemeyer: cool, thanks.17:24
niemeyerwrtp: That's what I do as well.. still boring, though17:24
niemeyerwrtp: Certainly better than anything else I used to do, OTOH17:25
wrtpniemeyer: i was happy when i discovered that using the keyboard shortcuts keeps the revisions intact.17:25
wrtpniemeyer: IMHO clicking "next" and "prev" should do too.17:25
niemeyerwrtp: It does for me.. ?17:25
wrtpniemeyer: hmm, it does for me too now. maybe it didn't work before. or more likely it's that crack again.17:28
niemeyer:-)17:30
* wrtp got the tool to wire the RJ45 jacks today... goodbye flaky wireless connection!17:31
wrtp(assuming i actually manage to use it properly.)17:32
niemeyerwrtp: :-(17:32
niemeyerwrtp: Welcome to my reality too..17:33
niemeyerwrtp: I think the "attempt" stuff is obscuring the actual logic going on in these functions, for little practical benefit17:33
niemeyerwrtp: I'd be curious to see a "for" versions that used a couple of constants.. I suspect it might be shorter, or of the same length at most17:34
niemeyerwrtp: and will be a lot more readable17:34
niemeyerwrtp: In the end, all of the logic around attempt is an abstraction for a for loop with a sleep, which somewhat of a trivial construct17:36
wrtpniemeyer: i appreciate that point of view17:37
wrtpniemeyer: but that logic in attempt.do is actually a little more complex than that (it's two for loops) and it's nice to have the policy centralised (for instance we might do exponential backoff or random delays)17:38
niemeyerwrtp: We might make it more complex, but we might also not, if the simpler version tends to work.17:38
niemeyerwrtp:17:39
niemeyer 72 »       err := longAttempt.do(17:39
niemeyer  73 »       »       func(err error) bool {17:39
niemeyer  74 »       »       »       return err == noDNS || err == environs.ErrMissingInstanc17:39
niemeyer     e17:39
niemeyer  75 »       »       },17:39
niemeyer  76 »       »       func() error {17:39
niemeyerwrtp: This is completely unreadable by itself17:39
niemeyerwrtp: I suspect the for version of this same function would be trivial17:40
wrtpniemeyer: obviously i'm biased, but if it you read it as "while transient() {body}" then i think it's ok to read17:41
niemeyerwrtp: Let me write the for version then..17:41
wrtpniemeyer: that is the most complex example, of course.17:41
niemeyerThe Unity in precise right now is disturbingly broken.. :(17:43
wrtpniemeyer: if we only need one loop then you're probably right. the two loop thing (one fast, for eventual consistency) and one slow (for other stuff) came out of discussion on this channel.17:46
fwereade_goodnight all, see you tomorrow :)17:46
niemeyerfwereade_: Have a good one!17:47
wrtpfwereade_: nighty night17:47
wrtpniemeyer: here's one example as a loop: http://paste.ubuntu.com/874880/17:47
TheMuefwereade_: bye17:47
wrtpniemeyer: or, a slightly alternative phrasing: http://paste.ubuntu.com/874882/17:48
wrtpniemeyer: but both of those would make it hard to move to a less naive way of timing out.17:49
wrtpactually, i have another idea17:51
niemeyerwrtp: http://paste.ubuntu.com/874887/17:51
wrtpniemeyer: there are two retrySleep constants, of course.17:52
niemeyerwrtp: Why?17:52
wrtpniemeyer: short and long.17:52
niemeyerwrtp: Why?17:52
wrtpniemeyer: short for pure eventual consistency issues17:52
wrtpniemeyer: long for stuff that takes ages17:53
niemeyerwrtp: Why do we need two constants, still?17:53
wrtpniemeyer: the DNS stuff can take 2 or 3 minutes.17:53
wrtpniemeyer: other stuff is good within 5 seconds17:53
niemeyerwrtp: and it may also not..17:53
niemeyerwrtp: I'd say let's have a single constant that feels enough a delay to not burn CPU/network to a noticeable level, and use it in both circumstances17:54
wrtpniemeyer: well, neither might the DNS name be ready in 3 minutes. we've got to choose a limit somewhere17:54
andrewsmedinaniemeyer: hi17:54
niemeyerandrewsmedina: Hi17:54
wrtpniemeyer: the problem is sometimes we *expect* an error. and we don't want to time out the whole three minutes when 5 seconds is enough.17:54
niemeyerwrtp: Sure, let's use 5 seconds for both then17:55
niemeyerwrtp: In computational time, 5 seconds is an eternity17:55
wrtpniemeyer: we can't. then users of the ec2 environ need to know about the constants themselves.17:55
wrtp[17:53] <wrtp> niemeyer: the DNS stuff can take 2 or 3 minutes.17:56
niemeyerwrtp: Huh?17:56
niemeyer<niemeyer> wrtp: and it may also not..17:56
niemeyer<niemeyer> wrtp: I'd say let's have a single constant that feels enough a delay to not burn CPU/network to a noticeable level, and use it in both circumstances17:56
wrtpniemeyer: yeah, but it *always* takes more than a minute17:56
niemeyerwrtp: Not necessarily..17:57
wrtpniemeyer: whereas the other stuff *always* takes less than 5 seconds17:57
niemeyerwrtp: Please complete the sentence: "Using 5 seconds for both will be *really* bad because ..."17:57
wrtpniemeyer: ... because we'll always return an error for the DNS name when booting up and trying to connect.17:58
niemeyerwrtp: Why?17:58
wrtpniemeyer: because the DNS name takes longer than 5 seconds to be set.17:58
niemeyerwrtp: This is the *sleep*, not the limit17:59
wrtpniemeyer: i'm sure i discussed this with you before.17:59
wrtpniemeyer: oh, sorry, i was talking about the limit17:59
wrtpniemeyer: (which is the important thing IMHO)17:59
wrtpah, i see now18:00
wrtpi mentioned retrySleep18:00
niemeyerRight18:00
wrtpthe same applies to retryCount.18:01
niemeyerwrtp: The same doesn't apply to retrySleep, though18:01
niemeyer:-)18:01
wrtpniemeyer: 5s is too long for the short wait, i think18:01
wrtpniemeyer: currently i'm using 200ms18:02
wrtpniemeyer: and usually it's good on the second attempt18:02
niemeyerwrtp: Sounds good then.. we can have retryShortLimit and retryLongLimit18:02
niemeyerwrtp: and the same for retrySleep if needed18:02
niemeyerwrtp: It's still two constants in a for18:02
niemeyerAll of that amounts to the following four lines:18:03
niemeyerfor i := 0; i < retryLimit; i++ {18:03
niemeyerif i > 0 {18:03
niemeyer18:03
niemeyerHmm.. copy & paste broken18:03
niemeyerAgain:18:03
niemeyerfor i := 0; i < retryLimit; i++ {18:03
niemeyerif i > 0 {18:03
niemeyertime.Sleep(retrySleep)18:03
niemeyer}18:03
wrtpniemeyer: i still like the idea of centralising the policy somewhere. i have an idea, one mo.18:03
niemeyerThis is "attempt"18:03
niemeyerwrtp: The policy is centralized.. retryLimit and retrySleep are central18:04
niemeyerwrtp: The overhead of reinventing for and if in those 4 lines is not really buying us anything, and is making the logic unreadable18:04
wrtpniemeyer: as i said, i have an idea that you *might* like.18:05
* wrtp is just putting a paste together18:06
wrtpniemeyer: how does this look to you for usage: http://paste.ubuntu.com/874912/18:14
wrtpniemeyer: (this is the implementation: http://paste.ubuntu.com/874913/)18:15
wrtpnot compiled or tested, of course18:16
niemeyerwrtp: That might work, but I suggest a small tweak as:18:24
niemeyerwrtp: l := retryLoop(delay); for l.next() {18:24
niemeyerwrtp: Well, I guess you still two variables.. l := retryLong() and l := retryShort() then18:26
niemeyerstill need18:26
niemeyerwrtp: and have the loop as "for l.next() {".. resembles vaguely the use in the sql package18:26
wrtpniemeyer: actually, i started off by calling next, but thought you'd prefer waitNext :-)18:27
wrtps/calling/calling it/18:27
niemeyerwrtp: it's rows.Next() in database/sql, and it's iter.Next(&foo) in mgo, so we have some precedents there18:28
niemeyerwrtp: +1 on the approach, either way.. much better18:29
wrtpniemeyer: agreed. i thought it was perhaps different because it slept. i like next though.18:29
wrtpniemeyer: cool.18:29
wrtpniemeyer: are you ok with the names i chose?18:29
wrtp(attempt, start and next, in particular)18:30
wrtpi think they work quite well18:30
niemeyerwrtp: I'd prefer retry rather than attempt, since it makes more obvious that the goal is to try again while attempt does not18:30
niemeyerwrtp: But this is minor.. won't fight over it if you have a strong preference for attempt18:31
wrtpthe idea behind "attempt" is that this all makes up one larger attempt to do something, although we try several times.18:32
wrtp"retry" could work too though. i'll try both and see how they look.18:32
niemeyerwrtp: Cheers18:32
wrtpniemeyer: i think the difficulty i have with "retry" is that to me it implies a single try, whereas "attempt" hints at more sustained activity.18:36
niemeyerwrtp: You can retry 100 times too18:37
niemeyerwrtp: no connotation of singularity there18:37
wrtpniemeyer: indeed, but i want the name of the type to imply the whole thing, rather than one part of it.18:37
niemeyerwrtp: Please name as you please :)18:37
wrtpthanks18:38
niemeyerhazmat: wtf was hung on bzr pull somehow.18:48
wrtpniemeyer: here's what DNSName now looks like BTW. (untested) http://paste.ubuntu.com/874994/18:53
wrtpdefinite improvement from my previous version, i think. thanks for pushing me in this direction.18:53
niemeyerwrtp: Much better indeed, thanks!18:54
niemeyerwrtp: noDNS can die, btw18:54
wrtpniemeyer: it's gone already18:54
niemeyerwrtp: Cool, it was in the paste18:54
wrtpniemeyer: yeah, i noticed it just afterwards18:54
wrtpniemeyer: wouldn't've got through the compiler anyway18:55
wrtp80 lines of test code disappear, mmmm19:20
niemeyerwrtp: 8)19:39
wrtpniemeyer: hmm, goyaml seems to have broken19:40
niemeyerwrtp: How so? charmstore depends on it, and apparently it compiled a few hours ago fine19:40
wrtpniemeyer: i'll try updating19:41
niemeyerwrtp: What's the error you got?19:41
wrtpniemeyer: it's not unmarshalling correctly19:41
niemeyerwrtp: Uh oh19:41
wrtpniemeyer: http://paste.ubuntu.com/875062/19:42
wrtpniemeyer: lots of contents lost19:42
wrtpniemeyer: seems like it's true of the latest revision (33) too.19:43
wrtpniemeyer: i'll push go-ec2-robustness anyway, as this seems like an unrelated bug.19:44
niemeyerwrtp: This doesn't even look like valid yaml19:45
niemeyerwrtp: I'm surprised it doesn't return an error19:45
wrtpniemeyer: oh. it was produced as output from goyaml itself19:45
wrtpniemeyer: what's the problem with it?19:45
niemeyerwrtp: Wait, my copy & paste is what's broken (at least)19:46
wrtp:-)19:46
wrtpsheesh, indentation-based nesting19:47
niemeyerwrtp: Broken indeed.. checking it out20:00
wrtp niemeyer: yeah, goyaml doesn't pass its tests20:00
niemeyerwrtp, rogpeppe: goyaml was unintendedly exploring the mutability of interfaces21:31
niemeyerBroke up after Russ fixed the bug21:31
rogpeppeniemeyer: interesting21:33
rogpeppeniemeyer: easy to fix?21:33
niemeyerrobbiew: Yeah21:41
niemeyerArgh21:41
niemeyerrog: Yeah21:42
robbiewlol21:42
* robbiew was confused at first21:42
rogniemeyer: cool.21:42
rognie21:42
niemeyerwrtp: Still facing network issues?21:53
niemeyerrogpeppe: https://codereview.appspot.com/578406021:55
rogpeppeniemeyer: LGTM22:17
fwereade_niemeyer, ignore my PTAL of a few minutes ago, nothing you mentioned has been addressed (I came back to the machine to see a half-complete propose from fixing rog's previous and merging trunk)22:18
niemeyerfwereade_: Ah, no worries22:24
niemeyerrogpeppe: Cheers, submitting22:24
niemeyerQuite surprising how much goyaml has resisted without changing, given how much everything else broke in the Go 1 rush22:30
rogpeppeniemeyer: have you done the goyaml tagging?22:31
niemeyerrogpeppe: Just doing it22:31
rogpeppeniemeyer: BTW before you LGTM it, i think the lxc environ should keep the Container type private. i've been meaning to review that branch for a bit.22:32
niemeyerrogpeppe: Done22:33
niemeyerrogpeppe: Hmm, not sure I see why.. seems like a thin wrapper on the lxc command line22:34
rogpeppeniemeyer: isn't the point of the package to implement an environs.Environ?22:35
rogpeppeniemeyer: Container is just an implementation detail, i think22:35
niemeyerrogpeppe: No, that's go/lxc22:35
niemeyerrogpeppe: What you have in mind is go/environs/lxc22:36
niemeyerNot even that, actually22:36
niemeyerrogpeppe: it's go/environs/local22:36
niemeyerlxc is a thing wrapper for management of lxc, independent from juju22:36
rogpeppeniemeyer: oh. i think i misundersood originally. the current package path being reviewed is environs/lxc22:37
rogpeppeniemeyer: (and given how small it is, i'm not sure if it needs its own package)22:37
niemeyerrogpeppe: Ok, that's the problem then22:37
niemeyerrogpeppe: Maybe..22:37
niemeyerrogpeppe: My plan was actually to move that out onto its own package22:38
niemeyerrogpeppe: That said, you're right..22:38
rogpeppeniemeyer: depends how much more of it there is to come, i guess, but the code to run the lxc commands is less than that to call the ec2 functions...22:38
niemeyerrogpeppe: We can keep that internal for the moment, and then make it public when/if we ever move it out22:38
rogpeppeniemeyer: that seems good to me22:39
niemeyerrogpeppe: goamz is its own package ;)22:39
rogpeppeniemeyer: i meant to call the goamz/ec2 functions...22:39
niemeyerrogpeppe: goamz/ec2 is its own package too.. the point is that there's an external wrapper that handles the bits that do not depend on juju22:39
niemeyerrogpeppe: You're right that what we have there is small, though22:40
niemeyerrogpeppe: So we can keep it internal for the moment22:40
rogpeppeniemeyer: sure. but in the goamz/ec2 case it's doing real work. here it's almost trivial.22:40
rogpeppeyeah22:40
rogpeppewell, if it gets bigger, i'd be happy to factor it out22:40
niemeyerrogpeppe: Can you please make the point in the review so that andrewsmedina can address it before we merge?22:42
rogpeppeniemeyer: ok, i'm about to go to bed, but i'll write a brief reply.22:42
andrewsmedinaniemeyer: hi22:42
niemeyerandrewsmedina: Heya22:42
niemeyerandrewsmedina: You got a review22:42
andrewsmedinaniemeyer: yes22:42
andrewsmedinaniemeyer: 2 reviews22:43
niemeyerandrewsmedina: and rog was just suggesting the Container type stay as "container" for the moment22:43
andrewsmedinaniemeyer: one for initial work with lxc in juju22:43
andrewsmedinaniemeyer: and another in goetveld https://codereview.appspot.com/5683064/22:44
andrewsmedinaniemeyer: I just read your review22:46
niemeyerandrewsmedina: This goetveld branch has been submitted two weeks ago.. ?22:47
niemeyerandrewsmedina: Maybe I'm just missing the tag22:47
andrewsmedinaniemeyer: I can skip the tests if sudo isn't acessible22:47
niemeyerandrewsmedina: That's what was suggested there22:48
andrewsmedinaniemeyer: I've updated the old review in goetveld22:48
niemeyerandrewsmedina: But skip it at all times please, unless -lxc is provided22:48
niemeyerandrewsmedina: Updated the old review!?22:48
andrewsmedinaniemeyer: I got issues to memorize the "goetveld" name22:49
niemeyerandrewsmedina: This branch and CL in Rietveld have been submitted, which means they are effectively dead now22:49
niemeyerandrewsmedina: If you have more changes, they must be proposed in a different branch/CL22:49
andrewsmedinaniemeyer: I will create another branch then22:50
rogpeppeandrewsmedina: you've got another review22:51
niemeyerandrewsmedina: Thanks!22:51
andrewsmedinarogpeppe: thanks22:51
rogpeppeoff to bed now. 'night all.22:51
andrewsmedinaI'm very happy to contribute with juju and others projects in go22:51
andrewsmedinarogpeppe: night22:52
rogpeppeandrewsmedina: it's much appreciated!22:52
fwereade_niemeyer, https://codereview.appspot.com/5672067 may actually be done now :)22:55
niemeyerfwereade_: Cheers :)22:58
niemeyerfwereade_: I may only get to it in the morning as well, though22:58
niemeyerIt's getting late here22:58

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