| wallyworld_ | davecheney: hi there, i'd love a review if you have a change sometime https://codereview.appspot.com/6940069/ | 02:53 |
|---|---|---|
| wallyworld_ | also maybe https://codereview.appspot.com/6923056/ | 02:57 |
| davecheney | wallyworld_: no probs | 03:00 |
| wallyworld_ | awesome thanks | 03:00 |
| === davecheney is now known as geddan | ||
| geddan | wallyworld___: heading off, are there any more reviews to do tonight ? | 07:26 |
| fwereade | breakfast break, bbiab | 08:44 |
| rogpeppe | mornin' all | 08:57 |
| jam | morning | 09:15 |
| jam | I'm running into a test suite hang, where I end up with a "mongod <defunct>" process, and the test suite ends up being killed after waiting 5 minutes. | 09:15 |
| jam | Anyone seen that and can help me fix it? | 09:15 |
| jam | (very reproducible, it happens with about 10 of the juju-core packages) | 09:16 |
| jam | rogpeppe, fwereade: ^^ ? | 09:16 |
| rogpeppe | jam: do you know which test is hanging? | 09:16 |
| jam | rogpeppe: START: /home/jameinel/dev/go/src/launchpad.net/juju-core/cmd/juju/addrelation.go:0: AddRelationSuite.SetUpTest | 09:16 |
| jam | TestAddRelation | 09:17 |
| jam | but similar behavior is in a bunch of packages, so there may be a smaller one we could track down. | 09:17 |
| rogpeppe | jam: have you got a stack dump out of it, so that we can see what line it's hanging on? | 09:17 |
| jam | rogpeppe: well, if I try to connect with gdb, I get a permission denied, if I try as root, it dumps a memory map, perhaps there is a back trace. | 09:19 |
| rogpeppe | jam: i usually do it by killing the process with SIGQUITE | 09:20 |
| rogpeppe | QUIT | 09:20 |
| jam | rogpeppe: http://paste.ubuntu.com/1444879/ | 09:20 |
| fwereade | rogpeppe, jam, heyhey | 09:20 |
| rogpeppe | fwereade: yo! | 09:20 |
| fwereade | jam, sorry, I'm not familiar with that problem | 09:20 |
| rogpeppe | jam: hmm, that's not very helpful :-) | 09:20 |
| jam | rogpeppe: SIGQUIT output: http://paste.ubuntu.com/1444882/ | 09:20 |
| jam | formatting seems hard to read | 09:21 |
| rogpeppe | jam: i agree, but i lost that argument a while ago (in go core) | 09:21 |
| jam | It looks like the test is hanging in a cleanup section, possibly as a bad side effect of something failing. | 09:21 |
| rogpeppe | jam: it looks to me as if it's hanging trying to connect to mongo | 09:22 |
| rogpeppe | jam: ah, i see where your bad formatting is coming from | 09:22 |
| rogpeppe | jam: you probably just typed ^\ | 09:22 |
| jam | rogpeppe: correct | 09:22 |
| rogpeppe | jam: which is actually ok now on go tip, i believe, but in earlier versions, go test doesn't catch the signal | 09:23 |
| rogpeppe | jam: so you get two stack dumps interleaved | 09:23 |
| jam | rogpeppe: ah, interestting. new dump: http://paste.ubuntu.com/1444889/ | 09:23 |
| rogpeppe | jam: the easiest way around it is to compile the binary with go test -c, then run that | 09:23 |
| jam | using "kill -SIGQUIT" | 09:23 |
| rogpeppe | jam: is mongod actually running? | 09:24 |
| jam | rogpeppe: I've tried it with it alive and dead | 09:24 |
| jam | that one could have been stopped | 09:24 |
| jam | one more with it "start/running" | 09:24 |
| jam | http://paste.ubuntu.com/1444893/ | 09:25 |
| fwereade | rogpeppe, offhand, does a `func (*State)Info() *Info` method sound like a sensible thing to you? | 09:25 |
| rogpeppe | fwereade: i've been wondering about that, off and on | 09:26 |
| rogpeppe | fwereade: i *think* so. it could just return the info it was opened with | 09:26 |
| fwereade | rogpeppe, well, it would be very convenient for the deployer, which needs to tell its deployees how to connect | 09:27 |
| rogpeppe | fwereade: the problem is that it's not always appropriate to use | 09:28 |
| rogpeppe | fwereade: for instance, we might have opened the state with a localhost address, which might not be appropriate to use in a container | 09:28 |
| rogpeppe | jam: could you try it against go tip on amd64, just to see if it's a go issue? | 09:29 |
| fwereade | rogpeppe, well, we're always going to be deploying things which use a modified variant of the deployer's state info anyway, so having another field to swap out isn't the end of the world I think | 09:29 |
| jam | rogpeppe: is there a simple way to do so? | 09:30 |
| fwereade | davecheney, morning | 09:30 |
| jam | rogpeppe: dimitern says he sees the same thing | 09:30 |
| rogpeppe | jam: hg clone the go repo, cd src; all.bash | 09:30 |
| rogpeppe | jam: then set $GOROOT=your-go-repo, and rebuild | 09:31 |
| rogpeppe | jam: (i actually have a separate juju tree that i compile against 1.0.2 rather than tip) | 09:31 |
| rogpeppe | jam: if you just test cmd/juju on its own, do you see the same hangup? | 09:33 |
| fwereade | rogpeppe, but there is a minor wrinkle... so, if I expose an Info method and use that to create the deployer manager, I no longer need to pass in a separate (deploying) entity name, because that is already in the state info | 09:33 |
| fwereade | rogpeppe, and this mostly feels like a win, *so long as* we always want to run an agent against a state opened on that agent's behalf | 09:33 |
| jam | rogpeppe: yes | 09:34 |
| jam | this is doing "cd cmd/juju; go test -gocheck.v -gocheck.vv" | 09:34 |
| fwereade | rogpeppe, that seems reasonable to me, but it will complicate the tests a bit, because the test State is not connected with an EntityName matching the arbitrary unit/machine I want to use for testing | 09:35 |
| rogpeppe | fwereade: the entity name issue makes me feel like this might not be such a great idea | 09:35 |
| rogpeppe | fwereade: because the info isn't just about the state, it's also about who is connecting to the state | 09:36 |
| fwereade | rogpeppe, well, yes, this is the property of the Info that I want to use | 09:36 |
| fwereade | rogpeppe, what I'm trying to figure out is whether it's ok for me to do so or whether it's icky and wrong | 09:37 |
| rogpeppe | fwereade: it means we can't hand a state off to some code without giving them our password, which seems... i dunno, maybe it's not an issue, it just seems potentially so | 09:37 |
| fwereade | rogpeppe, ok, alternative: how would you feel about Addrs() and CACert() methods? | 09:38 |
| rogpeppe | fwereade: that feels less controversial | 09:39 |
| fwereade | rogpeppe, ok, thanks :) | 09:39 |
| rogpeppe | fwereade: although there's still the localhost vs non-localhost address issue | 09:39 |
| fwereade | rogpeppe, well, we always need CACert and we sometimes need Addrs | 09:39 |
| fwereade | rogpeppe, and always have to supply our own EntityName and Password | 09:40 |
| davecheney | fwereade: shouldn't you be somewhere else, drinking tall drinks with umbrellas in them ? | 09:41 |
| fwereade | davecheney, nah, I took a holiday on fri to synergise with the public holiday on thu, working again until EOD weds | 09:42 |
| fwereade | davecheney, that said, when did a breakfast cocktail ever fail to improve the day? | 09:42 |
| * fwereade falls over | 09:42 | |
| * rogpeppe wouldn't mind a bloody mary | 09:43 | |
| davecheney | mmmm, rum | 09:44 |
| davecheney | lads, i am very happy | 09:44 |
| davecheney | juju now works properly in ap-southeast-2 | 09:44 |
| rogpeppe | davecheney: good work! | 09:44 |
| davecheney | also, if anyone see's frank | 09:45 |
| davecheney | please send my apologies | 09:45 |
| davecheney | i had to roll back his change because it broke the initial auth phase | 09:45 |
| davecheney | i don't know how | 09:45 |
| davecheney | and I wasn't going to get into it trying to cut a release | 09:45 |
| davecheney | speaking of that does cmd/jujud import environs/openstack | 09:48 |
| davecheney | i noticed that I didn't have to update the deps when doing the deb build | 09:48 |
| jam | davecheney: probably not yet | 09:53 |
| davecheney | jam: that is fine | 09:56 |
| davecheney | it was only something i noted doing the build | 09:56 |
| fwereade | davecheney, was it maybe just that the jujud tests were expecting the FW to act differently somehow (ie just hadn't been run)? or something weirder? | 10:04 |
| davecheney | nah, the auth code is quite touchy | 10:04 |
| davecheney | rogpeppe: would you say that is a fair statement ? | 10:04 |
| dimitern | jam: the standup is at 10:30 or at 11 utc? | 10:21 |
| rogpeppe | davecheney: any auth code is touchy :-) | 10:27 |
| rogpeppe | davecheney: i'm interested to know how it failed BTW | 10:27 |
| rogpeppe | davecheney: here's an interesting little problem i found when trying to build juju for 386 (to try to replicate jam's problem) | 10:28 |
| davecheney | rogpeppe: revert your working copy to r780 and see | 10:28 |
| rogpeppe | davecheney: if you do, GOARCH=386 GOBIN=/tmp/foo go install launchpad.net/juju-core/cmd/juju | 10:29 |
| rogpeppe | davecheney: the binary doesn't end up in $GOBIN | 10:29 |
| rogpeppe | davecheney: but in $GOBIN/linux_386 | 10:29 |
| rogpeppe | davecheney: which means our install script in environs/tools.go fails to work | 10:30 |
| davecheney | rogpeppe: yup | 10:30 |
| davecheney | cross compile will do that | 10:30 |
| davecheney | GOHOSTARCH=386 ./all.bash | 10:30 |
| davecheney | might help | 10:30 |
| davecheney | is jam using 386 ? | 10:30 |
| davecheney | ewww | 10:30 |
| rogpeppe | davecheney: yes, as far as i could make out | 10:30 |
| rogpeppe | davecheney: see http://paste.ubuntu.com/1444893/ | 10:31 |
| rogpeppe | davecheney: note, for instance: /build/buildd/golang-1.0.2/src/pkg/runtime/zsema_386.c:146 +0x29 | 10:31 |
| jam | davecheney: yes to 386 I believe | 10:31 |
| rogpeppe | davecheney: i'll try the GOHOSTARCH thing | 10:31 |
| davecheney | those pointers are rather small :) | 10:31 |
| jam | dimitern: 11 | 10:32 |
| dimitern | jam: ok, wasn't sure | 10:32 |
| Aram | hello. | 10:32 |
| * davecheney waves at Aram | 10:32 | |
| jam | dimitern: I run juju and launchpad in a VM to avoid having Mongo exposed to the world | 10:33 |
| jam | and I tend to run 386 vms | 10:33 |
| dimitern | Aram: hiya | 10:33 |
| dimitern | jam: and it worked that way? | 10:33 |
| davecheney | jam: honeslty, juju probalby wont' work compiled on 386 | 10:34 |
| davecheney | we've never considered that case | 10:34 |
| davecheney | it'll probably produce a stillborn toolset using juju bootstrap --upload-tools | 10:35 |
| rogpeppe | davecheney: i dunno, why should it be a problem? | 10:35 |
| davecheney | rogpeppe: i'm worried about a default-series kind of 'oh, we never tested that' situation | 10:35 |
| rogpeppe | davecheney, jam: cmd/juju tests pass for me, at any rate | 10:36 |
| jam | I'm pretty sure juju tests worked for me at one point. Though perhaps not on this VM. More oddly, why would it be hanging trying to talk to mgo based on CPU word size? | 10:44 |
| davecheney | jam: i doubt that is the problem | 10:44 |
| davecheney | but i am concerned there are assumptions baked into the tests that assume amd64 | 10:45 |
| davecheney | i'm pretty sure environs/ec2 image tests assume amd64 | 10:45 |
| rogpeppe | jam: just tried with latest juju trunk against i386 and all tests pass for me | 10:45 |
| rogpeppe | jam: haven't tried live tests yet | 10:45 |
| davecheney | rogpeppe: my concern is tests != juju bootstrap --upload-tools | 10:45 |
| rogpeppe | davecheney: exactly, i'm just gonna try live tests | 10:46 |
| Aram | unrelated, juju tests are sometimes flaky in a VM, especially with -gocheck.vv | 10:46 |
| Aram | there's some timing issues exposed only in VMs. | 10:46 |
| rogpeppe | davecheney: but jam's test failures were local ones, and that's what i was trying (and failing) to reproduce | 10:47 |
| jam | Aram: well, it fails with and without -gocheck.vv | 10:48 |
| jam | I just used that to see what was actually hanging, though SIGQUIT worked out better for that | 10:49 |
| rogpeppe | ah, here's a problem: | 10:52 |
| rogpeppe | http://juju-dist.s3.amazonaws.com/tools/mongo-2.2.0-precise-i386.tgz: | 10:52 |
| rogpeppe | 2012-12-17 10:50:22 ERROR 404: Not Found. | 10:52 |
| rogpeppe | other than that, it all seems to have worked ok | 10:53 |
| jam | rogpeppe: the 386 thing is probably a red herring. dimitern is hanging as well, but he's on a amd64 | 11:02 |
| rogpeppe | jam: ah. | 11:02 |
| rogpeppe | jam: i just wanted to make sure that i could run tests ok in a similar env to yours | 11:03 |
| jam | sure. | 11:03 |
| rogpeppe | jam: (and it's an interesting experiment too...) | 11:03 |
| jam | btw, do you know the difference between "apt-get install mongodb" and "apt-get install mongodb-server" ? | 11:03 |
| jam | (I used to have nothing installed, installed mongodb-server, and it stopped the 'could not find binary' failures. I'll try installing the former and see if that changes anything) | 11:03 |
| jam | same failure (in Semacquire during (*mongoCluster).AcquireSocket | 11:05 |
| jam | dimitern is also running on bare metal, vs a VM | 11:05 |
| jam | rogpeppe: rev 780 seems to fail in the same place | 11:09 |
| rogpeppe | jam: hmm. | 11:10 |
| rogpeppe | jam: what version of mgo are you using? | 11:25 |
| rogpeppe | jam: (i.e. what revision number are you on?) | 11:25 |
| fwereade | rogpeppe, jam: lots of nice deletions in https://codereview.appspot.com/6938072 | 11:29 |
| fwereade | dimitern, and you, if you feel like reviewing ^^ | 11:32 |
| jam | mongodb-2.0.6-1ubuntu4, mgo: v2/mgo/ revno 183 | 11:32 |
| dimitern | fwereade: 10x, I'll look at it | 11:32 |
| fwereade | Aram, or you, sorry, I didn't see you come on -- morning :) | 11:32 |
| Aram | yo | 11:33 |
| rogpeppe | fwereade: looking | 11:34 |
| jam | rogpeppe: ^^ | 11:34 |
| jam | (rev 183 of mgo) | 11:34 |
| rogpeppe | jam: darn, same as me | 11:34 |
| mramm | jam: thanks for the update on dependencies, reading through it now. | 11:41 |
| dimitern | fwereade: FWIW you have LGTM from me | 12:21 |
| fwereade | dimitern, cool, thanks | 12:22 |
| fwereade | rogpeppe, dimitern: a followup: https://codereview.appspot.com/6944058 | 13:51 |
| rogpeppe | fwereade: why conflate provisioner and firewaller? is it never reasonable to have one without the other? | 13:54 |
| rogpeppe | fwereade: also, perhaps HostPrincipalUnits would be better as HostUnits - you can't have subordinates without principals. | 13:56 |
| * dimitern looking | 13:56 | |
| rogpeppe | fwereade: (and principals imply subordinates too) | 13:56 |
| fwereade | rogpeppe, at the moment, we always want to run the FW and the P together, and the jobs are explicitly not meant to be 1:1 with workers | 14:03 |
| fwereade | rogpeppe, things may migrate in future | 14:03 |
| fwereade | rogpeppe, re HPA, the MA's job is to host the principal units -- those units' agents may then host subordinates, but the MA doesn't know or care about that | 14:04 |
| rogpeppe | fwereade: from the point of view of AddMachine, i think HostUnits makes sense - we're asking that machine to host any units, and we don't care if it's the machine agent interpreting those flags, or something else. | 14:07 |
| fwereade | rogpeppe, I'm fine with that too :) | 14:11 |
| fwereade | rogpeppe, HostUnits then has an uncomfortable similarity to WatchUnits though | 14:11 |
| fwereade | rogpeppe, HostPrincipalUnits/WatchPrincipalUnits is clearer | 14:12 |
| rogpeppe | fwereade: well, if you think about it, WatchPrincipalUnits is only used for the top level, but that's just an implementation detail - we want the machine to host any kind of unit in the end | 14:12 |
| dimitern | fwereade: done | 14:13 |
| rogpeppe | fwereade: another thing i'm not entirely sure about: there was a nice correlation between worker constant names and the Workers function, but Host* doesn't relate strongly to "job" | 14:14 |
| fwereade | rogpeppe, both the jobs are currently about hosting other things | 14:14 |
| fwereade | rogpeppe, JobHostBlah felt over-wordy | 14:15 |
| rogpeppe | fwereade: so if we had more constants, they might not all have a common element in the names? | 14:15 |
| fwereade | rogpeppe, that might be the case -- I presume you are advocating for Job* | 14:16 |
| rogpeppe | fwereade: i'm not sure. i'm possibly advocating changing AgentJobs to HostJobs or something along that kind of line | 14:16 |
| fwereade | rogpeppe, am definitely fine with that | 14:16 |
| fwereade | rogpeppe, ah not so much with that, sorry | 14:16 |
| fwereade | rogpeppe, well, letme think ;) | 14:16 |
| fwereade | rogpeppe, I think I would maybe like HostPrincipalUnits/RunEnvironController, probably with a Job prefix | 14:18 |
| rogpeppe | fwereade: those names seem, erm, a bit verbose | 14:19 |
| rogpeppe | fwereade: i'm thinking that from a top-down pov, we're declaring what kinds of things a given machine will run | 14:20 |
| rogpeppe | fwereade: so maybe RunUnits, RunEnvironController? | 14:21 |
| rogpeppe | fwereade: i definitely think we should lose the "Principal" - we want a machine to run all units, not just principals | 14:21 |
| fwereade | rogpeppe, I still feel the other way re Principal, but not enough to fight it :) -- Run SGTM | 14:24 |
| rogpeppe | fwereade: actually Task might work | 14:25 |
| rogpeppe | fwereade: UnitsTask, EnvironControllerTask | 14:25 |
| fwereade | rogpeppe, uncomfortable collision with jujud.task | 14:25 |
| rogpeppe | fwereade: that should probably be named worker | 14:26 |
| rogpeppe | fwereade: because it corresponds to the interface implemented by workers | 14:26 |
| fwereade | rogpeppe, well, niemeyer is adamant that there is a distinction between a task and a worker, and an upgrader is not a worker | 14:26 |
| rogpeppe | fwereade: hrmph | 14:27 |
| fwereade | rogpeppe, or that is my understanding, which is probably flawed | 14:27 |
| fwereade | rogpeppe, I don't think there's a meaningful distinction | 14:27 |
| rogpeppe | fwereade: i *think* that was due to the old state Worker constants | 14:27 |
| fwereade | rogpeppe, ha, ok | 14:27 |
| rogpeppe | fwereade: which were the high level description of what to do | 14:27 |
| rogpeppe | fwereade: and i'm proposing to rename those to tasks, to make the distinction clear. | 14:28 |
| rogpeppe | fwereade: that starts to make quite a lot of sense to me, actually. | 14:29 |
| rogpeppe | fwereade: the machine agent uses some number of workers in order to fulfil its allocated tasks. | 14:29 |
| fwereade | rogpeppe, task feels a bit too singular for the intent here, I think -- in my mind I'm not thinking of workers at all, I'm thinking of tasks (ie runTasks(...task)), and I'm thinking of a Job as something that is made up of some number of tasks | 14:30 |
| rogpeppe | fwereade: for me, "task" is a description of something to do, and "worker" is a something that accomplishes that | 14:30 |
| rogpeppe | fwereade: "job" works ok as well as "task", but seems more like something with a defined beginning and end. | 14:31 |
| rogpeppe | fwereade: whereas the tasks we're talking about are indefinite in extent | 14:31 |
| fwereade | rogpeppe, IMO "job" is a pretty good name for a group of ongoing tasks | 14:32 |
| rogpeppe | fwereade: "job" feels like something that can be finished. then again, maybe "task" does too... | 14:33 |
| rogpeppe | fwereade: "we task this machine with the job of running the environment controller" :-) | 14:33 |
| fwereade | rogpeppe, haha | 14:33 |
| fwereade | rogpeppe, (IMO task feels >= job re finishitude) | 14:34 |
| rogpeppe | fwereade: ok. UnitsJob, EnvironControllerJob ? | 14:35 |
| fwereade | rogpeppe, I still want the common prefix for some reason | 14:35 |
| fwereade | rogpeppe, what's the argument the other way? | 14:36 |
| rogpeppe | fwereade: we had Worker as a suffix before; it seems to read more nicely to me, at any rate | 14:36 |
| fwereade | rogpeppe, heh, it always seemed icky to me | 14:36 |
| rogpeppe | fwereade: JobUnits seems... non-obvious. JobRunUnits perhaps | 14:37 |
| rogpeppe | afk, one mo | 14:37 |
| fwereade | rogpeppe, hmm: JobHostUnits/JobManageEnviron? | 14:38 |
| rogpeppe | fwereade: sgtm | 14:39 |
| fwereade | rogpeppe, cool | 14:39 |
| rogpeppe | fwereade: and i think the task interface *should* probably be renamed to worker, but maybe in another cl | 14:44 |
| fwereade | rogpeppe, I'd prefer the opposite change, I will let it stew in my mind for a bit | 14:45 |
| rogpeppe | fwereade: renaming worker to task is a much bigger change | 14:45 |
| rogpeppe | fwereade: we'd have to rename the whole worker package hierarchy | 14:45 |
| rogpeppe | fwereade: (and i quite like the "worker" name currently - it seems to represent what's going on quite well) | 14:46 |
| fwereade | rogpeppe, (I have worse thoughts -- "task/uniter", uniter.NewTask(), etc, which would then let me get a Deployer naming I'm happy with) | 14:46 |
| fwereade | rogpeppe, but, anyway :) | 14:47 |
| rogpeppe | fwereade: i'm not quite sure what you're thinking of there | 14:48 |
| fwereade | rogpeppe, it's really not important -- as I say I will ponder worker/task some more, and perhaps attain enlightenment | 14:48 |
| dimitern | rogpeppe, fwereade: PTAL https://codereview.appspot.com/6940073 | 16:35 |
| * fwereade looks | 16:35 | |
| dimitern | brb | 16:43 |
| dimitern | back | 16:53 |
| rogpeppe | fwereade: you've got a review | 17:26 |
| fwereade | rogpeppe, awesome, ty | 17:27 |
| fwereade | rogpeppe, I dunno -- at the moment it seems that an agent with no jobs is just not a useful thing to have | 17:29 |
| dimitern | rogpeppe: would you ? ^^ | 17:29 |
| rogpeppe | fwereade: it is for tests :-) | 17:29 |
| rogpeppe | dimitern: will do | 17:29 |
| fwereade | rogpeppe, ha, well, yeah -- it always was, though, and that didn't stop us then ;p | 17:30 |
| rogpeppe | fwereade: although a test for no jobs in AddMachine seems reasonable too | 17:30 |
| fwereade | rogpeppe, there is one, isn't there? | 17:30 |
| rogpeppe | fwereade: quite likely, i didn't check | 17:31 |
| fwereade | rogpeppe, I think I wrote one :0 | 17:31 |
| fwereade | rogpeppe, +1 on MachineJob, I think | 17:32 |
| rogpeppe | fwereade: cool | 17:32 |
| fwereade | rogpeppe, then I guess Jobs() rather than AgentJobs() | 17:32 |
| rogpeppe | fwereade: didn't i suggest that? | 17:32 |
| fwereade | rogpeppe, ha, hadn;t read far enough | 17:32 |
| fwereade | rogpeppe, not sure about the bitmask though -- feels like going to extra effort to restrict the number of jobs we have and make it fiddlier to add or remove them (when we eventually want to do that) | 17:34 |
| rogpeppe | fwereade: i don't think it makes it any fiddlier to add or remove jobs | 17:34 |
| rogpeppe | fwereade: and i think most logic gets simpler | 17:34 |
| fwereade | rogpeppe, more complicated txn retries anyway, surely | 17:34 |
| rogpeppe | fwereade: (no need to check for dupes for example) | 17:35 |
| rogpeppe | fwereade: why is the txn retry more complicated? | 17:35 |
| rogpeppe | fwereade: we're just setting an integer, no? | 17:35 |
| fwereade | rogpeppe, $addToSet? | 17:35 |
| fwereade | rogpeppe, we can $addToSet and $pull jobs out of an []int without having to mess around retrying because we're trying to store bitmask X which was dervived from Y which has become Z in the meantime | 17:36 |
| rogpeppe | fwereade: we don't want to use that for jobs, do we? | 17:36 |
| fwereade | rogpeppe, won't we? | 17:37 |
| fwereade | rogpeppe, so the bootstrap machine will be the single bootstrap machine forever? | 17:37 |
| fwereade | rogpeppe, (forgive the terminology) | 17:37 |
| rogpeppe | fwereade: there's nothing to say we can't change the jobs for a machine; it just means we can't have an AddJob/RemoveJob API rather than a SetJobs API. | 17:40 |
| rogpeppe | fwereade: ... which is quite possibly a sufficient reason | 17:40 |
| fwereade | rogpeppe, I *think* it is... or at least it looks close enough from here | 17:41 |
| rogpeppe | fwereade: but even if we store stuff in the database as []int, we could still have a bitmask as the externally visible API | 17:41 |
| fwereade | rogpeppe, is there some advantage of a bitmask I'm not seeing? it seems strictly worse to me -- by the time we have enough flags we care about the extra space of an [], we have enough flags that we're worried about space in a bitmask | 17:42 |
| rogpeppe | fwereade: it's not about space | 17:42 |
| fwereade | rogpeppe, not looping through it? | 17:42 |
| rogpeppe | fwereade: it's about the ease of checking for membership and the automatic duplicate deletion | 17:42 |
| fwereade | rogpeppe, how common are these operations, and how much code do we waste each time we do them? | 17:44 |
| rogpeppe | fwereade: not that common - most code never manipulates a set of jobs. maybe 8 or so lines in the places we do. | 17:44 |
| rogpeppe | fwereade: to me, a bitmask represents like a natural representation of a set | 17:45 |
| rogpeppe | s/represents/seems/ | 17:45 |
| fwereade | rogpeppe, to me, it's something I use when there's a compelling reason to pack stuff into a small space and never besides :) | 17:46 |
| rogpeppe | fwereade: ha | 17:46 |
| rogpeppe | fwereade: C vs Python background i guess :-) | 17:46 |
| Aram | bitmaks are awesome. | 17:46 |
| fwereade | rogpeppe, it's not like I *didn't* spend time up to my elbows in bits, it's just that that time was not generally otherwise representative of sensible development practices ;) | 17:47 |
| rogpeppe | fwereade: using bitmasks as sets actually works pretty well, and isn't unsafe | 17:48 |
| fwereade | rogpeppe, still feels like an [] is the more natural mongo representation, and I'm still not sure the other benefits really outweigh that | 17:50 |
| rogpeppe | fwereade: i think i agree with that. seems like a pity that mongodb doesn't have bitwise ops, but there we go | 17:51 |
| fwereade | rogpeppe, cool, cheers | 17:53 |
| fwereade | dimitern, I'm afraid cath has just gone out without laura, so I won't finish your review any time soon: probably tomorrow :( | 17:57 |
| dimitern | fwereade: sure, no probs | 17:58 |
| fwereade | rogpeppe, dimitern, Aram: but I do have another CL for a horrifying moronic uniter bug: https://codereview.appspot.com/6946071 | 17:58 |
| fwereade | I would very much appreciate reviews of that | 17:58 |
| rogpeppe | fwereade: a surprisingly large change for such a small bug :-) | 18:00 |
| rogpeppe | dimitern: sorry, run out of time on your review. FWIW, i got stuck on getId. i couldn't work out whether it's the right function with a dubious doc comment, or if it should be refactored somehow | 18:13 |
| * rogpeppe is off for the evening. g'night all. | 18:17 | |
| === flaviamissi is now known as flaviamissi_ | ||
| === flaviamissi_ is now known as flaviamissi | ||
| === carif_ is now known as carif | ||
| davecheney | fwereade: ping | 23:35 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!