| === wedgwood is now known as wedgwood_away | ||
| wallyworld_ | davecheney: hi, any chance of a quick 2 line code review? improved boilerplate for 1.9.8 release https://code.launchpad.net/~wallyworld/juju-core/improve-boilerplate-config/+merge/147279 | 04:28 |
|---|---|---|
| davecheney | sure | 04:28 |
| wallyworld_ | thanks | 04:28 |
| davecheney | have you guys stopped using lbox ? | 04:29 |
| wallyworld_ | i can't login | 04:29 |
| wallyworld_ | it won't auth | 04:29 |
| wallyworld_ | trying to debug it now - it seems the login response returns no cookies | 04:29 |
| wallyworld_ | even though it nominally succeeds | 04:29 |
| davecheney | LGTM | 04:31 |
| davecheney | afk for a bit - hangout | 04:31 |
| wallyworld_ | thanks | 04:32 |
| thumper | hi wallyworld_ | 04:33 |
| thumper | I'm just about to sign off | 04:33 |
| wallyworld_ | g'day | 04:33 |
| thumper | busy getting my head around juju | 04:33 |
| wallyworld_ | hah | 04:33 |
| wallyworld_ | have a good weekend | 04:33 |
| * thumper waves at davecheney too | 04:33 | |
| thumper | you too | 04:33 |
| * wallyworld_ wishes it would stop raining so soccer isn't cancelled tonight | 04:33 | |
| davecheney | 013/02/08 17:34:22 JUJU juju bootstrap command failed: cannot start bootstrap instance: cannot allocate a public IP as needed | 06:35 |
| davecheney | any ideas ? | 06:35 |
| davecheney | this is on canonistack | 06:35 |
| TheMue | Morning | 07:38 |
| rogpeppe | davecheney: i think public IPs are few and far between in canonistack | 08:06 |
| rogpeppe | davecheney: istr there was some talk of making getting a public IP optional | 08:08 |
| rogpeppe | TheMue, fwereade_, all: mornin' | 08:08 |
| TheMue | rogpeppe: Hello Mr Peppe ;) | 08:08 |
| TheMue | davecheney, fwereade_: And also a hello to you. | 08:09 |
| * rogpeppe loves it when tests find obvious mistakes that you've looked at many times without seeing. | 10:12 | |
| rogpeppe | fwereade_: ping | 11:11 |
| rogpeppe | if anyone wants to take a look, here's the CL that implements API authentication: https://codereview.appspot.com/7299066/ | 11:16 |
| TheMue | rogpeppe: *click* | 11:16 |
| TheMue | rogpeppe: You've got a +1. | 11:43 |
| rogpeppe | TheMue: thanks | 11:43 |
| TheMue | rogpeppe: yw | 11:44 |
| rogpeppe | " | 11:45 |
| rogpeppe | Name "stm" has been a bit misleading because we mostly talk about state.Machine | 11:45 |
| rogpeppe | here. | 11:45 |
| rogpeppe | " | 11:45 |
| rogpeppe | TheMue: the point of naming it stm there is because it *is* a state.Machine | 11:45 |
| rogpeppe | TheMue: an alternative would be to call all the api-specific variables apist, apim, etc | 11:46 |
| rogpeppe | TheMue: rather than st, m | 11:46 |
| TheMue | rogpeppe: All m's in there are state.Machines. And in the same method you also mad a pure m. In that case the M(achine) has a special semantic. That's why I proposed a different prefix. | 11:48 |
| rogpeppe | TheMue: "pam" is not great, because it doesn't say anything about why that machine is special (the machine returned by the api.State.Machine call is a representation of that machine too) | 11:49 |
| rogpeppe | TheMue: perhaps i should just call it apim | 11:49 |
| TheMue | rogpeppe: +1 on that, yep. | 11:50 |
| TheMue | rogpeppe: But even w/o a change it would be ok for me, it only has been a minor. | 11:50 |
| rogpeppe | TheMue: ok, cool | 11:51 |
| TheMue | rogpeppe: One look into the according function and it has been clear. | 11:51 |
| TheMue | rogpeppe: But your func() func() (func(), error) is nice. *bg* | 11:51 |
| rogpeppe | TheMue: closures are great :-) | 11:51 |
| TheMue | rogpeppe: Indeed. | 11:52 |
| TheMue | rogpeppe: Only missed a func() as arg of that func. *rofl* | 11:52 |
| rogpeppe | TheMue: higher-order tests, yay! | 11:53 |
| TheMue | rogpeppe: Or a func, getting a func, producing a func which has a func as arg to produce a func. | 11:53 |
| rogpeppe | TheMue: now now, then, we're not using haskell :-) | 11:53 |
| TheMue | rogpeppe: Hehe, and it would look weird. Smalltalk closures are pretty short. [:a :b | a doSomethingWith: b] | 11:55 |
| TheMue | rogpeppe: But the call is ugly. | 11:55 |
| TheMue | rogpeppe: In case that one is stored in c it has to be "c value: something value: anotherThing." | 11:56 |
| TheMue | Oh, just called for lunchtime. | 11:57 |
| TheMue | biab | 11:57 |
| rogpeppe | TheMue: of course, no unnamed params in smalltalk | 11:57 |
| fwereade_ | rogpeppe, pong, sorry I missed you | 12:13 |
| dimitern | fwereade_: fancy a quick review? :) | 12:16 |
| fwereade_ | dimitern, sure :) | 12:17 |
| dimitern | fwereade_: https://codereview.appspot.com/7299047/ - should be simple enough | 12:17 |
| dimitern | fwereade_: 10x | 12:17 |
| rogpeppe | mgz: i'm interested to know what you thought was particularly bad about the ec2 port handling code. nothing seems to stand out for me, but i'm certainly biased :-) | 12:35 |
| rogpeppe | fwereade_: just thought you might wanna have a look at the API authentication CL: : https://codereview.appspot.com/7299066/ | 12:35 |
| rogpeppe | dimitern: looking | 12:35 |
| fwereade_ | rogpeppe, ah, cheers | 12:35 |
| mgz | rogpeppe: which particular frustration I feel with it varies :) | 12:36 |
| rogpeppe | fwereade_: it's a bit bigger than i intended, but hopefully still not *too* bad | 12:36 |
| rogpeppe | mgz: top 3 frustrations? | 12:36 |
| mgz | so, currently, | 12:37 |
| mgz | the copy code between ec2/openstack (need to learn go trick for sharing code without making something public interface) | 12:38 |
| rogpeppe | mgz: the difficulty there is that openstack and ec2 interfaces are very similar but not the same... | 12:38 |
| rogpeppe | mgz: if there was any code you wanted to factor out, it could go into another package, say environs/ec2like | 12:39 |
| rogpeppe | mgz: it has always been the intention to factor out common code between providers - but before we had multiple providers, it was hard to say which code was appropriate. | 12:40 |
| mgz | batch rule handling where it doesn't make sense for openstack, magic-name lookup and unclear behaviour on individual errors | 12:40 |
| mgz | and the long-standing but less current one of security group per-machine and cleanup of resources | 12:41 |
| rogpeppe | mgz: yeah, the magic name lookup was a legacy from the python port | 12:41 |
| rogpeppe | mgz: in openstack, can you change the security groups assigned to a machine? | 12:42 |
| mgz | yup. | 12:42 |
| rogpeppe | mgz: ah, then you can avoid the one-security-group-per machine issue | 12:42 |
| mgz | though, there are caveats that make that less promising that it sounds. | 12:42 |
| rogpeppe | mgz: in ec2 you can't | 12:42 |
| dimitern | rogpeppe, mgz: you can change the secgroups of a machine, but you need to reboot it to take effect | 12:44 |
| mgz | in that, I think deployment variable, you can't trust rules on a new group to apply unless the vm is rebooted | 12:44 |
| rogpeppe | mgz: cleanup of resources is an issue - the main problem is when you kill the instances but never run destroy-environment. | 12:44 |
| rogpeppe | mgz: ah, that makes it pretty useless | 12:44 |
| mgz | in openstack cleanup can be done properly, if careful, because you can delete groups on terminate machine | 12:45 |
| rogpeppe | mgz: "i'll call juju expose - oops, all my machines have just rebooted!" | 12:45 |
| mgz | anyway, the port batching is annoying | 12:48 |
| mgz | especially given it's still port-at-a-time | 12:48 |
| mgz | and range ignorant | 12:48 |
| rogpeppe | mgz: at one time, i did have the ec2 provider delete security groups on instance termination, but it failed all the time, because the instance takes a while to relinquish its security groups | 12:50 |
| mgz | right, I know that code | 12:50 |
| mgz | it can be done safely with openstack because you can remove the group from the server first | 12:50 |
| rogpeppe | mgz: the port batching means you do at least have the capability to do it as a batch. but i agree it's not entirely clear what to do about partial errors. | 12:50 |
| mgz | leads to some slightly-complicated deferred chaining with twisted though :) | 12:51 |
| rogpeppe | mgz: but that issue applies to the agent too - what should the agent do if it can't remove a given port? | 12:52 |
| rogpeppe | mgz: hopefully not so complicated under Go :-) | 12:52 |
| mgz | clearly report that one part failed and continue, probably | 12:53 |
| mgz | the fun has tended to be that things get logged somewhere far from what cares about it so it's hard to tell why something hasn't worked | 12:54 |
| rogpeppe | mgz: yeah, logging is an issue we need to think hard about | 13:04 |
| rogpeppe | mgz: i'd be happy to see the batch operations return an error that summarises all the ports that couldn't be closed, rather than just stop at the first. | 13:04 |
| rogpeppe | lunchtime | 13:05 |
| dimitern | what does it mean `json:"-"` as a tag? | 13:11 |
| dimitern | on a struct field | 13:12 |
| TheMue | dimitern: The field is ignored in serialization. | 13:15 |
| dimitern | TheMue: ignored only on marshaling, but not unmarshaling? | 13:16 |
| TheMue | dimitern: Oh, would have to test. IMHO only marshalling. | 13:17 |
| dimitern | TheMue: ok, thanks | 13:17 |
| dimitern | fwereade_, rogpeppe: any thoughts on the CL? | 13:28 |
| fwereade_ | dimitern, reviewed, let me know your thoughts; I might be missing some subtleties | 13:29 |
| dimitern | fwereade_: cheers, I'll take a look | 13:30 |
| dimitern | fwereade_: I'm not sure I get your idea with simplifying the matching loop - can you elaborate a bit? | 13:36 |
| fwereade_ | dimitern, say I ask for the following ids: "", "a", "b", "c", "d" (hope that's enough) | 13:39 |
| === wedgwood_away is now known as wedgwood | ||
| dimitern | fwereade_: so, you propose to take the API calls out of the function and pass the result as a map | 13:40 |
| fwereade_ | dimitern, "a" is missing due to eventual consistency, "b" is in the wrong env, "c" is terminated, and "d" actually works correctly | 13:40 |
| fwereade_ | dimitern, gatherInstances would put an instance in the map under "d" | 13:41 |
| fwereade_ | dimitern, and return only {"a"} | 13:41 |
| fwereade_ | dimitern, because it has enough information after the first call to eliminate all the others from consideration | 13:42 |
| dimitern | fwereade_: ah, so the API calls stay there | 13:42 |
| fwereade_ | dimitern, yeah | 13:42 |
| rogpeppe | TheMue, dimitern: json:"-" means it's ignored for both marshalling and unmarshalling | 13:42 |
| rogpeppe | http://play.golang.org/p/QdPfN_Dv4Z | 13:42 |
| dimitern | rogpeppe: got it, thanks | 13:43 |
| dimitern | fwereade_: so gather will return only the ones it cannot find | 13:43 |
| dimitern | fwereade_: but that'll change Instances and make it more complex it seems | 13:44 |
| fwereade_ | dimitern, maybe I'm missing something, because it seemed it would become simpler, I'll try to sketch what I'm imagining | 13:45 |
| dimitern | fwereade_: quite a lot is happening there - handling all of these cases: empty ids passed, multiple ids, single ids with empty ones (essentially a single id), servers with errors/missing | 13:45 |
| dimitern | rogpeppe: Add() is there because we reuse url.Values and its methods | 13:47 |
| dimitern | rogpeppe: I don't know how to override an inherited method so it's not public any more and cannot be called | 13:48 |
| rogpeppe | dimitern: i hadn't seen that. i'm not sure it's entirely appropriate to embed url.Values there | 13:49 |
| dimitern | rogpeppe: we need it because of the Encode() method mostly, but also Get and Set | 13:49 |
| rogpeppe | dimitern: do nova clients need to call Encode on it? | 13:49 |
| TheMue | rogpeppe: tThx for the info. | 13:50 |
| dimitern | rogpeppe: yes, to pass it to nova in the url | 13:50 |
| dimitern | rogpeppe: it happens internally in ListServers/Detail | 13:50 |
| rogpeppe | dimitern: but that's an implementation detail, right. nothing external to nova needs to see that the underlying representation is a url.Values, does it? | 13:51 |
| rogpeppe | s/right./right?/ | 13:51 |
| dimitern | rogpeppe: right | 13:52 |
| rogpeppe | dimitern: i'd be much happier to see type Filter struct {v url.Values} | 13:52 |
| rogpeppe | dimitern: implementing only the single method Add | 13:52 |
| dimitern | rogpeppe: I see you point, and I was wondering so myself | 13:52 |
| fwereade_ | dimitern, ahh, I see the problem -- you can't tell whether something missing from ListServersDetail is because it's not there, or because it is there but in in the wrong environment | 13:52 |
| rogpeppe | dimitern: a mean the single method *Set* of course :-) | 13:53 |
| rogpeppe | s/a mean/i mean/ | 13:53 |
| dimitern | rogpeppe: I should propose a follow-up changing this, so we only have explicit methods on Filter | 13:53 |
| rogpeppe | dimitern: +1 | 13:53 |
| dimitern | fwereade_: exactlty | 13:53 |
| dimitern | fwereade_: exactly even :) | 13:53 |
| dimitern | rogpeppe: I'm on it - it should be trivial | 13:55 |
| fwereade_ | dimitern, ok, but that is very much an edge case indicating that things are seriously wrong elsewhere, right? | 13:55 |
| dimitern | fwereade_: not necessarily, why? | 13:55 |
| fwereade_ | dimitern, because for that value to have been passed in, something must have some reason to believe that instance is part of the environment, right? | 13:56 |
| dimitern | fwereade_: right | 13:58 |
| fwereade_ | dimitern, I'm not saying it *can't* happen, but it certainly shouldn't ;p | 13:58 |
| rogpeppe | fwereade_, dimitern: i'm not sure whether a map will help, but it does appear to me that the code could be simpler | 14:06 |
| dimitern | I'd appreciate a short example how, because I spent all day yesterday trying to make it as good and fast as I can, but probably failed it seems | 14:09 |
| fwereade_ | dimitern, how about http://paste.ubuntu.com/1624792/ ? | 14:11 |
| fwereade_ | dimitern, I appreciate that it's a bit of a gnarly problem and I've probably missed something | 14:11 |
| dimitern | fwereade_: looking | 14:11 |
| dimitern | rogpeppe: here's the follow-up - I think it should be trivial enough - https://codereview.appspot.com/7301064 | 14:12 |
| dimitern | fwereade_: is x := map[T]Y{} equivalent to x := make(map[T]Y) ? | 14:14 |
| fwereade_ | dimitern, yeah, as far as I'm aware | 14:14 |
| dimitern | fwereade_: cool, I'll remember that :) | 14:14 |
| fwereade_ | dimitern, yeah, I prefer it myself usually | 14:14 |
| * rogpeppe prefers make when appropriate :-) | 14:15 | |
| dimitern | rogpeppe: because it's more explicit? | 14:15 |
| rogpeppe | dimitern: yeah - i often have to look twice for the {} at the end | 14:16 |
| dimitern | rogpeppe: yes, I can see how it could be confusing - I missed it at first | 14:16 |
| rogpeppe | dimitern: the difference between var x map[T]Y and x := map[T]Y{} is subtle :-) | 14:17 |
| rogpeppe | dimitern: i don't mind either form though tbh | 14:18 |
| dimitern | rogpeppe: but var x map[X]Y is not the same, right? you still need to make() it before using? | 14:18 |
| rogpeppe | dimitern: exactly. that's why it's a subtle difference to spot. | 14:18 |
| dimitern | rogpeppe: yeah | 14:18 |
| dimitern | i'll go grab some food, bbiab | 14:19 |
| rogpeppe | in limbo if you used a type expression as a normal expression, it made something of that type. so you could say x := chan of int, and it was equivalent to x := make(chan int) | 14:20 |
| rogpeppe | that was nice but syntactically limiting | 14:21 |
| rogpeppe | fwereade_: +1 to your collectInstances | 14:22 |
| fwereade_ | rogpeppe, cheers :) | 14:22 |
| rogpeppe | fwereade_: although an approach similar to ec2's can also work fine there i think. it's got an n^2 loop, but n is always 1 in our code... | 14:23 |
| fwereade_ | rogpeppe, I think ec2 gathers more than it needs to, but it's a simple fix: n++ at :504 before continuing | 14:26 |
| rogpeppe | fwereade_: omg you're right. that's a bug. | 14:27 |
| rogpeppe | fwereade_: and i think that it cause errors actually | 14:28 |
| rogpeppe | s/cause/could cause/ | 14:28 |
| fwereade_ | rogpeppe, glad we saw it then :) | 14:29 |
| rogpeppe | fwereade_: ah, i see why the tests don't catch it - it can only happen if a request fails first time, then succeeds the next. | 14:29 |
| fwereade_ | rogpeppe, yeah, think so | 14:30 |
| rogpeppe | fwereade_: we should probably add something to ec2test to enable particular failure modes | 14:30 |
| fwereade_ | rogpeppe, +100 | 14:30 |
| rogpeppe | fwereade_: perhaps like the hook feature in the openstack double | 14:31 |
| fwereade_ | rogpeppe, yeah, given that it doesn't seem to be causing too much pain, I think it's a solid model | 14:31 |
| rogpeppe | fwereade_: i didn't do it before because i wasn't sure of a nice way to do it. | 14:31 |
| dimitern | fwereade_: right :) that pesky n++ :) | 14:31 |
| fwereade_ | rogpeppe, I completely understand that :) | 14:31 |
| dimitern | fwereade_: but the other thing - removing the for loop checking if any(insts) is nil should stay as is | 14:32 |
| rogpeppe | fwereade_: have you had a glance at the api auth CL, by any chance? i'm interested how you think it's turning out. | 14:32 |
| fwereade_ | rogpeppe, was just starting to when I got distracted by this discussion :) | 14:33 |
| rogpeppe | fwereade_: lol | 14:33 |
| fwereade_ | dimitern, ah, I thought I'd handled that -- what scenario did I miss? | 14:33 |
| dimitern | fwereade_: collectInstances won't work correctly with [id0, ""] or [id0, "", ""] for that mater, needs an extra check to avoid doing List when you can do Get | 14:33 |
| dimitern | fwereade_: no I meant ec2 Instances() | 14:34 |
| fwereade_ | dimitern, ahok | 14:34 |
| fwereade_ | dimitern, yeah, I guess we could start by just filtering ""s out in Instances | 14:35 |
| dimitern | fwereade_: sure, we can filter them, but we need to still return ErrPartialInstances whenever there are empty ids | 14:36 |
| fwereade_ | dimitern, that'll happen anyway, won;t it? | 14:36 |
| dimitern | fwereade_: that's what a lot of the code and tests expect (why though, it beats me) | 14:36 |
| fwereade_ | dimitern, we just construct missing by filtering ids at the start | 14:36 |
| dimitern | fwereade_: yes, but len(ids) == 1 is not enough, we should scan through it until we find one or more ids to check | 14:37 |
| dimitern | fwereade_: imagine ["",id0] or ["", "", id0], etc. | 14:37 |
| dimitern | fwereade_: in this case all we need is Get(id0) | 14:38 |
| fwereade_ | dimitern, if we pre-filter ids to missing in Instances, we'll just pass ["id0"] up to collectInstances, right? | 14:38 |
| dimitern | fwereade_: that'll work, but despite filtering, we still have to return ErrPartialInstances in Instances() when there are empty ones | 14:39 |
| fwereade_ | dimitern, I assert that it will work correctly, because the final loop won't find any Instance for "" | 14:39 |
| dimitern | fwereade_: you're right, so just a filtering before collect should be enough | 14:41 |
| dimitern | fwereade_: and not calling it at all if after filtering len == 0 | 14:42 |
| dimitern | fwereade_: i.e. ["", ""] was passed | 14:42 |
| fwereade_ | dimitern, yeah, +1 | 14:42 |
| dimitern | fwereade_: great, I'll give it a go then | 14:43 |
| dimitern | fwereade_: tyvm | 14:43 |
| rogpeppe | dimitern: reviewed https://codereview.appspot.com/7301064/ | 14:46 |
| rogpeppe | dimitern: i don't think you should worry if we call ListInstances when the user passes in blank instances | 14:48 |
| rogpeppe | dimitern: it should never happen, and the code should work correctly even when it does | 14:48 |
| rogpeppe | dimitern: so i wouldn't bother doing any initial filtering - it complicates the code for no gain | 14:49 |
| dimitern | rogpeppe: thanks for the review | 14:50 |
| dimitern | rogpeppe: it happens a lot in tests | 14:51 |
| rogpeppe | dimitern: do we care? | 14:51 |
| rogpeppe | dimitern: the performance will be almost exactly the same | 14:51 |
| dimitern | rogpeppe: hmm.. since we're not actually trying to pass any ids to nova, but rather filter them later, this will affect only the single id - Get case, we shouldn't call Get("") | 14:53 |
| rogpeppe | dimitern: i don't even care if we do that | 14:54 |
| rogpeppe | dimitern: the tests should probably use "somerandomthing" rather than "" as an invalid id | 14:55 |
| rogpeppe | dimitern: but the result is probably the same tbh | 14:55 |
| rogpeppe | dimitern: there's nothing special about "" | 14:55 |
| rogpeppe | dimitern: in this case | 14:55 |
| dimitern | rogpeppe: not exactly, passing "" will try to fetch /servers/ rather than /servers/id and till return 404 | 14:56 |
| rogpeppe | dimitern: that seems like a bug in goose to me | 14:56 |
| dimitern | rogpeppe: s/till/will/, but this is the same really | 14:56 |
| rogpeppe | dimitern: it should error out if the id is empty | 14:57 |
| dimitern | rogpeppe: this makes sense | 14:57 |
| dimitern | rogpeppe: anyway - I only added the Get() single id case as jam suggested | 14:58 |
| dimitern | rogpeppe: but now it seems more of an overkill to support this case will all its subtleties | 14:59 |
| rogpeppe | dimitern: i think using Get when appropriate is good | 14:59 |
| rogpeppe | dimitern: but i don't think that you need to worry about the case where some ids are invalid | 14:59 |
| rogpeppe | dimitern: the only difficulty is that the server might return an "invalid id" error. | 15:00 |
| dimitern | rogpeppe: yes, and having List only will solve any issues | 15:01 |
| dimitern | rogpeppe: and frankly, he performance benefit of get escapes me now | 15:01 |
| dimitern | rogpeppe: when we have to handle all those extra cases so it won't blow unexpectedly | 15:02 |
| dimitern | rogpeppe: changes applied - https://codereview.appspot.com/7301064/ | 15:10 |
| dimitern | my irc client behaves odd | 15:20 |
| dimitern | not sure I managed so send anything in the last 10m | 15:20 |
| rogpeppe | dimitern: last messages i've seen from you: | 15:49 |
| rogpeppe | [15:10:11] <dimitern> rogpeppe: changes applied - https://codereview.appspot.com/7301064/ | 15:49 |
| rogpeppe | [15:20:20] <dimitern> my irc client behaves odd | 15:49 |
| rogpeppe | [15:20:42] <dimitern> not sure I managed so send anything in the last 10m | 15:49 |
| dimitern | rogpeppe: so I didn't miss anything, cheers | 15:50 |
| dimitern | fwereade_: fresh from the oven - https://codereview.appspot.com/7299047/ :) | 16:13 |
| fwereade_ | dimitern, cheers -- I'm just going through rog's API stuff now | 16:14 |
| dimitern | fwereade_: no worries | 16:15 |
| dimitern | rogpeppe: refactored filters, as discussed: https://codereview.appspot.com/7301064 | 16:54 |
| dimitern | fwereade_: when you can, can you look at this too please? ^^ | 16:54 |
| fwereade_ | dimitern, ack | 16:55 |
| dimitern | fwereade_: thanks | 16:55 |
| rogpeppe | fwereade_: i still see Get and Del methods on nova.Filter | 16:55 |
| dimitern | rogpeppe: oops, sorry they're unused now, but forgot to remove them | 16:56 |
| dimitern | rogpeppe: fixed now, 10x | 16:57 |
| * dimitern afk 10m | 16:57 | |
| rogpeppe | dimitern: reviewed | 17:02 |
| dimitern | back | 17:08 |
| dimitern | rogpeppe: tyvm | 17:08 |
| dimitern | is a map always passed by reference? | 17:13 |
| rogpeppe | dimitern: yes | 17:13 |
| dimitern | func f(m *map[X]Y) <=> func f(m map[X]Y) ? | 17:13 |
| rogpeppe | nope | 17:14 |
| rogpeppe | dimitern: a pointer to a map is a pointer to a map | 17:14 |
| rogpeppe | dimitern: the first form of the function there is redundant | 17:14 |
| dimitern | but i suppose having a pointer here no longer makes sense | 17:14 |
| mgz | dimitern: so, just to double check with you, to land things on goose now I should set a commit message on lp and mark approved? | 17:14 |
| rogpeppe | dimitern: yup | 17:14 |
| dimitern | mgz: yes | 17:14 |
| dimitern | rogpeppe: cheers | 17:14 |
| mgz | dimitern: ta | 17:15 |
| dimitern | rogpeppe: so LGTM? :) | 17:24 |
| dimitern | mgz: so the release is today? | 17:28 |
| mgz | well, by monday we assume | 17:28 |
| rogpeppe | dimitern: LGTM | 17:28 |
| dimitern | rogpeppe: great, 10x | 17:29 |
| fwereade_ | rogpeppe, I'm not going to finish it today, but I think I'm comfortable with what I've seen so far | 17:34 |
| fwereade_ | dimitern, I won't get to yours tonight I'm afraid | 17:34 |
| dimitern | fwereade_: no problem | 17:37 |
| dimitern | mgz: can you take a look then? https://codereview.appspot.com/7299047/ | 17:38 |
| mgz | sure | 17:38 |
| dimitern | cheers | 17:38 |
| rogpeppe | fwereade_: great, thanks | 17:43 |
| rogpeppe | fwereade_: any chance of publishing any comments you have so far? | 17:43 |
| dimitern | rogpeppe: can i bug you one last time (for today)? :) https://codereview.appspot.com/7299047/ | 17:55 |
| rogpeppe | dimitern: i preferred william's version that used GetServer most of the time | 17:57 |
| dimitern | rogpeppe: the round-trip is still the same actually | 17:57 |
| rogpeppe | dimitern: because we really don't want to be downloading all server details when we're after just a single instance (which is actually the only way we use Instances currently) | 17:58 |
| rogpeppe | dimitern: the amount of traffic is not though | 17:58 |
| mgz | right, there are a few slightly complex things there dimitern that I'm not certain of | 17:58 |
| dimitern | rogpeppe: so it's certain we're using mostly Instances([]{id}) ? | 17:58 |
| rogpeppe | dimitern: if you've got 10000 servers, you're going to get details for all of them, even though you're just interested in one. | 17:58 |
| mgz | I don't mind landing it if we actually get a chance to revisit though | 17:59 |
| rogpeppe | dimitern: yup. grep for it yourself... | 17:59 |
| rogpeppe | dimitern: we never actually call Instances with more than one instance, except in tests. | 17:59 |
| dimitern | rogpeppe: and how about passing empty ids ? is this just for tests for it actually happens often? | 17:59 |
| rogpeppe | dimitern: that's just for tests. | 18:00 |
| rogpeppe | dimitern: i have another idea actually | 18:00 |
| rogpeppe | dimitern: you could just spawn off n concurrent GetServer requests | 18:00 |
| dimitern | rogpeppe: ok, so that's easy enough to fix - i'll just test the len() == 1 case and leave the rest | 18:00 |
| rogpeppe | dimitern: and avoid using List at all | 18:00 |
| rogpeppe | dimitern: sounds good. | 18:00 |
| dimitern | rogpeppe: wow! that's seems an overkill | 18:00 |
| dimitern | rogpeppe: how it would be better? | 18:01 |
| rogpeppe | dimitern: yeah, ignore that. although it's probably not much code. | 18:01 |
| dimitern | rogpeppe: no, i can see it in like 5 lines, but I'm curious about the merits | 18:01 |
| rogpeppe | dimitern: it would mean that if you're fetching only 2 instances out of 1000, you only get info for those two instances. | 18:02 |
| rogpeppe | dimitern: and the round-trip time would be similar | 18:02 |
| dimitern | rogpeppe: exactly, it'll effectively increase in fact - n*GetServer >> m*ListServers, and now ISTM m is usually 1 | 18:03 |
| rogpeppe | dimitern: i dunno; it's not n*GetServer because we're doing them all concurrently. | 18:04 |
| dimitern | rogpeppe: the traffic I mean, not the time | 18:04 |
| dimitern | rogpeppe: with n=100k it makes a huge difference to use 1 getserver if possible | 18:05 |
| rogpeppe | dimitern: yeah. and even 2 getservers when n is two. | 18:05 |
| rogpeppe | dimitern: if you've got 1000 servers but you're only requesting two instances, the traffic for ListServers is proportional to 1000, but the traffic for GetServer is proportional to 2. | 18:06 |
| rogpeppe | dimitern: but tbh the 1-instance case is the only one i care about. | 18:06 |
| dimitern | rogpeppe: i didn't quite get that 1-instance? you mean optimize both len == 1 and len == 2 cases? | 18:07 |
| dimitern | rogpeppe: going even further, we can estimate how many getserver calls we can make and still be better of than a single listservers if we know N :) | 18:08 |
| rogpeppe | dimitern: you mean if we know M, presumably? | 18:08 |
| dimitern | rogpeppe: and of course, we parallelize the getserver calls | 18:09 |
| rogpeppe | dimitern: yeah | 18:09 |
| dimitern | rogpeppe: yeah | 18:09 |
| dimitern | rogpeppe: but perhaps that's going a bit too far now | 18:09 |
| rogpeppe | actually, we didn't have a variable name for overall number of instances... | 18:09 |
| rogpeppe | dimitern: yeah. just implement the n=1 case, and that will work fine for now. | 18:10 |
| dimitern | rogpeppe: we can assume len == 1 case is special and simple enough to optimize for | 18:10 |
| rogpeppe | dimitern: +1 | 18:10 |
| dimitern | rogpeppe: we can cache len(AllInstances) | 18:10 |
| rogpeppe | dimitern: not worth it | 18:10 |
| rogpeppe | dimitern: in the cases we're concerned about, there might be huge churn, and our cache is likely worthless. | 18:11 |
| rogpeppe | dimitern: and the number of instances we're requesting is always likely to be small, i think. | 18:11 |
| dimitern | rogpeppe: well, if we have metering about traffic per cloud API call we can easily estimate such dynamic run-time optimizations | 18:12 |
| rogpeppe | dimitern: there are many optimisation possibilities :-) | 18:12 |
| dimitern | rogpeppe: juju will achieve some level of consciousness | 18:12 |
| dimitern | :) | 18:13 |
| rogpeppe | dimitern: it's gonna happen :-) | 18:13 |
| dimitern | rogpeppe: great book btw - born to run, fwereade_gave it to me and I few through it | 18:13 |
| rogpeppe | dimitern: isn't it fascinating? | 18:14 |
| rogpeppe | dimitern: glad you like it | 18:14 |
| rogpeppe | dimitern: do you run at all? | 18:14 |
| dimitern | rogpeppe: great for running motivation, i already ordered a pair of thin sole sandals :) | 18:14 |
| rogpeppe | dimitern: cool! | 18:14 |
| rogpeppe | dimitern: i think the main take-away is "strike toe first" | 18:14 |
| dimitern | rogpeppe: i started a few weeks ago, but it's tough to get out | 18:14 |
| dimitern | rogpeppe: oh yeah, easy, light, smooth, fast, etc. | 18:15 |
| rogpeppe | dimitern: and most importantly easier on the joints | 18:15 |
| dimitern | rogpeppe: this week i did like 10m and i'm still recovering my gait :) | 18:15 |
| rogpeppe | dimitern: i used the couch-to-5k routine for a while | 18:16 |
| dimitern | rogpeppe: very easy to overdo it, but as they suggest - watch your breathing | 18:16 |
| rogpeppe | http://www.c25k.com/ | 18:16 |
| rogpeppe | which is very good for not overdoing it | 18:16 |
| dimitern | rogpeppe: cool! I can use something handy like that | 18:17 |
| dimitern | 10x | 18:17 |
| rogpeppe | right, that's me for the night | 18:17 |
| rogpeppe | dimitern: see ya monday | 18:18 |
| rogpeppe | g'night all! | 18:18 |
| dimitern | happy weekend :) | 18:18 |
| dimitern | mgz: ping | 18:18 |
| mgz | dimitern: hey | 18:21 |
| dimitern | mgz: so you think is LGTM, assuming I complete rogpeppe's suggestion? | 18:21 |
| mgz | I'll leave some kind of comment to that effect, yeah | 18:22 |
| dimitern | thanks | 18:23 |
| davecheney | the 1.9.8 packages are in the gophers PPA | 20:35 |
| davecheney | if anyone feels like testing them before I hit the send button on the announcement | 20:35 |
| davecheney | that would be helpful | 20:36 |
| hazmat | davecheney, isn't terminate-machines implemented (ic destroy-machine) in revno876 | 23:08 |
| hazmat | davecheney, it would be good to tag release revnos (via bzr tag) | 23:11 |
| hazmat | davecheney, there's some useful tools for releng management in landscape-devel .. moving unclosed bugs to next milestone, etc | 23:36 |
| davecheney | hazmat: well that sucks | 23:38 |
| davecheney | i did remind people who were merging features on friday to update the release notes | 23:39 |
| davecheney | consider it a bonus | 23:39 |
| davecheney | hazmat: ta, haven't needed them recently because mostof the development is being tracked in leankit | 23:39 |
| davecheney | but *hopefully* now this is a public release, we'll get lots of wonderful feedback | 23:40 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!