hazmatdavecheney, the newer samsung ssds (830?) are 7mm01:50
davecheneyhazmat: yeah, i'll have to get one of those01:51
davecheneyhazmat: for a moment I considered taking the metal case off my sandforce01:51
davecheneybut only for a moment01:51
TheMuedavecheney, fwereade, rogpeppe: Morning.06:30
fwereadeTheMue, heyhey06:30
fwereadeand everyone else :)06:30
bigjoolsfwereade: o/06:32
fwereadebigjools, heyhey06:33
bigjoolswho do I bug to get my branches reviewed today? :)06:33
fwereadebigjools, consider me bugged, I'll do that now :)06:34
bigjoolstip top, thanks06:34
fwereadebigjools, one minor on https://code.launchpad.net/~julian-edwards/juju/maas-provider-non-mandatory-port/+merge/10757706:43
fwereadebigjools, just _s for unused vars06:43
fwereadebigjools, otherwise both LGTM06:44
fwereadehazmat, ping06:44
bigjoolsfwereade: is that some juju convention?06:44
bigjools(for reference)06:44
fwereadebigjools, my heart says it is, but I couldn't point you to a document saying so06:45
bigjoolsfwereade: so: _s, port, _s = connect_call.args06:45
fwereadebigjools, soory, _06:46
fwereadebigjools, I should have said `_`s or something06:46
bigjoolsright, makes more sense now!06:46
bigjoolsfwereade: ok change pushed up, thanks muchly06:47
fwereadebigjools, also I'm not sure what the current datekeeper rules are for python; I'll have a word with hazmat when he's around and either extract more approves if required, or just merge them06:47
bigjoolsyou will land them right?06:47
fwereadebigjools, (I should probably just land them but I'm reluctant to come charging back into the python without checking)06:49
* bigjools nods06:49
fwereadebcsaller, ping06:59
hazmatfwereade, pong07:00
davecheneyfwereade: rogpeppe TheMue does anyone have any comments about testing commands and package main_test ?07:14
fwereadehazmat, sorry: what are the current landing requirements for juju? bigjools has a couple of patches I'm happy with myself... what else should be done before landing?07:15
hazmatfwereade, the sru just landed07:16
hazmatfor the connect args, i think in this case its actually more helpful to have the args documented.. its a completely mocked situation07:16
fwereadedavecheney, I'm not sure my own preferences are useful... main_test seems to me like the obvious and sane thing to do but if it's flaky I guess we should do things differently07:17
fwereadehazmat, heh, sorry07:17
hazmatfwereade, and those params are never checked anywhere else in the codebase07:17
TheMuedavecheney: Not yet. So far I had no main commands to test.07:17
hazmatso its completely unclear what they are if their ellidded07:17
hazmatbigjools, ^ sorry07:17
hazmatbigjools, actually i'd take the reverse notion.. and actually verify those values07:19
davecheneyfwereade: for a long time gotest didn't work with main packages07:19
hazmatsince their not really checked otherwise07:19
hazmatwhich is the root cause of this bug07:19
davecheneyas they are the end of the line, dependency wise07:20
davecheneyall the other code i've found that does test main, tests it inside package main07:20
davecheneyi don't think there is a problem doing unit tests inside the package you are testing07:20
* hazmat checks tests_auth07:20
fwereadedavecheney, putting the tests in the package you're testing feels icky to me; but like I said, if main_test is flaky and not expected to be fixed any time soon, I guess we should just change them07:21
davecheneyfwereade: you aren't putting them in the same package, well, only when testing, but there is no other way to test unexported functions07:23
davecheneyanyway, i've got to go07:24
davecheneyi'll see you on the flip side07:24
hazmatfwereade, trunk is green for this fix07:26
fwereadehazmat, ok then, I'll land them both once the change you asked for is done07:29
fwereadehazmat, cheers07:29
hazmatfwereade, thanks07:29
rogpeppefwereade, TheMue, hazmat: mornin' all07:36
fwereaderogpeppe, heyhey07:37
hazmatg'night all07:37
bigjoolshazmat: ok I'll reverse the last commit07:45
bigjoolsI did think it was rather odd to remove useful variable names :)07:45
hazmatwhen their well known and unused, we do tend to elide them.. but this is a special case.. since their effectively unknown.07:46
bigjoolswell they are test case logs07:47
bigjoolsthe other ones are tested elsewhere07:47
bigjoolshazmat: ok pushed up07:48
hazmatbigjools, are they? :-)07:48
hazmatbigjools, i couldn't find anywhere else where args was looked at07:48
hazmatbigjools, thanks07:49
bigjoolsI t hought I'd seen it somewhere07:50
bigjoolsmy brain ceased to function 30 minutes ago, it's been a long day07:50
bigjoolshazmat: ah just saw your paste, I'll poke that in too, one sec07:51
* hazmat gives up on going back to sleep07:53
bigjoolsand pushed07:53
* bigjools looks forward to a merge email07:54
hazmatbigjools, shouldn't this also do port 443 for https?08:53
rogpeppefwereade: i've been looking at CL 6244060. i *think* that having multiple zk Servers sitting around in dummy isn't a good idea.08:58
fwereaderogpeppe, I'm somewhat ambivalent, but I think in practice that when one is added we will remember to defer a removal as well08:59
rogpeppefwereade: i think we want to reuse the zk server regardless of the name that the test gives to the environment08:59
rogpeppefwereade: how do we remove a server?09:00
fwereaderogpeppe, I guess doing it this way does effectively force us to use one dummy env name per package, which prevents us from encoding any helpful context in there09:00
fwereaderogpeppe, SetZookeeper("foo", nil)09:00
rogpeppefwereade: that too09:00
rogpeppefwereade: and if we're going to do that, why bother allowing multiple zk servers09:01
rogpeppethere's an alternative, actually09:02
fwereaderogpeppe, it seemed sensible to do that regardless... I think the issues are orthogonal?09:02
fwereadego on09:02
rogpeppefwereade: hmm, another thought: why do we bother with SetZookeeper. why don't we let the dummy environ start its own zk?09:04
rogpeppefwereade: then the dummy environ can manage the server "cache". (it could keep just one server around after a Reset)09:04
fwereaderogpeppe, hmm, how do we know when to shut the servers down?09:05
rogpeppefwereade: oh yeah, good point. *that* was why :-)09:05
rogpeppefwereade: anyway, i *think* we want to be able to reuse a zookeeper regardless of the name of the environ.09:07
fwereaderogpeppe, tbh that was my initial preference but we do seem to have multiple storages (right?) and consistency seems sensible09:08
rogpeppefwereade: storage is *much* lighter weight than zk. and we trash all the storages after Reset.09:09
rogpeppefwereade: i think the 10s startup overhead of zk means we will never want to start up two09:09
fwereaderogpeppe, my instinct says that indeed we won't, and that it will be much easier to note this and remove the capacity at some point in the future than it will to get into heavy discussions with niemeyer about it right now09:11
fwereaderogpeppe, my primary consideration, sad to say, is getting it merged so I can get to work on deploy without it being at the end of a looong pipeline of CLs under discussion ;)09:11
niemeyerGood mornings!09:12
fwereadeniemeyer, heyhey!09:12
rogpeppefwereade: i'm concerned that this kinda breaks the dummy environs model09:12
rogpeppeniemeyer: yo!09:12
rogpeppeniemeyer: just expressing mild concern about SetZookeeper taking an environ name09:12
rogpeppeniemeyer: we want the zk to be reused regardless of the name of the environment that's opened, i think.09:13
niemeyerfwereade, rogpeppe: No need to get into heavy discussions.. it was just an opinion.. happy to be shown the other side of it :)09:13
niemeyerrogpeppe: So what's the point of having environment names in the dummy provider?09:13
fwereadeniemeyer, my personal view is that the cost of either approach is dwarfed by the cost of talking about it for more than 20s ;)09:13
niemeyerrogpeppe: Environment names explicitly enable talking to completely different environments09:14
rogpeppeniemeyer: so we can open multiple environments. but Reset should reset everything09:14
niemeyerrogpeppe: If we have two different environments that have different internal representations but that in fact are the same ZooKeeper state, isn't it the same environment?09:14
niemeyerrogpeppe: Agreed, but if we have a single ZooKeeper state, it's not multiple environments09:14
rogpeppeniemeyer: we weren't allowing that09:14
rogpeppeniemeyer: we weren't going to allow multiple zk environments09:15
rogpeppeniemeyer: because i don't think we'll ever actually want that09:15
niemeyerrogpeppe: There's no such thing as a "zk environment"..  an environment contains a ZooKeeper09:15
rogpeppeniemeyer: lots of tests use the dummy env without requiring zk09:15
niemeyerrogpeppe: If we have two environments with one zookeeper, we don't have two environments09:15
niemeyerrogpeppe: Yep, that sounds fine09:16
rogpeppeniemeyer: what sounds fine?09:16
niemeyer<rogpeppe> niemeyer: lots of tests use the dummy env without requiring zk09:16
niemeyerfwereade: Do we have a use case for opening two environments with a single ZooKeeper?09:17
fwereadeniemeyer, I don't think so09:17
rogpeppeniemeyer: at the same time, you mean?09:17
niemeyerfwereade: So what's the concern?09:17
niemeyerfwereade: I'm concern we're modeling something unrealistic in the dummy environment09:18
rogpeppeniemeyer: we don't want to restart zk every time we start a test.09:18
niemeyerfwereade: But doing dummy.SetZooKeeper("name", zk) vs dummy.SetZooKeeper(zk) execute in the same amount of test09:18
niemeyerof time09:18
fwereadeniemeyer, from my perspective, that it's just more bookkeeping... I expect to run a bunch of sequential tests, each of which uses a dummy env, and which in practice may as well all use the same ZK09:18
niemeyerrogpeppe: Yep.. agreed09:18
niemeyerfwereade: Sounds fine too?09:19
fwereadeniemeyer, doing this effectively means we either need multiple zookeepers, or to explicitly set multiple envs to use the same one, if we ever want to use different env names09:19
fwereadeniemeyer, but I really have no horse in this race, I think the cost is minimal either way and I'm happy enough with either approach09:20
niemeyerfwereade: The cost is zero if we're never doing that.. and if we ever do it we can consciously choose to assign to the same ZooKeeper, right?09:20
niemeyerfwereade: I guess I'm still lacking what's the argument against making the interface of the dummy provider a bit more realistic in terms of what happens09:21
niemeyerfwereade: We're debating about SetZooKeeper(name, zk) vs. SetZooKeeper(zk), right?09:21
rogpeppeniemeyer: i like the fact that dummy.Reset forgets all environ names09:22
fwereadeniemeyer, the cost is (1) a little bit of extra bookkeeping code and (2) consciously choosing to set up extra zookeepers in the package test function09:22
niemeyerrogpeppe: Sure, so continue doing that.. (?)09:22
rogpeppeniemeyer: but if we add SetZooKeeper(name, zk) that name remains around09:22
rogpeppeniemeyer: even after Reset09:22
fwereadeniemeyer, like I say, not enough for me to worry about09:22
niemeyerrogpeppe: I don't get it.. are you suggesting that Reset() won't reset what SetZooKeeper does? That sounds weird09:23
rogpeppeniemeyer: if it does reset it, then we'll need to call SetZooKeeper on every test09:23
fwereadeniemeyer, ah, hmm, so Reset() should clear all zookeepers and each test should explicitly set one at the start?09:23
fwereadeniemeyer, that does feel heavyweight to me09:23
rogpeppeniemeyer: perhaps that's what we should be doing i guess09:23
fwereadeniemeyer, having the magic test ZK in python felt like a win to me tbh09:24
niemeyerfwereade: Assigning an entry to a map sounds heavyweight?09:24
niemeyerfwereade: Reset resets everything else in Dummy.. why is it not acting on what SetZooKeeper does?09:24
fwereadeniemeyer, keeping track of the zookeeper in each test package, rather than just in the package setup func09:24
fwereadeniemeyer, Reset trashes the content of every active zookeeper09:25
fwereadeniemeyer, what I'm trying to avoid is starting a fresh ZK for every test09:25
niemeyerfwereade: You don't have to start a fresh zookeeper.. trashing content != starting zookeeper09:25
rogpeppeniemeyer: the idea behind SetZooKeeper was "here is a zookeeper server; use it for a zk environment whenever it's opened"09:25
niemeyerfwereade: Each test *should* get a zeroed out zookeeper, right?09:25
fwereadeniemeyer, agreed; and it does09:25
niemeyerfwereade: Ok, so I misunderstand your point09:26
fwereadeniemeyer, what rogpeppe said^^09:26
niemeyerrogpeppe, fwereade: Sure.. how does that change what was just pointed out, thought?09:26
fwereadeniemeyer, I think that allowing different ZKs for different envs is fine; I'm not sure we'll use it much in practice09:26
niemeyerfwereade: Agreed.. I'm not arguing that we use it.. I'm arguing that we don't have unrealistic scenarios for testing09:27
niemeyerfwereade: Two different environments in a configuration, with different names, will have different backing ZooKeepers09:27
fwereadeniemeyer, and I can get behind that; I've proposed a CL that, I think, does make the scenario more realistic as you suggest, and I'm perfectly happy with it :)09:28
rogpeppeniemeyer: if we have Reset clear out all the zk servers too, then every test will need to call SetZooKeeper, which means that every test suite will need a zk server field. if we have just a single zk server instance, then we can do the setup exactly as we do in state.TestPackage09:28
rogpeppeniemeyer: will we ever want to have two different environments concurrently with different zookeepers?09:29
rogpeppeniemeyer: it's a heavy weight test and one i can't really see a reason for doing09:29
fwereadeniemeyer, I'm not so keen on the idea of having a zkServer package var for every test package, and explicitly setting it as the dummy zookeeper in every test; is that what you're proposing?09:29
rogpeppeniemeyer: if you never want to do that (and you can make it panic if that ever happens accidentally) then the scheme is not unrealistic - it's indistinguishable, i think.09:30
niemeyerrogpeppe: <niemeyer> fwereade: Two different environments in a configuration, with different names, will have different backing ZooKeepers09:30
niemeyerrogpeppe: That's how environments work for real09:30
rogpeppeniemeyer: yes, but will we *ever* want to open two such environments *at the same time* in a test?09:31
niemeyerrogpeppe: If we're creating a dummy provider that works in a different way, it sounds like we're doing something wrong..09:31
niemeyerrogpeppe: You were the one changing the dummy package so it could take multiple environments with different names.. (!?)09:32
rogpeppeniemeyer: that's for basic tests that don't require zk.09:32
niemeyer<rogpeppe> niemeyer: yes, but will we *ever* want to open two such environments *at the same time* in a test?09:32
niemeyerrogpeppe: That does answer your own question09:33
rogpeppeniemeyer: by "such environments" i meant environments with a backing zk instance09:33
niemeyerrogpeppe: We're going in circles. I've already pointed out the reasoning behind that. You can agree or disagree.09:34
rogpeppeniemeyer: the approach i'm suggesting is the same one taken by state, for instance - we have a single zk instance that is reused in each test.09:34
niemeyerrogpeppe: That's where our conversation started.09:34
rogpeppeniemeyer: so... should Reset forget the servers registered with SetZooKeeper?09:35
niemeyerrogpeppe: That was my expectation, but I guess the dummy package is being used in a different way than what I expected09:36
rogpeppeniemeyer: if so, i could live with that. it's another line in each test or suite that opens a zk environ, but the zookeeper.Server could live in a global var just like state.TestingZkAddr.09:37
niemeyerrogpeppe, fwereade: So far we set up the dummy package on each suite, I think09:37
fwereadeniemeyer, ATM we set it up per package09:38
fwereadeniemeyer, following the model in state09:38
niemeyerfwereade: The dummy package?  Do you have an example where we set it up per package I could look at in the current source?09:39
rogpeppecmd/juju/cmd_test.go sets it up per test09:40
niemeyerRight, I meant an example where we use the dummy provider on a package context, rather than per test or suite09:41
fwereadeniemeyer, only in https://codereview.appspot.com/6243067/ which technically doesn't exist yet09:41
fwereadeniemeyer, (that's calling SetZookeeper in the package test func; did you mean something different?)09:42
niemeyerfwereade: Right, so that's why I was assuming a given way of using it, which doesn't match what you had in mind.. not saying either is right or wrong, but part of the argument originates there09:42
fwereadeniemeyer, I'm just monkey-see-monkey-doing what we did for state wrt zookeepers; seems to work ok ;)09:42
* fwereade wonders whether "following established conventions" is a better way of putting that ;)09:43
niemeyerfwereade: Yeah, you could have monkey-see-monkey-done what the dummy package currently do now as well, though :-)09:43
fwereadeniemeyer, so it seems; state is the one I already knew :009:44
rogpeppefwereade: "blindly following over a cliff"?09:44
fwereaderogpeppe, if all your friends jumped off a cliff and it all seemed to be going just fine...09:44
rogpeppefwereade: come on in, the water's lovely!09:45
niemeyerfwereade: Ok, let's just move back to the SetZooKeeper you had then.. it's very straightforward and will get you going09:46
fwereadeniemeyer, the cost of setting up a ZK was a contributing factor... if that weren't a consideration I'd happily do everything per-suite09:46
rogpeppeanyway, to get back to the point. i think i'd be happy with either: one zk instance, reused automatically, as originally; or an explicit SetZookeeper required after Reset09:46
fwereadeniemeyer, ok, cool09:46
fwereadeniemeyer, cheers :)09:46
niemeyerfwereade: I'm happy to delay my preciousism in the design (if that's even a word) to a second moment09:46
fwereadeniemeyer, and a panic if anyone tries to SetZookeeper while one is already set?09:46
niemeyerfwereade: The fact it matches what's being done with zookeeper now in terms of the test suite is what makes me more comfortable09:47
rogpeppeniemeyer, fwereade: thanks for bearing with me. i feel like i've been the precious one :-)09:47
rogpeppefwereade: i don't see why we shouldn't allow SetZookeeper twice.09:47
niemeyerfwereade: Not sure.. how do you plan to reset the zk?09:48
fwereaderogpeppe, ah ok, perhaps I misremembered an earlier comment of yours09:48
niemeyerfwereade: as in, how do you plan to unset the package variable09:48
fwereadeniemeyer, `defer SetZookeeper(nil)` in the package test func; clear contents on Reset() or Destroy()09:48
niemeyerfwereade: Ah, ok.. so accept nil, but not another zk09:48
niemeyerAnyway, I have to move rooms09:48
fwereadeniemeyer, yeah09:48
niemeyerWhich means I have to shutdown09:49
rogpeppefwereade: what bad thing might happen if you allowed setting another zk?09:50
fwereaderogpeppe, having two zookeepers around :p09:50
fwereaderogpeppe, if someone comes up with a legitimate use case removing the panic is not hard09:50
fwereaderogpeppe, but trying to do so indicates to me that something is likely to be being Done Wrong09:51
rogpeppefwereade: i'm not sure i see the big difference between { SetZookeeper(nil); SetZookeeper(anotherZk)} and SetZookeeper(anotherZk)09:52
rogpeppefwereade: but i don't mind actually. require the SetZookeeper(nil) if you prefer.09:53
fwereaderogpeppe, in either case, assuming package-level setup, either you need to keep track of the original package ZK *or* someone has failed to clear it out from a previous package09:53
rogpeppefwereade: i can't see how that would happen. we only test one package at a time, right?09:54
fwereaderogpeppe, and if a server from a previous package is still there, someone's ballsed something up09:55
rogpeppefwereade: so it would only happen if we happened to call SetZookeeper twice in the same package. which could happen, i guess, but i don't see it as an easy-to-find failure mode.09:55
fwereaderogpeppe, fair enough :)09:55
rogpeppefwereade: how could a server from a previous package still be there? they're different programs?09:55
* fwereade waves his hands vaguely, then runs out of steam and slinks off lokking embarrassed09:56
fwereaderogpeppe, should Destroy be clearing out the appropriate key in providerInstance.state (and closing the listener)?10:06
rogpeppefwereade: erm, zookeeper.Server.Destroy?10:07
fwereaderogpeppe, sorry, dummy.environ.Destroy10:07
rogpeppefwereade: i don't see that method. do you mean Reset?10:08
fwereaderogpeppe, Reset clears them all10:08
rogpeppejust grepped the wrong dir10:08
rogpeppei don't *think* so10:09
rogpeppefwereade: it's not like s3 goes away when you destroy an environment10:09
rogpeppefwereade: i think it's ok for Reset to do that job10:10
rogpeppeAram: yo!10:11
Arammorni g all.10:11
fwereaderogpeppe, I'll read some more, see if I can figure out why your proposed tweak to open panics... that was just a half-formed hypothesis10:11
TheMueAram: Hi10:12
rogpeppefwereade: it could be that something's not calling Reset10:12
fwereadeAram, heyhey10:12
fwereaderogpeppe, HAHA DISREGARD THAT I wrote the code in the wrong place :/10:14
* rogpeppe disregards it10:14
rogpeppefwereade, TheMue, niemeyer: i'm wondering about the way that we manage the state with watchers. if i'm watching something, i'll get a notification that something has happened to it (because its config node changed, for example) but then i have to re-read the same info from zk if i want to find out attributes of the object. but the info might have changed - the info i see might not be consistent with what i'm told10:50
rogpeppei'm wondering if a model where each kind of thing (Unit, Service, Machine) caches the info that was known when it was created.10:51
rogpeppes/if a/about a/10:51
TheMuerogpeppe: Typically the specialized watchers return the needed info.10:51
TheMuerogpeppe: So in your case a watcher returns an info but you need a different one depending on it?10:52
rogpeppeTheMue: i'm looking at doing a unit watcher. the obvious thing to do is have the channel sending *Units that have been added or removed.10:52
niemeyerrogpeppe: That's what the machine watcher does..10:52
rogpeppeTheMue: but if i want to get any info out of those units, i have to reread it10:53
rogpeppeniemeyer: yeah, and i'm a bit concerned about it10:53
niemeyerrogpeppe: No, you're not re-reading.. you're reading.. the information out of those units wasn't read before10:53
niemeyerrogpeppe: There are different ways to model that problem for sure10:53
niemeyerrogpeppe: And perhaps the model we have in place today isn't the best one, but changing it will incur in other issues10:54
rogpeppeniemeyer: ah, that's true. the info *about* the units is in the topology, but the info *of* the units is in the unit's config node, right?10:54
niemeyerrogpeppe: Yep10:54
niemeyerrogpeppe: So, in either case, I suggest not modifying that today so that we can move forward10:54
TheMuerogpeppe: Nice description, yes.10:54
niemeyerrogpeppe: I do think eventually we'll want to cache more than we do, but that can't be done without pondering further about the algorithms10:56
rogpeppeniemeyer: it doesn't seem that that's entirely true. i'm looking at AssignedMachineId and it reads the topology. to find new units, i'm going to need to watch the toplogy. so i'll be reading the toplogy for every unit as well.10:56
TheMueniemeyer: The test of store fails for me due to a not matching error message in lpad_test.go:42:. Is that only here or well known?10:57
niemeyerrogpeppe: Yeah, it's a mix of both10:57
niemeyerTheMue: Certainly not well known by me.. trunk should never be broken10:59
rogpeppea possible model to consider for the future: each object acts as a local cache of some state. it only goes to zk when it doesn't already hold that state. an Update method could be implemented on each type to fetch new values from zk.11:00
niemeyerrogpeppe: Yeah11:00
rogpeppeTheMue: tests all pass for me11:00
TheMueniemeyer: Ah, found the error. It's the locale. So bzr returns a German message here while an English one is expected in the test. (sigh)11:01
rogpeppeniemeyer: that way most methods could lose their error return perhaps - the only place returning an error would be on Update.11:01
TheMuerogpeppe: See above, it's my locale.11:01
rogpeppeTheMue: ah. useful to have someone in a different language locale!11:02
TheMuerogpeppe: Yes. While our own messages are all English the ones of external tools may vary.11:02
niemeyerrogpeppe: and then all writes have to be cached as well?  That would mean the flush operation could fail, and we wouldn't know why.. which means we'd have to implement retry points that don't really know what is being retried11:03
niemeyerrogpeppe: etc11:03
rogpeppeniemeyer: no, i think writes would just write through11:04
niemeyerrogpeppe: It's a different model.. worth thinking about at some point.. not obviously better11:04
TheMuerogpeppe: As long as we don't have any problem with re-reading I won't do an application based caching. This may be task of the backend (like ZK, Mongo, whatelse).11:05
rogpeppeniemeyer: it's this kind of thing that concerns me: client A watches units. client B changes unit 1 to X. client A observes change X. client B changes unit 1 to Y. client A reads Y. client A observes change Y. client A reads Y.11:09
niemeyerrogpeppe: How's that an issue?11:10
rogpeppeniemeyer: it's very inefficient, particular when we're observing the topology and it's getting large.11:11
rogpeppeniemeyer: maybe i shouldn't worry about efficiency here though.11:11
niemeyerrogpeppe: Yeah, let's make it work first11:12
niemeyerrogpeppe: Reliably11:12
niemeyerrogpeppe: Note that, in your example, even if we cached the data and client B read X instead, there would be a second notification11:13
rogpeppeniemeyer: it's reliability i'm concerned about too. working on a slightly newer version than we've just been told about. maybe it's not an issue, but it seems potentially problematic.11:13
niemeyerrogpeppe: We haven't been told about anything.. we have simply been notified that a changed happened11:13
rogpeppeniemeyer: yeah, there are two changes, so there would be two notifications11:13
rogpeppeniemeyer: we've been told that some new units have been created, for example.11:14
niemeyerrogpeppe: Yes, we have, but that's unrelated to your example11:14
rogpeppeniemeyer: and we *did* know some info about those units, but we've thrown it away11:14
niemeyerrogpeppe: Yes, but that's unrelated to reliability11:14
rogpeppethe caching model seems to me like it has less potential for odd race conditions.11:16
rogpeppeniemeyer: anyway, food for thought.11:18
hazmatrogpeppe,  agreed it is inefficient11:18
hazmatrogpeppe, the other options are to have some notion of logical transactions that the topology records for11:19
hazmatnot nesc. the 3.4 multi-node txn, but a logical  one where we carry the topology across multiple ops11:19
hazmatjust to be clear its not a race condition per se that its avoiding, its the spurious notifications caused by every state change, that may actually compose a larger logical op11:19
hazmatbut really the largest source of spurious notifies, is everything listening to the topology, getting hit by notifications they don't care about11:20
rogpeppehazmat: yeah that's true11:20
niemeyerrogpeppe: Indeed, I do think those are good ideas, and that there's something there for us to evolve towards11:21
rogpeppehazmat: but it doesn't help when we've been listening to the topology, we're told that it's changed and what it changed to, and then we do a round trip just to fetch it again.11:21
niemeyerrogpeppe: We quickly did that when we started.. but didn't quite work well11:22
niemeyerLuncnh time11:22
rogpeppeniemeyer: did what, sorry?11:22
hazmatas an alternative i think we either need to carefully devolve into multiple topo indexes, and/or starting using multi-node transactions..11:23
hazmatrogpeppe, that's the nature of watches11:23
hazmatrogpeppe, oh.. we got told what it changed to?11:24
rogpeppehazmat: ah, no, of course. in this case we're doing a round trip to fetch it. and then another round trip to fetch it again.11:24
hazmatrogpeppe, ah11:25
hazmatrogpeppe, we did briefly experiment with an up to date object cache, but lacking transactions or pub/sub events (from watches), it ended up just being more complexity imo.11:26
hazmatrogpeppe, but if you could interject into the watch channel to an object cache invalidation protocol it might have some value, but effectively anything in cache would need to be watched11:27
hazmatelse it could be stale11:27
rogpeppehazmat: i wasn't thinking of a global cache, just a local cache for an object when you read it.11:27
hazmatalternatively work with an operation context, like a transaction scope11:27
hazmater. scoped cache11:27
rogpeppehazmat: and a way of saying "please update"11:28
hazmatrogpeppe, yeah.. that makes sense, but speaking of race conditions ;-)11:28
hazmatits unavoidable i suppose, the local cache gives a consistent view at least11:28
rogpeppehazmat: yeah, that's my thought11:28
hazmatwe always have to consider the possibility of state change11:28
rogpeppehazmat: indeed.11:29
hazmatbut if we go to modify a cached obj, we have to reconcile current state and modifications, not cache state and mods11:29
rogpeppehazmat: but it can change when we ask it to11:29
hazmatie. capture versions11:29
hazmatat a min or use retry change utilities11:29
rogpeppehazmat: i'd be happy with a write-through cache11:29
rogpeppehazmat: it doesn't solve all the problems, but it alleviates the worst11:30
hazmatniemeyer, are you in uk?11:30
rogpeppehazmat: he is11:30
* hazmat notes its not lunch time yet ;-)11:30
hazmatrogpeppe, cool, is there a gojuju sprint going on?11:30
rogpeppehazmat: nope11:30
rogpeppehazmat: niemeyer's at some sprint in london11:31
hazmatwe should get a juju sprint  going on11:31
hazmati can't even remember the last sprint we had11:31
hazmatlike august of last year maybe..11:31
rogpeppehazmat: +111:31
rogpeppehazmat: i haven't done a sprint yet11:32
hazmatwe should fix that..11:32
TheMuerogpeppe, hazmat: Isn't something planned when Mark is on board?11:32
hazmati dunno11:32
niemeyerrogpeppe: ping12:52
rogpeppeniemeyer: pong12:53
niemeyerrogpeppe: Yo12:53
niemeyerrogpeppe: Are you game to come tomorrow and return on Friday?12:53
rogpeppeniemeyer: i think so12:53
niemeyerrogpeppe: Awesome, sorting out hotel12:53
rogpeppeniemeyer: will just have a word with carmen12:53
niemeyerrogpeppe: Cheers12:53
fwereaderogpeppe, any particular reason StorageReader.URL doesn't return a url.URL?12:57
Aramniemeyer: eager to meet you tomorrow!12:58
Aramanyone else there?12:58
niemeyerAram: Looking forward too12:58
niemeyerAram: Lots of people, but no jujuers unfortunately12:58
niemeyerAram: rog should come, thogh12:59
niemeyerrogpeppe: Are we good to go?13:35
fwereadeniemeyer, do I recall you saying we should revisit the way we handle unit placement in python?13:51
fwereadeniemeyer, because we're aproaching the point at which we should be considering it :)13:51
niemeyerfwereade: Hmm.. not that I remember off hand13:51
fwereadeniemeyer, jolly good then -- if it crosses your mind again, do let me know (I didn't see anything wrong with it myself, but...)13:52
TheMueniemeyer: I now start the go test with a temporary locale en_US. But now I've got this error: http://paste.ubuntu.com/1014818/14:04
rogpeppeniemeyer: yes.14:06
rogpeppeniemeyer: sorry about the wait. got into an involved discussio.n14:06
niemeyerrogpeppe: COol, thanks14:07
niemeyerrogpeppe: Moving forward then14:07
rogpeppefwereade: there's no need for it to be a parsed URL. http.Get doesn't take a url.URL as an argument.14:07
niemeyerTheMue: Sorry, can't look at that now14:07
TheMueniemeyer: OK, no problem so far and doesn't hinder state development. ;)14:08
rogpeppefwereade: a string seems a fine representation of a URL to me if you don't need to be looking inside it.14:08
fwereaderogpeppe, ah, ok; in that case the question becomes: TheMue, is there any particular reason AddCharm takes a url.URL?14:09
TheMuefwereade: Have to think back. IMHO because I'm getting it from an API call. One moment, I'll take a look.14:10
fwereadeTheMue, thanks14:10
rogpeppefwereade, TheMue: it looks like it could as well be a string, as all it does is call String on it...14:10
rogpeppefwereade, TheMue: and charmData.BundleURL is a string14:11
fwereaderogpeppe, concur14:11
TheMuefwereade: Don't know the reason anymore, it became a URL during the discussion.14:15
fwereadeTheMue, rogpeppe: any reason not to make it a string again?14:16
TheMuefwereade: Are there any problems with URL?14:16
rogpeppeTheMue: it's worth using the same representation for the same thing throughout, i'd say14:16
fwereadeTheMue, just that it seems kinda silly to turn a string into a url just to turn it back into a string a few us later :)14:16
rogpeppefwereade: +114:17
TheMuerogpeppe: Same representation for the same thing: yes. A URL is a URL. ;)14:18
TheMuerogpeppe: But it's ok for me.14:18
TheMuerogpeppe: Can parse it to URL if access to the parts is needed.14:19
rogpeppeTheMue: yeah14:19
TheMuerogpeppe: The only thing is: URL verifies the URL to have a valid scheme.14:19
rogpeppeTheMue: i don't think so14:21
rogpeppeTheMue: it just verifies that the scheme matches [a-zA-Z][a-zA-Z0-9+-.]*14:21
TheMuerogpeppe: OK, not much (but even more than a pure string).14:22
TheMuerogpeppe: So AddCharm() gets a charm URL as charm.URL but the bundle URL as string. Hmm.14:24
rogpeppeTheMue: assuming we're expecting to have got the charm URL from the charm package, i don't think that's an issue14:27
TheMuerogpeppe: Yes, no issue, it only 'smells' a bit when reading the function signature.14:28
rogpeppeTheMue: can you enlighten me as to the role of unit names?14:38
rogpeppeTheMue: i can't quite see a unit has both a name and a sequence number14:38
rogpeppeTheMue: and a key14:38
TheMuerogpeppe: One moment.14:39
TheMuerogpeppe: The name is build by service name and sequence number.14:41
TheMuerogpeppe: The key is the internal key in ZK, like unit-0000000001.14:41
rogpeppeTheMue: i don't quite see why key and sequence number aren't pretty much the same thing in different forms14:43
rogpeppeah! i see it, i think14:43
rogpeppeTheMue: i *think* it's because a unit is built in two operations; make unit directory, then add the newly made name to the topology.14:44
rogpeppeTheMue: i'm still not quite sure why we can't just take the unit number from the unit directory name.14:44
TheMuerogpeppe: Yes14:45
rogpeppeTheMue: and live with the fact that unit numbers might sometimes skip14:45
rogpeppefwereade: any idea?14:45
TheMuerogpeppe: Do you have any trouble with it?14:46
rogpeppeTheMue: yeah, it's awkward because i'm implementing topology.UnitsForMachine and i'm not sure what to return from it14:46
fwereaderogpeppe, TheMue: I'm torn14:46
rogpeppeTheMue: i can't return []*zkUnit because that doesn't include the service name14:47
rogpeppeTheMue: i can't return *Unit because that's outside the purview of topology.go14:47
TheMuerogpeppe: The unit stuff is almost a 1:1 of the Py code.14:47
fwereaderogpeppe, TheMue: smoothly increasing ids throughout would obviously be nicest, and not represent a clear encapsulation breakdown14:47
TheMuerogpeppe: So I would investigate there.14:48
rogpeppeTheMue: i don't want to define *another* type representing a unit14:48
rogpeppefwereade: that's what we've got now, i think.14:48
rogpeppefwereade: at least, smoothly increasing *external* ids14:48
fwereaderogpeppe, TheMue: cool, because that's what we have with machines for sure14:48
rogpeppefwereade: exactly.14:49
fwereaderogpeppe, TheMue: and in this I favour consistency14:49
rogpeppefwereade: i'm thinking that machines is doing a very similar thing, and there (i *think) we have a 1-1 correspondence between key and machine id14:49
fwereaderogpeppe, yep, I think so too14:49
TheMuerogpeppe: The sequence no is increasing per service, the unit key over all units.14:50
rogpeppefwereade, TheMue: doing this would also mean we could get rid of all the UnitSequence logic in topology.go14:50
rogpeppeTheMue: i don't think so, but let me check14:50
rogpeppeTheMue: oh yeah14:51
TheMuerogpeppe: It is, just checkt. It increases a counter per service name in topology.14:51
rogpeppeTheMue: that's a bit odd.14:51
rogpeppeTheMue: i'm surprised it's not a directory inside the service.14:51
fwereaderogpeppe, hmm, yeah, that might be nicer indeed14:51
TheMuerogpeppe: It's a value of topology, maybe because of the 'tranctional' behavior.14:52
rogpeppeTheMue: it's both actually14:52
rogpeppeTheMue: well... it's a directory and a node inside the topology14:52
TheMuerogpeppe: The increment only happens when the topology change succeeds.14:52
rogpeppeTheMue: the directory is inside /units though14:52
TheMuerogpeppe: The unit node is below units, yes.14:53
rogpeppeTheMue: yeah. but if the directory was inside the service, we could use a zk auto-increment to do the counting for us14:53
rogpeppeTheMue: but perhaps there's another reason for having units in a global namespace14:54
TheMuerogpeppe: We can do a lot. Do we have the need while the state isn't even completed?14:54
rogpeppeTheMue: probably not. it would simplify the code a bit though.14:55
hazmatits to give a semantic useful number14:55
hazmatzk sequence numbers can have gaps if an op fails14:55
rogpeppehazmat: that's true. machine ids can do that.14:56
hazmatand there global14:56
rogpeppehazmat: do gaps matter?14:56
hazmatacross all services, where as we want the service unit increments14:56
hazmatrogpeppe, its rather disconcerting to see wordpress/5 and  wordpress/22214:56
hazmatas sequential units14:57
hazmater.. sequentially allocated taht is14:57
hazmatie.. the zk sequence is across all units14:57
TheMuehazmat: So while the key is purely internal the name containing the sequence no is also external visible?14:57
hazmatTheMue, yup14:57
TheMuehazmat: Makes sense, thx.14:58
hazmatTheMue, the python code base distinguishes between names and internal_ids14:58
rogpeppehazmat: doesn't the same logic apply to machine ids?14:58
hazmatnames being visible, and internal ids being the impl detail14:58
TheMuehazmat: Yes, in Go the internal ids are now keys, but have the same role.14:58
hazmatrogpeppe, to some extent, its commutes, but its not really something we ever really care about per se, a) juju cares about presenting the services not the machines b) the machine global sequence is correct for machine allocation14:59
rogpeppehazmat: i guess i wonder if it's worth putting logic and code behind something that is a) very rare and b) only mildly disconcerting if it does happen.14:59
hazmatwhere as the units want service namespace'd sequences14:59
hazmatrogpeppe, i doubt it re machines14:59
hazmatrogpeppe, for service units its important, unless you have another impl of per service unit sequences15:00
rogpeppehazmat: do we rely on the sequentiality anywhere?15:00
hazmatrogpeppe, never15:01
hazmatrogpeppe, its a user interface question15:01
rogpeppehazmat: or is it just the fact that someone might see 1 followed by 3 and wonder what's going on.15:01
hazmatrogpeppe, its not about the 1 and 3, its about 1 and 25..15:02
hazmatrogpeppe, the unit sequence in zk is global to all units across all services15:02
rogpeppehazmat: if we put unit directories inside service directories, that wouldn't be a problem15:02
rogpeppehazmat: but maybe there's a good reason why that is a bad idea15:02
hazmatrogpeppe, right.. if you have another impl of per service unit sequences15:02
rogpeppehazmat: ah, i see what you mean by that now15:02
* hazmat loves repetition15:02
hazmatrogpeppe, that sounds fine to me15:03
rogpeppeTheMue: a reason for doing this is that it means i can map directly from a unit name e.g. wordpress/0 to its directory, without needing to consult the topology.15:04
rogpeppeTheMue: so unit name, sequence number and key are all derivable from each other.15:04
TheMuerogpeppe: Do we have problems so far consulting the topology?15:05
TheMuerogpeppe: Or asking it differently: When do we get scaling probs due to the top size?15:06
rogpeppeTheMue: what should UnitsForMachine return?15:06
hazmatit hasn't been a problem at scale, but we haven't been measuring the network traffic to get a better understanding of the outflow15:06
rogpeppeTheMue: it's not really a performance issue, but one of neatness.15:06
hazmatand from a size of topology, its really a non issue with any compression15:07
hazmatthe sequence numbers that is15:07
hazmatwe can get about 15k machines/services/service units in atm without blowing 1mb, without compression15:07
rogpeppeTheMue: i'll have a play and see how it looks15:07
TheMuerogpeppe: So both solutions have to be compared.15:07
rogpeppeTheMue: yeah15:08
TheMuerogpeppe: Breaking todays top concept could lead to a larger redesign, stop by step. Let's do it here, oh, and there.15:08
hazmatrogpeppe, its not clear thats its meanigful though, you need the topology for most anything.. and units to service name lookups aren't common15:09
TheMuerogpeppe: I only see the risk of changing too much of what we reached before the rest is done. 12.10 is coming. ;)15:09
hazmatrogpeppe, ie. that translation is pretty rare in practice for the runtime15:09
hazmatrogpeppe, because typically its holding onto a service unit state which has both values captured15:09
rogpeppehazmat, TheMue: ok, i'll leave as is. it should be fairly easy to refactor later actually.15:09
hazmatyes.. avoid.. premature optimization ;-)15:10
TheMuehazmat: Hehe, yes.15:10
hazmatrogpeppe, i agree though that we can better about the topology slinging, i just don't think that particular case is particularly relevant.15:11
TheMuerogpeppe: Maybe we get a better solution when the layer around ZK is done and we change the backend in a second step.15:11
rogpeppehazmat: as always. although i wasn't really considering this in performance terms.15:11
rogpeppehazmat: just that we could reduce the number of entities w.l.o.g.15:11
hazmatisn't neatness is an eye of the beholder thing isn't it?15:11
hazmatand is the neatness you defined avoiding fetching the topology?15:12
rogpeppehazmat: no, that was another thing.15:12
hazmatnm.. i should move on.. silly deadlines15:12
rogpeppehazmat: in this case we've already got the topology15:12
rogpeppeTheMue, hazmat: my impulse to change things stemmed from my "so why does a unit need two names" question. and the answer is "it doesn't, but that's the way it's been done" AFAICS.15:14
hazmatrogpeppe, then you've ignored the discussion to this point.. its two fold, to present a nice interface, and to separate interface from impl15:15
TheMuerogpeppe: So let's keep it as long as there's no real need. There are still enough tasks to do.15:15
rogpeppehazmat: putting unit directories inside their service directories solves both of those issues, i think.15:16
hazmatrogpeppe, agreed15:17
hazmatrogpeppe, but that's solving those two issues in a different way, that doesn't obviate the reasons15:18
rogpeppehazmat: if we did that, then a unit would only have one name15:18
rogpeppehazmat: or at least, the different categories of names would map 1-115:19
* hazmat nods15:19
niemeyerrogpeppe: Did you get the hotel confirmation?15:25
rogpeppeniemeyer: yes thanks15:25
rogpeppeniemeyer: should i organise my own travel?15:26
rogpeppeniemeyer: (if so, what time should i plan to arrive/leave?)15:26
niemeyerrogpeppe: Yeah, I think that's simpler in your case15:26
niemeyerrogpeppe: Arriving around noonish or early afternoon sounds fine.. I expect Aram will be arriving around that time as well15:27
rogpeppeniemeyer: ok, that's easy enough. leaving?15:29
niemeyerrogpeppe: Planning for end of afternoon/early night on Friday should work well15:29
rogpeppeniemeyer: not cheap, those train tickets!15:31
rogpeppeniemeyer: looks like minimum price is around £230.15:34
rogpeppeniemeyer: i bet Aram's flight isn't far off that15:34
niemeyerrogpeppe: Ugh.. a bit unfortunate indeed15:35
niemeyerrogpeppe: Still a lot cheaper than flying you to Brazil, though ;)15:36
rogpeppeniemeyer: very true15:36
rogpeppeniemeyer: if i want a flexible return time, the price is £301 !15:38
rogpeppeniemeyer: so i think i'll plan to leave at 1700 on friday, which should get me home by 2100, if that sounds ok15:42
niemeyerrogpeppe: Yep, sounds good15:42
rogpeppeniemeyer: that's the train leaving at 1700, so i guess i'd be leaving millbank (which is presumably where we'd be?) at 1600.15:43
niemeyerrogpeppe: No, we'll actually not have a room on Friday afternoon.. so we'll be meeting somewhere more open15:43
niemeyerrogpeppe: We have a room for the morning rented somewhere, though15:44
rogpeppeniemeyer: just as long as it's not too far away from king's cross, that'd be good15:44
niemeyerrogpeppe: Everybody is leaving EOD.. so won't be an issue15:44
rogpeppeniemeyer: cool. will book tickets now.15:45
rogpeppeniemeyer: all booked.16:18
niemeyerrogpeppe: Woohay17:07
niemeyerAram: Do you have the new flight confirmation?17:07
niemeyerAram: For real Thursday?17:07
Aramniemeyer: yes, I printed my boarding pass.17:08
niemeyerAram: Cool, just to be sure, you noticed that the flight was booked wrong and then re-booked, right?17:09
Aramyes, yes, got a funny email why wasn't I present in today's flight.17:09
niemeyerAram: Cool17:09
niemeyerAram: It was half my mistake.. I was silly to assume they would actually book in the day I asked for17:10
niemeyerRather than double checking the confirmation17:10
rogpeppeniemeyer, fwereade, TheMue: first step towards the unit-assignment watcher: https://codereview.appspot.com/6256070/17:13
niemeyerAram: I'm not being ironic.. that's why they send the flights for confirmation17:13
rogpeppei'm off for the night17:14
rogpeppeAram, niemeyer: see ya tomorrow!17:14
Aramhave fun.17:14
niemeyerrogpeppe: Awesome, have a good one17:14
niemeyerI shouldn't take too long either17:15
niemeyerAram: I assume you have address and all?17:24
Aram27th Floor, Millbank Tower17:25
Aram21-24 Millbank17:25
Aramit's good, right?17:25
niemeyerAram: So if we don't talk again today, I'll see you here tomorrow17:28
niemeyerStepping out for dinner17:28
niemeyerCheers all17:28
Aramyes, great!17:28
davecheneyniemeyer: are you happy for me to extend, go-cmd-jujud-fix-testing, to include the other commands then submit ?22:30
davecheneyor should I do another round of reviews ?22:31
davecheneyis schema_test.go failing for anyone else ?22:53

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