=== flaviamissi_ is now known as flaviamissi | ||
* hazmat wonders if he needs to send a mother may i email to the list | 03:13 | |
fwereade | rog, hopefully trivial: https://codereview.appspot.com/5786051 | 10:52 |
---|---|---|
fwereade | rog, and, blast, I just realised you've got that gozk review still up (right?) | 10:53 |
fwereade | rog, I checked for *juju* reviews and completely forgot the rest :/ | 10:53 |
rog | fwereade: yeah, i've been waiting for almost 2 weeks now. sigh. | 10:53 |
fwereade | rog, I know the feeling | 10:54 |
fwereade | rog, I think I've passed some sort of milestone now, though: I have more go branches stacked up than I do python ones | 10:54 |
rog | fwereade: :-) | 10:54 |
rog | fwereade: i wonder if instead of a new package, whether we might expand the jujutest package a bit to add testing code. | 10:55 |
rog | fwereade: it could be moved out of environs, and maybe just renamed "test", perhaps. | 10:56 |
fwereade | rog, doesn't sound like a bad idea, I could follow up with a branch that does that perhaps? | 10:57 |
fwereade | rog, for some reason I expect things called "test" to contain actual tests | 10:58 |
fwereade | rog, but I'm not seriously opposed to that name | 10:58 |
rog | fwereade: given that testutil is new, why not add ZkSetUpEnvironment to jujutest now? | 10:58 |
rog | fwereade: we could call it "testing" | 10:58 |
rog | fwereade: i know it matches the name of the std testing package, but i don't think that's a problem | 10:59 |
rog | import "launchpad.net/juju/go/testing" sounds reasonable to me | 10:59 |
rog | fwereade: 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 |
TheMue | rog: +1 | 11:02 |
rog | TheMue: nothing we can do about that though... | 11:03 |
TheMue | rog: the testing package or the directory part? | 11:04 |
rog | TheMue: the directory part. were you referring to the testing package? | 11:04 |
fwereade | rog, so we'd do `import jtesting "launchpad.net/juju/go/testing"` or something? | 11:04 |
TheMue | rog: yep | 11:05 |
rog | fwereade: 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 |
rog | i quite like "scenarion" | 11:05 |
TheMue | rog, fwereade : as long as our functions in the package have a clean naming it also could be imported with _. | 11:06 |
rog | TheMue: huh? surely we can't if we want to use it... | 11:07 |
rog | ? | 11:07 |
TheMue | rog: eh, i meant . like with gocheck | 11:08 |
rog | TheMue: no no no! no more import "."s! gocheck is one too many as it is! | 11:09 |
TheMue | rog: what troubles do you have with it? | 11:09 |
rog | TheMue: namespace pollution | 11:09 |
rog | TheMue: why don't we import many things with "."? | 11:10 |
rog | TheMue: answer: because it's nice to see a name and know where it comes from. | 11:10 |
rog | TheMue: 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 |
rog | TheMue: and sometimes there's a cyclic dependency | 11:11 |
TheMue | rog: 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 aspect | 11:11 |
rog | TheMue: but IMHO it should never be used | 11:11 |
rog | TheMue: i don't think that's a problem. | 11:11 |
rog | TheMue: the compiler knows and will tell you very quickly | 11:12 |
rog | TheMue: we already have two packages with the identifier "ec2" for example | 11:12 |
rog | TheMue: and it's not a problem. | 11:12 |
TheMue | rog: i don't talk about the compiler, i talk about people who maintain different sources where the same testing package is imported with different aliases | 11:12 |
TheMue | rog: and i our tests we would need to import both, so one always has to be aliased | 11:13 |
rog | TheMue: it's not uncommon to rename a package with a different alias when you import it | 11:13 |
rog | TheMue: if we have a standard alias in that case, i don't think there's a problem | 11:13 |
rog | TheMue: for example, when i import "launchpad.net/goamz/ec2" and "launchpad.net/juju/go/environs/ec2", i name the former "amzec2". | 11:14 |
rog | TheMue: similarly we can import "juju/go/testing" as jujutesting when there's a clash. | 11:14 |
TheMue | rog: 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 terms | 11:14 |
rog | TheMue: it doesn't matter too much. it's a variable name - we have preferences but not strict rules. | 11:15 |
TheMue | rog: that's one of the problems | 11:16 |
TheMue | rog: it will differ over time | 11:16 |
rog | TheMue: the package identifier? | 11:17 |
TheMue | rog: and new code maintainers will stumble about it | 11:17 |
TheMue | rog:the aliases for the same package in different tests | 11:17 |
rog | TheMue: i don't think it'll cause more than a moment's pause. | 11:18 |
rog | TheMue: most of our packages don't use the standard "testing" package. | 11:18 |
TheMue | rog: the tests do, and they often will import both, the standard and our testiing package | 11:19 |
rog | TheMue: and it can't give rise to any more problems than a failed compile AFAICS | 11:19 |
TheMue | rog: 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 it | 11:20 |
rog | TheMue: i don't see that as a problem in this case - a quick glance at the top of the file will immediately resolve any queries | 11:20 |
TheMue | rog: this look here, that look there, another look at a different time - that sums | 11:21 |
TheMue | rog: just again, 70% of software costs are maintenance costs | 11:23 |
rog | TheMue: also, which one you're using will be obvious from the context. | 11:23 |
rog | TheMue: remember, you were suggesting importing as ".". that's much worse! | 11:23 |
TheMue | rog: yep, i've got to admit that this has been a bad idea. a reason why i wonna look for a better one | 11:24 |
rog | TheMue: 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 |
rog | TheMue: all that said, have you got another suggestion for a name? | 11:26 |
TheMue | rog: not yet, im trying to find one. | 11:26 |
TheMue | rog: hmm, opposit to testing or gocheck it doesn't provide a testing itself | 11:29 |
rog | TheMue: no, but it's about testing juju, and that seems ok to me | 11:29 |
TheMue | rog: it's more that it will contain tools for testing juju | 11:29 |
rog | TheMue: the other suggestion was "test", which might be ok too | 11:29 |
rog | TheMue: as it does contain some tests as well as utils | 11:30 |
TheMue | rog: my ideas are testtools or testenv so far | 11:30 |
rog | TheMue: it's more than just tools - it has tests. and it's more than testing the environment. | 11:31 |
TheMue | rog: shall it DO tests or only SUPPORT tests? and i didn't meant to test the environment but to provide an environment for juju testing | 11:32 |
rog | TheMue: 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 tools | 11:33 |
TheMue | rog: how does it relate to the tests inside the packages? | 11:34 |
TheMue | rog: i would like a package providing the environment and tools but to keep the individual tests inside the packages | 11:34 |
rog | TheMue: it currently implements provider-independent tests to test an Environ | 11:34 |
rog | TheMue: the whole point is that those tests are independent of a particular provider, so they can't live in a single package. | 11:35 |
rog | fwereade: 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 |
TheMue | rog: don't the provider packages have a superior pckage for those tests? | 11:37 |
rog | TheMue: sorry, i don't understand. | 11:37 |
fwereade | rog, ah, cool | 11:38 |
TheMue | rog: a package one level above. | 11:38 |
rog | TheMue: do you mean launchpad.net/juju/go/environs ? | 11:39 |
TheMue | rog: yes | 11:39 |
rog | TheMue: i don't want to pollute its exported namespace with testing code | 11:40 |
TheMue | rog: won't that testing package be exported too? | 11:40 |
rog | TheMue: hence i use a different package (currently launchpad.net/juju/go/environs/jujutest) | 11:40 |
rog | TheMue: yes, but that testing package is *just* about testing. | 11:40 |
rog | TheMue: i don't want to mix testing code and production code - it'll just confuse people. | 11:41 |
TheMue | rog: yep, that's right | 11:42 |
TheMue | rog: so far in my case the testing stuff is invisible | 11:42 |
rog | TheMue: so i was proposing to fwereade that the jujutest package becomes launchpad.net/juju/go/testing | 11:43 |
rog | TheMue: because i'm not sure we need a proliferation of packages to do with juju testing. | 11:44 |
TheMue | rog: the movement makes sense, still i've got a naming problem | 11:44 |
rog | TheMue: ok, well perhaps we'll keep the existing name ("jujutest") for the time being, pending discussion. | 11:44 |
TheMue | rog:it's that the standard "testing" is a test framework/library while our "testing" would be tests | 11:45 |
TheMue | rog: "juju" would be redundant there, how simply "tests" | 11:45 |
rog | i 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 tests | 11:46 |
rog | TheMue: 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 |
fwereade | rog, the thing is I do see the proposal as creating the very rudimentary beginnings of a testing framework | 11:47 |
rog | fwereade: +1 | 11:47 |
TheMue | fwereade: in this case we should not put this framework and the tests themself into one package | 11:48 |
fwereade | rog, 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 example | 11:49 |
fwereade | rog, my perspective is that the proposal moves a small step in a good direction that I can then use to start testing hook contexts | 11:50 |
rog | TheMue: 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 IMHO | 11:59 |
rog | fwereade: 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 |
rog | fwereade: i'm assuming a couple of changes to the zk package | 12:09 |
rog | fwereade: 1) that the server type gets an Addr function that can be used to find out an address for the server | 12:09 |
rog | fwereade: 2) that the server can be created on an existing directory, as long as it's empty | 12:10 |
rog | fwereade: 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 |
rog | fwereade: here's another idea, that saves us the error checking each time: http://paste.ubuntu.com/874481/ | 12:13 |
rog | fwereade: (TestingT is implemented both by *testing.T and *gocheck.C) | 12:13 |
* hazmat yawns | 12:50 | |
fwereade | rog,sorry, Iwasat lunch, give me a sec to catch up | 13:14 |
fwereade | rog, tbh I'm entirely unattached to any specific interface | 13:15 |
fwereade | rog, what I suggested I suggested purely because it looked like it'd work both for state and for my needs | 13:16 |
wrtp | fwereade: cool. what do you think of, say, my last suggestion? | 13:21 |
fwereade | wrtp, that looks fine to me | 13:22 |
fwereade | wrtp, I like the idea of having Addr on Server too | 13:22 |
wrtp | fwereade: good. | 13:23 |
fwereade | wrtp, btw, re the zk locking: | 13:23 |
fwereade | wrtp, I'm comfortable with tests that can theoretically fail under enough load | 13:23 |
fwereade | wrtp, but that's kinda hard to quantify | 13:24 |
wrtp | fwereade: yeah, i kind of am too, but it's that creeping "i'm adding just that little bit more flakiness" feeling that concerns me | 13:24 |
fwereade | wrtp, for example, how low does the tiemout have to be on your machine before you start to see semi-regular failures? | 13:24 |
wrtp | fwereade: i don't know. wait until we're running the tests on ARM :-) | 13:25 |
fwereade | wrtp, heh | 13:25 |
wrtp | niemeyer: yo! | 13:25 |
fwereade | wrtp, ok, what if you run them 100 times now? | 13:26 |
niemeyer | Good morning jujuers! | 13:26 |
fwereade | niemeyer, heyhey | 13:26 |
niemeyer | wrtp: Heya | 13:26 |
niemeyer | fwereade: Alow | 13:26 |
fwereade | wrtp, what would be the expected failure count? | 13:26 |
wrtp | fwereade: i'll have a try. am just writing a test for CreateServer currently. | 13:26 |
wrtp | niemeyer: after discussion this morning, i'm changing zookeeper.CreateServer so that it will succeed if the server directory exists and is empty. | 13:27 |
wrtp | niemeyer: which means that Server.Destroy is sufficient to delete the tmp directory. | 13:27 |
niemeyer | wrtp: Hmm.. why is it necessary? | 13:27 |
niemeyer | (awww... charmstore build broke in the PPA) | 13:28 |
wrtp | niemeyer: darn. hope you get good logs... | 13:28 |
wrtp | niemeyer: because fwereade proposed this: https://codereview.appspot.com/5786051/diff/1/testutil/zk.go | 13:28 |
wrtp | niemeyer: and i thought the complexity is unecessary | 13:28 |
wrtp | niemeyer: it seems silly that you can create a temp directory and run a zookeeper server in it. | 13:29 |
wrtp | s/can/can't/ | 13:29 |
wrtp | niemeyer: so this was my counter-suggestion: http://paste.ubuntu.com/874481/ | 13:29 |
wrtp | niemeyer: assuming also an Addr method on Server. | 13:30 |
niemeyer | wrtp: Looks good. Don't we have Addr? | 13:30 |
wrtp | niemeyer: nope, i don't think so. | 13:31 |
wrtp | niemeyer: it would just return 127.0.0.1:port | 13:31 |
niemeyer | wrtp: Yeah, it sounds fine.. how do we even connect to the test server nowadays then/ | 13:32 |
niemeyer | ? | 13:32 |
wrtp | niemeyer: we just assume localhost | 13:32 |
wrtp | niemeyer: and we specify the port, so we know the address. | 13:32 |
niemeyer | wrtp: Ah, gotcha | 13:32 |
niemeyer | wrtp: Yep, +1 on Addr too | 13:32 |
wrtp | niemeyer: there was also a discussion about where to put this kind of helper function. | 13:32 |
wrtp | niemeyer: i wondered if it might make sense to make environs/jujutest more general. | 13:33 |
wrtp | niemeyer: and perhaps rename it to launchpad.net/juju/go/testing | 13:33 |
wrtp | niemeyer: TheMue wasn't so happy about that name though, because of the clash with the standard testing package | 13:33 |
hazmat | niemeyer, http://wtf.labix.org has been mia for a few weeks. | 13:35 |
hazmat | niemeyer, also the enhanced relation support spec is ready for a review (its been revised and scoped appropriately) | 13:36 |
hazmat | niemeyer, and g'morning | 13:36 |
niemeyer | hazmat: Morning :-) | 13:36 |
niemeyer | hazmat: Will check it out | 13:36 |
niemeyer | s/it/them | 13:36 |
niemeyer | wrtp: Hmm | 13:37 |
niemeyer | wrtp: jujutest doesn't feel entirely generic | 13:38 |
wrtp | niemeyer: the name or the stuff in it? | 13:38 |
niemeyer | wrtp: The stuff in it | 13:39 |
wrtp | niemeyer: it's only two types. if renamed EnvironTests and EnvironLiveTests, i think they'd live quite happily alongside other more generic testing-related functions | 13:40 |
fwereade | wrtp, I guess the question is, how many other packages will be using them? | 13:42 |
wrtp | fwereade: all the environs implementations. that's it, probably. | 13:42 |
fwereade | wrtp, that does suggest to me that it might live more happily under environs tbh | 13:42 |
niemeyer | wrtp: Maybe rename go/environs/jujutest as go/environs/envtest, and create a new go/jujutest/? | 13:43 |
wrtp | niemeyer: yeah, that would work, although i quite like go/testing as a name | 13:45 |
niemeyer | wrtp: envtesting and testing then? | 13:45 |
wrtp | niemeyer: sounds good | 13:45 |
niemeyer | wrtp: I don't mind "testing".. we don't use the stock testing package except for hooking gocheck in anyway | 13:45 |
wrtp | niemeyer: i agree. | 13:46 |
wrtp | niemeyer: 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 |
wrtp | niemeyer: i'm happy to import "launchpad.net/juju/go/testing" as gotesting, or "testing" as stdtesting though. | 13:48 |
niemeyer | wrtp: The latter, please | 13:48 |
wrtp | niemeyer: yeah. internal gets preference. | 13:48 |
niemeyer | wrtp: Yeah | 13:49 |
wrtp | fwereade: do you want to mutate your CL accordingly while i'll make the changes to zk. | 13:50 |
wrtp | s/\./?/ | 13:50 |
wrtp | fwereade: (assuming it sounds good to you, of course) | 13:51 |
fwereade | wrtp, sure, sounds great | 13:51 |
fwereade | wrtp, except, sorry, I'm slow | 13:55 |
fwereade | wrtp, we surely don't actually want to leave the temp dir hanging around, do we? | 13:55 |
wrtp | fwereade: srv.Destroy removes the zk directory and its contents. | 13:56 |
fwereade | wrtp, doh sorry | 13:56 |
wrtp | np | 13:56 |
niemeyer | OMG.. | 14:30 |
niemeyer | The charmstore package is biult | 14:30 |
niemeyer | built | 14:30 |
niemeyer | Woohay! | 14:34 |
niemeyer | "You can now launch 64-bit operating systems on the m1.small and c1.medium instances." | 14:34 |
wrtp | niemeyer: congrats! | 14:48 |
niemeyer | wrtp: 8) | 14:49 |
niemeyer | wrtp: Thanks | 14:49 |
fwereade | niemeyer, respectively: cool!; grar, constraints change | 14:49 |
wrtp | fwereade: 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 |
wrtp | fwereade: sorry. | 14:50 |
fwereade | wrtp, no worries, I'll just duplicate the setup stuff for now, it's not too much to carry for a limited time | 14:50 |
wrtp | fwereade: sounds good | 14:51 |
fwereade | wrtp, although one note I decided not to bother with was "shouldn't the gocheck changes be independent?" | 14:51 |
wrtp | fwereade: yes, they should. | 14:51 |
niemeyer | Getting no-deps binaries out of "dpkg -L <go app>" never gets old | 14:51 |
wrtp | fwereade: but it takes so long to turn around one branch, i thought it better to do them in the same one. bitten! | 14:52 |
fwereade | wrtp, yeah, it's a self-reinforcing problem | 14:52 |
niemeyer | robbiew: ping | 14:52 |
wrtp | niemeyer: i don't understand one bit of that remark :-) | 14:52 |
fwereade | wrtp, slow turnaround -> bigger diffs -> yet slower turnaround | 14:52 |
niemeyer | wrtp: This command lists the files in a package | 14:53 |
wrtp | fwereade: yeah. well, the diffs were fairly small for the gocheck changes, hence my merging. | 14:53 |
fwereade | wrtp, indeed, not an irrational response | 14:54 |
wrtp | niemeyer: still in the dark "Getting no-deps binaries"? you don't need to explain, BTW :-) | 14:55 |
niemeyer | wrtp: binaries without dependencies | 14:55 |
wrtp | ah, i understand now. no dynamic library dependencies... | 14:57 |
niemeyer | wrtp: No additional package dependencies, at least.. it does link against some dynamic libraries | 14:59 |
wrtp | niemeyer: 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 | |
robbiew | niemeyer: pong | 15:03 |
niemeyer | robbiew: Heya | 15:06 |
niemeyer | robbiew: I could use that RT push :) | 15:06 |
robbiew | :) | 15:06 |
robbiew | niemeyer: ticket #? | 15:06 |
niemeyer | wrtp: I can tell there are only two binaries | 15:06 |
niemeyer | robbiew: 51351.. a pretty number! | 15:07 |
robbiew | lol | 15:07 |
robbiew | ack | 15:07 |
niemeyer | robbiew: I've contacted #is first, but given the silence filed this RT yesterday with the IRC questioning | 15:07 |
niemeyer | robbiew: and then enhanced to include more details such as the PPA, package name, etc | 15:07 |
niemeyer | robbiew: It'd be great if we managed to find someone in IS that would be indeed interested on mongodb | 15:08 |
niemeyer | robbiew: But I suspect it won't be easy, given previous conversations with James | 15:09 |
robbiew | heh...my powers of influence only expand but so far | 15:09 |
niemeyer | robbiew: 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 way | 15:11 |
robbiew | right | 15:11 |
* robbiew will begin his Jedi mind trick training next week :P | 15:12 | |
niemeyer | robbiew: LOL | 15:13 |
niemeyer | and that's lunch time | 15:43 |
TheMue | So, agent with watcher is ready for review: https://codereview.appspot.com/5782053 | 16:09 |
niemeyer | TheMue: WOOHAY | 16:38 |
niemeyer | I'm now actually blocked on the store since I depend on IS, so I'm moving back onto reviewing mode | 16:39 |
wrtp | TheMue: i'm having a look now | 16:40 |
wrtp | niemeyer: any chance you could start with the oldest ones? :-) | 16:40 |
niemeyer | wrtp: Sounds like a plan ;) | 16:42 |
niemeyer | wrtp: Do you have a preference of ordering to make better use of your remaining time today? | 16:43 |
TheMue | wrtp: *lol* | 16:43 |
wrtp | niemeyer: jeeze. let me think. | 16:43 |
wrtp | niemeyer: go-ec2-robustness is the major one that i'd love to see | 16:43 |
niemeyer | fwereade_: Btw, can you help me out a bit by providing a map for your branches? | 16:45 |
niemeyer | fwereade_: 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 on | 16:46 |
fwereade_ | niemeyer, go-add-cmd-context and go-add-hook-package are the independent ones | 16:46 |
niemeyer | fwereade_: 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 think | 16:47 |
fwereade_ | niemeyer, sounds good | 16:47 |
niemeyer | fwereade_: 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 well | 16:48 |
niemeyer | fwereade_: It's fine to keep it in review then, IMO.. but please just don't merge while the pre-req isn't there | 16:48 |
fwereade_ | niemeyer, ok,sounds good :) | 16:48 |
wrtp | fwereade_: heads up: the zk.Server.Addr method returns (string, error) not just string. just so you know. | 16:51 |
fwereade_ | wrtp, cool, thanks | 16: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 ago | 16:55 |
TheMue | fwereade_: 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 instead | 16:56 |
TheMue | fwereade_: 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 be | 16:58 |
fwereade_ | TheMue, hopefully it will at least make your code a bit smaller | 16:58 |
fwereade_ | TheMue, but I think we've hit a bit of a communications snafu | 16:58 |
fwereade_ | niemeyer, ^^ | 16:58 |
TheMue | fwereade_: 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 helpful | 17:00 |
TheMue | fwereade_: will take a look | 17:00 |
fwereade_ | TheMue, cheers | 17:00 |
wrtp | TheMue, fwereade_: i think you're right. it seems it was an unfortunate place to start... :-) | 17:02 |
niemeyer | fwereade_, TheMue: Agreed | 17:02 |
niemeyer | TheMue: Please check out the IRC logs | 17:02 |
wrtp | niemeyer: are there any logs of this channel? | 17:02 |
niemeyer | wrtp, TheMue: http://irclogs.ubuntu.com/2012/03/08/%23juju-dev.html | 17:03 |
wrtp | niemeyer: cool. which user is doing the logging? | 17:04 |
wrtp | we've got a meeting now, right? | 17:04 |
niemeyer | wrtp: ubuntulog2 sounds suspect | 17:04 |
wrtp | lol | 17:05 |
TheMue | niemeyer: 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 |
wrtp | ah, logs have only been for the last two weeks | 17: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 |
niemeyer | TheMue: Sorry, but that's how the discussion took place.. fwereade_, rog and myself have been punching that idea for several days here in this channel | 17:07 |
TheMue | niemeyer: yes, it can't be changed for the past, no pro. but i'm looking for an idea for the future. | 17:08 |
wrtp | TheMue: your best bet is to look at fwereade_'s current proposal, i think | 17:08 |
TheMue | wrtp: as i already wrote i'll do this | 17: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 on | 17:08 |
niemeyer | TheMue: Even in the future, this channel will continue to be the best place to discuss ideas like that live | 17:09 |
fwereade_ | TheMue, for example, I had some vague idea you were working on relation state | 17:09 |
wrtp | TheMue: and the comments on that proposal are moderately coherent, i think | 17:09 |
niemeyer | TheMue: Major features will have associated specs and whatnot | 17:09 |
niemeyer | TheMue: But this one, specifically, was developed very organically.. | 17:10 |
TheMue | niemeyer: 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 zones | 17:10 |
TheMue | fwereade_: i've got to wait with relations. we'll discuss it then. | 17:10 |
niemeyer | TheMue: 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 idea | 17:10 |
niemeyer | TheMue: The conclusion, though, is available. fwereade_ has a branch, with a short and nice implementation of everything that was debated. | 17:12 |
niemeyer | TheMue: Please let us know what you think about it. | 17:12 |
TheMue | niemeyer: yep | 17:12 |
wrtp | hazmat, niemeyer, jimbaker, bcsaller, TheMue, fwereade_, niemeyer: are we doing a meeting, or defer it for a week? | 17:14 |
bcsaller | wrtp: I'm listening to the charm school webcast still | 17:15 |
jimbaker | wrtp, i'm good either way | 17:15 |
niemeyer | wrtp: I'm game to defer it, if no one has important needs to be discussed | 17:15 |
niemeyer | I'd rather focus on reviews.. | 17:15 |
wrtp | niemeyer: sounds good to me :-) | 17:16 |
TheMue | wrtp: thx for review | 17:16 |
wrtp | TheMue: np | 17:17 |
niemeyer | fwereade_: I'll review the gozk locking now, then | 17:21 |
niemeyer | I wish Rietveld had a handy "Diff since my last comment" button | 17:23 |
niemeyer | wrtp: LGTM on safe-close | 17:24 |
niemeyer | fwereade_: ^ | 17:24 |
fwereade_ | niemeyer, sweet | 17:24 |
wrtp | niemeyer: i usually click on the comment, then adjust the view manually to show that revision and the latest one. | 17:24 |
wrtp | niemeyer: cool, thanks. | 17:24 |
niemeyer | wrtp: That's what I do as well.. still boring, though | 17:24 |
niemeyer | wrtp: Certainly better than anything else I used to do, OTOH | 17:25 |
wrtp | niemeyer: i was happy when i discovered that using the keyboard shortcuts keeps the revisions intact. | 17:25 |
wrtp | niemeyer: IMHO clicking "next" and "prev" should do too. | 17:25 |
niemeyer | wrtp: It does for me.. ? | 17:25 |
wrtp | niemeyer: 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 |
niemeyer | wrtp: :-( | 17:32 |
niemeyer | wrtp: Welcome to my reality too.. | 17:33 |
niemeyer | wrtp: I think the "attempt" stuff is obscuring the actual logic going on in these functions, for little practical benefit | 17:33 |
niemeyer | wrtp: 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 most | 17:34 |
niemeyer | wrtp: and will be a lot more readable | 17:34 |
niemeyer | wrtp: In the end, all of the logic around attempt is an abstraction for a for loop with a sleep, which somewhat of a trivial construct | 17:36 |
wrtp | niemeyer: i appreciate that point of view | 17:37 |
wrtp | niemeyer: 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 |
niemeyer | wrtp: We might make it more complex, but we might also not, if the simpler version tends to work. | 17:38 |
niemeyer | wrtp: | 17:39 |
niemeyer | 72 » err := longAttempt.do( | 17:39 |
niemeyer | 73 » » func(err error) bool { | 17:39 |
niemeyer | 74 » » » return err == noDNS || err == environs.ErrMissingInstanc | 17:39 |
niemeyer | e | 17:39 |
niemeyer | 75 » » }, | 17:39 |
niemeyer | 76 » » func() error { | 17:39 |
niemeyer | wrtp: This is completely unreadable by itself | 17:39 |
niemeyer | wrtp: I suspect the for version of this same function would be trivial | 17:40 |
wrtp | niemeyer: obviously i'm biased, but if it you read it as "while transient() {body}" then i think it's ok to read | 17:41 |
niemeyer | wrtp: Let me write the for version then.. | 17:41 |
wrtp | niemeyer: that is the most complex example, of course. | 17:41 |
niemeyer | The Unity in precise right now is disturbingly broken.. :( | 17:43 |
wrtp | niemeyer: 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 |
niemeyer | fwereade_: Have a good one! | 17:47 |
wrtp | fwereade_: nighty night | 17:47 |
wrtp | niemeyer: here's one example as a loop: http://paste.ubuntu.com/874880/ | 17:47 |
TheMue | fwereade_: bye | 17:47 |
wrtp | niemeyer: or, a slightly alternative phrasing: http://paste.ubuntu.com/874882/ | 17:48 |
wrtp | niemeyer: but both of those would make it hard to move to a less naive way of timing out. | 17:49 |
wrtp | actually, i have another idea | 17:51 |
niemeyer | wrtp: http://paste.ubuntu.com/874887/ | 17:51 |
wrtp | niemeyer: there are two retrySleep constants, of course. | 17:52 |
niemeyer | wrtp: Why? | 17:52 |
wrtp | niemeyer: short and long. | 17:52 |
niemeyer | wrtp: Why? | 17:52 |
wrtp | niemeyer: short for pure eventual consistency issues | 17:52 |
wrtp | niemeyer: long for stuff that takes ages | 17:53 |
niemeyer | wrtp: Why do we need two constants, still? | 17:53 |
wrtp | niemeyer: the DNS stuff can take 2 or 3 minutes. | 17:53 |
wrtp | niemeyer: other stuff is good within 5 seconds | 17:53 |
niemeyer | wrtp: and it may also not.. | 17:53 |
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 circumstances | 17:54 |
wrtp | niemeyer: well, neither might the DNS name be ready in 3 minutes. we've got to choose a limit somewhere | 17:54 |
andrewsmedina | niemeyer: hi | 17:54 |
niemeyer | andrewsmedina: Hi | 17:54 |
wrtp | niemeyer: 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 |
niemeyer | wrtp: Sure, let's use 5 seconds for both then | 17:55 |
niemeyer | wrtp: In computational time, 5 seconds is an eternity | 17:55 |
wrtp | niemeyer: 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 |
niemeyer | wrtp: 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 circumstances | 17:56 |
wrtp | niemeyer: yeah, but it *always* takes more than a minute | 17:56 |
niemeyer | wrtp: Not necessarily.. | 17:57 |
wrtp | niemeyer: whereas the other stuff *always* takes less than 5 seconds | 17:57 |
niemeyer | wrtp: Please complete the sentence: "Using 5 seconds for both will be *really* bad because ..." | 17:57 |
wrtp | niemeyer: ... because we'll always return an error for the DNS name when booting up and trying to connect. | 17:58 |
niemeyer | wrtp: Why? | 17:58 |
wrtp | niemeyer: because the DNS name takes longer than 5 seconds to be set. | 17:58 |
niemeyer | wrtp: This is the *sleep*, not the limit | 17:59 |
wrtp | niemeyer: i'm sure i discussed this with you before. | 17:59 |
wrtp | niemeyer: oh, sorry, i was talking about the limit | 17:59 |
wrtp | niemeyer: (which is the important thing IMHO) | 17:59 |
wrtp | ah, i see now | 18:00 |
wrtp | i mentioned retrySleep | 18:00 |
niemeyer | Right | 18:00 |
wrtp | the same applies to retryCount. | 18:01 |
niemeyer | wrtp: The same doesn't apply to retrySleep, though | 18:01 |
niemeyer | :-) | 18:01 |
wrtp | niemeyer: 5s is too long for the short wait, i think | 18:01 |
wrtp | niemeyer: currently i'm using 200ms | 18:02 |
wrtp | niemeyer: and usually it's good on the second attempt | 18:02 |
niemeyer | wrtp: Sounds good then.. we can have retryShortLimit and retryLongLimit | 18:02 |
niemeyer | wrtp: and the same for retrySleep if needed | 18:02 |
niemeyer | wrtp: It's still two constants in a for | 18:02 |
niemeyer | All of that amounts to the following four lines: | 18:03 |
niemeyer | for i := 0; i < retryLimit; i++ { | 18:03 |
niemeyer | if i > 0 { | 18:03 |
niemeyer | 18:03 | |
niemeyer | Hmm.. copy & paste broken | 18:03 |
niemeyer | Again: | 18:03 |
niemeyer | for i := 0; i < retryLimit; i++ { | 18:03 |
niemeyer | if i > 0 { | 18:03 |
niemeyer | time.Sleep(retrySleep) | 18:03 |
niemeyer | } | 18:03 |
wrtp | niemeyer: i still like the idea of centralising the policy somewhere. i have an idea, one mo. | 18:03 |
niemeyer | This is "attempt" | 18:03 |
niemeyer | wrtp: The policy is centralized.. retryLimit and retrySleep are central | 18:04 |
niemeyer | wrtp: The overhead of reinventing for and if in those 4 lines is not really buying us anything, and is making the logic unreadable | 18:04 |
wrtp | niemeyer: as i said, i have an idea that you *might* like. | 18:05 |
* wrtp is just putting a paste together | 18:06 | |
wrtp | niemeyer: how does this look to you for usage: http://paste.ubuntu.com/874912/ | 18:14 |
wrtp | niemeyer: (this is the implementation: http://paste.ubuntu.com/874913/) | 18:15 |
wrtp | not compiled or tested, of course | 18:16 |
niemeyer | wrtp: That might work, but I suggest a small tweak as: | 18:24 |
niemeyer | wrtp: l := retryLoop(delay); for l.next() { | 18:24 |
niemeyer | wrtp: Well, I guess you still two variables.. l := retryLong() and l := retryShort() then | 18:26 |
niemeyer | still need | 18:26 |
niemeyer | wrtp: and have the loop as "for l.next() {".. resembles vaguely the use in the sql package | 18:26 |
wrtp | niemeyer: actually, i started off by calling next, but thought you'd prefer waitNext :-) | 18:27 |
wrtp | s/calling/calling it/ | 18:27 |
niemeyer | wrtp: it's rows.Next() in database/sql, and it's iter.Next(&foo) in mgo, so we have some precedents there | 18:28 |
niemeyer | wrtp: +1 on the approach, either way.. much better | 18:29 |
wrtp | niemeyer: agreed. i thought it was perhaps different because it slept. i like next though. | 18:29 |
wrtp | niemeyer: cool. | 18:29 |
wrtp | niemeyer: are you ok with the names i chose? | 18:29 |
wrtp | (attempt, start and next, in particular) | 18:30 |
wrtp | i think they work quite well | 18:30 |
niemeyer | wrtp: I'd prefer retry rather than attempt, since it makes more obvious that the goal is to try again while attempt does not | 18:30 |
niemeyer | wrtp: But this is minor.. won't fight over it if you have a strong preference for attempt | 18:31 |
wrtp | the 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 |
niemeyer | wrtp: Cheers | 18:32 |
wrtp | niemeyer: 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 |
niemeyer | wrtp: You can retry 100 times too | 18:37 |
niemeyer | wrtp: no connotation of singularity there | 18:37 |
wrtp | niemeyer: indeed, but i want the name of the type to imply the whole thing, rather than one part of it. | 18:37 |
niemeyer | wrtp: Please name as you please :) | 18:37 |
wrtp | thanks | 18:38 |
niemeyer | hazmat: wtf was hung on bzr pull somehow. | 18:48 |
wrtp | niemeyer: here's what DNSName now looks like BTW. (untested) http://paste.ubuntu.com/874994/ | 18:53 |
wrtp | definite improvement from my previous version, i think. thanks for pushing me in this direction. | 18:53 |
niemeyer | wrtp: Much better indeed, thanks! | 18:54 |
niemeyer | wrtp: noDNS can die, btw | 18:54 |
wrtp | niemeyer: it's gone already | 18:54 |
niemeyer | wrtp: Cool, it was in the paste | 18:54 |
wrtp | niemeyer: yeah, i noticed it just afterwards | 18:54 |
wrtp | niemeyer: wouldn't've got through the compiler anyway | 18:55 |
wrtp | 80 lines of test code disappear, mmmm | 19:20 |
niemeyer | wrtp: 8) | 19:39 |
wrtp | niemeyer: hmm, goyaml seems to have broken | 19:40 |
niemeyer | wrtp: How so? charmstore depends on it, and apparently it compiled a few hours ago fine | 19:40 |
wrtp | niemeyer: i'll try updating | 19:41 |
niemeyer | wrtp: What's the error you got? | 19:41 |
wrtp | niemeyer: it's not unmarshalling correctly | 19:41 |
niemeyer | wrtp: Uh oh | 19:41 |
wrtp | niemeyer: http://paste.ubuntu.com/875062/ | 19:42 |
wrtp | niemeyer: lots of contents lost | 19:42 |
wrtp | niemeyer: seems like it's true of the latest revision (33) too. | 19:43 |
wrtp | niemeyer: i'll push go-ec2-robustness anyway, as this seems like an unrelated bug. | 19:44 |
niemeyer | wrtp: This doesn't even look like valid yaml | 19:45 |
niemeyer | wrtp: I'm surprised it doesn't return an error | 19:45 |
wrtp | niemeyer: oh. it was produced as output from goyaml itself | 19:45 |
wrtp | niemeyer: what's the problem with it? | 19:45 |
niemeyer | wrtp: Wait, my copy & paste is what's broken (at least) | 19:46 |
wrtp | :-) | 19:46 |
wrtp | sheesh, indentation-based nesting | 19:47 |
niemeyer | wrtp: Broken indeed.. checking it out | 20:00 |
wrtp | niemeyer: yeah, goyaml doesn't pass its tests | 20:00 |
niemeyer | wrtp, rogpeppe: goyaml was unintendedly exploring the mutability of interfaces | 21:31 |
niemeyer | Broke up after Russ fixed the bug | 21:31 |
rogpeppe | niemeyer: interesting | 21:33 |
rogpeppe | niemeyer: easy to fix? | 21:33 |
niemeyer | robbiew: Yeah | 21:41 |
niemeyer | Argh | 21:41 |
niemeyer | rog: Yeah | 21:42 |
robbiew | lol | 21:42 |
* robbiew was confused at first | 21:42 | |
rog | niemeyer: cool. | 21:42 |
rog | nie | 21:42 |
niemeyer | wrtp: Still facing network issues? | 21:53 |
niemeyer | rogpeppe: https://codereview.appspot.com/5784060 | 21:55 |
rogpeppe | niemeyer: LGTM | 22: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 |
niemeyer | fwereade_: Ah, no worries | 22:24 |
niemeyer | rogpeppe: Cheers, submitting | 22:24 |
niemeyer | Quite surprising how much goyaml has resisted without changing, given how much everything else broke in the Go 1 rush | 22:30 |
rogpeppe | niemeyer: have you done the goyaml tagging? | 22:31 |
niemeyer | rogpeppe: Just doing it | 22:31 |
rogpeppe | niemeyer: 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 |
niemeyer | rogpeppe: Done | 22:33 |
niemeyer | rogpeppe: Hmm, not sure I see why.. seems like a thin wrapper on the lxc command line | 22:34 |
rogpeppe | niemeyer: isn't the point of the package to implement an environs.Environ? | 22:35 |
rogpeppe | niemeyer: Container is just an implementation detail, i think | 22:35 |
niemeyer | rogpeppe: No, that's go/lxc | 22:35 |
niemeyer | rogpeppe: What you have in mind is go/environs/lxc | 22:36 |
niemeyer | Not even that, actually | 22:36 |
niemeyer | rogpeppe: it's go/environs/local | 22:36 |
niemeyer | lxc is a thing wrapper for management of lxc, independent from juju | 22:36 |
rogpeppe | niemeyer: oh. i think i misundersood originally. the current package path being reviewed is environs/lxc | 22:37 |
rogpeppe | niemeyer: (and given how small it is, i'm not sure if it needs its own package) | 22:37 |
niemeyer | rogpeppe: Ok, that's the problem then | 22:37 |
niemeyer | rogpeppe: Maybe.. | 22:37 |
niemeyer | rogpeppe: My plan was actually to move that out onto its own package | 22:38 |
niemeyer | rogpeppe: That said, you're right.. | 22:38 |
rogpeppe | niemeyer: 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 |
niemeyer | rogpeppe: We can keep that internal for the moment, and then make it public when/if we ever move it out | 22:38 |
rogpeppe | niemeyer: that seems good to me | 22:39 |
niemeyer | rogpeppe: goamz is its own package ;) | 22:39 |
rogpeppe | niemeyer: i meant to call the goamz/ec2 functions... | 22:39 |
niemeyer | rogpeppe: 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 juju | 22:39 |
niemeyer | rogpeppe: You're right that what we have there is small, though | 22:40 |
niemeyer | rogpeppe: So we can keep it internal for the moment | 22:40 |
rogpeppe | niemeyer: sure. but in the goamz/ec2 case it's doing real work. here it's almost trivial. | 22:40 |
rogpeppe | yeah | 22:40 |
rogpeppe | well, if it gets bigger, i'd be happy to factor it out | 22:40 |
niemeyer | rogpeppe: Can you please make the point in the review so that andrewsmedina can address it before we merge? | 22:42 |
rogpeppe | niemeyer: ok, i'm about to go to bed, but i'll write a brief reply. | 22:42 |
andrewsmedina | niemeyer: hi | 22:42 |
niemeyer | andrewsmedina: Heya | 22:42 |
niemeyer | andrewsmedina: You got a review | 22:42 |
andrewsmedina | niemeyer: yes | 22:42 |
andrewsmedina | niemeyer: 2 reviews | 22:43 |
niemeyer | andrewsmedina: and rog was just suggesting the Container type stay as "container" for the moment | 22:43 |
andrewsmedina | niemeyer: one for initial work with lxc in juju | 22:43 |
andrewsmedina | niemeyer: and another in goetveld https://codereview.appspot.com/5683064/ | 22:44 |
andrewsmedina | niemeyer: I just read your review | 22:46 |
niemeyer | andrewsmedina: This goetveld branch has been submitted two weeks ago.. ? | 22:47 |
niemeyer | andrewsmedina: Maybe I'm just missing the tag | 22:47 |
andrewsmedina | niemeyer: I can skip the tests if sudo isn't acessible | 22:47 |
niemeyer | andrewsmedina: That's what was suggested there | 22:48 |
andrewsmedina | niemeyer: I've updated the old review in goetveld | 22:48 |
niemeyer | andrewsmedina: But skip it at all times please, unless -lxc is provided | 22:48 |
niemeyer | andrewsmedina: Updated the old review!? | 22:48 |
andrewsmedina | niemeyer: I got issues to memorize the "goetveld" name | 22:49 |
niemeyer | andrewsmedina: This branch and CL in Rietveld have been submitted, which means they are effectively dead now | 22:49 |
niemeyer | andrewsmedina: If you have more changes, they must be proposed in a different branch/CL | 22:49 |
andrewsmedina | niemeyer: I will create another branch then | 22:50 |
rogpeppe | andrewsmedina: you've got another review | 22:51 |
niemeyer | andrewsmedina: Thanks! | 22:51 |
andrewsmedina | rogpeppe: thanks | 22:51 |
rogpeppe | off to bed now. 'night all. | 22:51 |
andrewsmedina | I'm very happy to contribute with juju and others projects in go | 22:51 |
andrewsmedina | rogpeppe: night | 22:52 |
rogpeppe | andrewsmedina: it's much appreciated! | 22:52 |
fwereade_ | niemeyer, https://codereview.appspot.com/5672067 may actually be done now :) | 22:55 |
niemeyer | fwereade_: Cheers :) | 22:58 |
niemeyer | fwereade_: I may only get to it in the morning as well, though | 22:58 |
niemeyer | It's getting late here | 22:58 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!