| davechen1y | niemeyer: you make a good point | 01:17 |
|---|---|---|
| niemeyer | For context, | 01:17 |
| niemeyer | <davechen1y> http://codereview.appspot.com/6497057/ is my proposal | 01:17 |
| niemeyer | <niemeyer> I suspect it's a bit more involved than that | 01:17 |
| niemeyer | <niemeyer> As the comment suggests, we should only push secrets when they are not yet set, and should push *only* secrets | 01:17 |
| niemeyer | <davechen1y> should we take this to the chhanel ? | 01:17 |
| niemeyer | <niemeyer> Sure | 01:17 |
| davechen1y | niemeyer: what happenes currently is AllAttrs - SecretAttrs are pushed on bootstrap | 01:17 |
| davechen1y | so, with 6497057 | 01:17 |
| davechen1y | the set of attrs not pushed would only be the secrets | 01:18 |
| davechen1y | but this is only by coincidence, not design | 01:18 |
| niemeyer | davechen1y: Hmm.. I'm not sure I understand what that means | 01:18 |
| niemeyer | davechen1y: That's what have been aiming at | 01:18 |
| * davechen1y finds code | 01:18 | |
| niemeyer | davechen1y: Also, it's not true that we currently push AllAttrs-SecretAttrs.. it will be so once wrtp's branch goes in with the fix, though | 01:19 |
| davechen1y | niemeyer: hmm, there are unit tests that check this | 01:19 |
| davechen1y | certainly up to the point that they are yaml encoded and passed to cloudinit.Config | 01:20 |
| niemeyer | davechen1y: If we have unittests for this, they're broken or not being run | 01:20 |
| niemeyer | davechen1y: https://codereview.appspot.com/6500052/diff/2001/environs/ec2/ec2.go | 01:20 |
| niemeyer | davechen1y: Line 132 of the left-hand side | 01:20 |
| davechen1y | niemeyer: http://codereview.appspot.com/6458161/patch/17001/15007 | 01:24 |
| niemeyer | davechen1y: Sure, it works because updateSecrets is broken, right? | 01:30 |
| niemeyer | davechen1y: The configuration being tested there is after updateSecrets has set everything over agian | 01:30 |
| niemeyer | sent | 01:30 |
| davechen1y | niemeyer: yes, you are correct | 01:31 |
| davechen1y | niemeyer: http://codereview.appspot.com/6458161/diff/17001/state/state_test.go?column_width=80, rhs, like ~ 88 | 01:32 |
| davechen1y | that doesn't go through the conn | 01:32 |
| davechen1y | so the problem must be the handoff to jujud bootstrap-state via cloudinit | 01:32 |
| niemeyer | davechen1y: I'm not entirely sure about what you're trying to show me.. the test contains the full configuration.. | 01:32 |
| niemeyer | davechen1y: The problem is in the exact line I've shown you above, and wrtp's branch fixes it | 01:33 |
| davechen1y | cool, i'll wait for it to lang | 01:34 |
| davechen1y | land | 01:34 |
| niemeyer | davechen1y: You can also use it as a pre-req if you'd like to build on it | 01:36 |
| niemeyer | davechen1y: It's already approved | 01:36 |
| davechen1y | sweet, will do | 01:36 |
| niemeyer | OK: 1 passed | 02:11 |
| niemeyer | PASS | 02:11 |
| niemeyer | ok launchpad.net/juju-core/mstate/presence 1.111s | 02:11 |
| niemeyer | !!!!! | 02:11 |
| niemeyer | On that note, I'm heading to bed as tomorrow will be an early day | 02:11 |
| * davecheney opens the champaign | 02:11 | |
| niemeyer | davecheney: I wish I could enjoy it with you :) | 02:11 |
| davecheney | TO LISBON! | 02:12 |
| niemeyer | davecheney: Oh yeah! | 02:12 |
| davecheney | MOAR PORT WINE | 02:12 |
| niemeyer | SO TRUE | 02:12 |
| niemeyer | 8) | 02:12 |
| niemeyer | 'davecheney: Have a great working day, and will see you in a bit! | 02:12 |
| davecheney | will do | 02:12 |
| * niemeyer takes off | 02:12 | |
| davecheney | common LP, you can do it | 03:02 |
| davecheney | % lbox propose | 03:02 |
| davecheney | error: Failed to load data for project "juju-core": Get https://api.launchpad.net/devel/juju-core: unexpected EOF | 03:02 |
| wrtp | davecheney: mornin' | 04:59 |
| davecheney | wrtp: hody! | 05:15 |
| davecheney | can you commit your bootstrap branch | 05:15 |
| davecheney | so I can unfuck my cross branch that requiresit | 05:15 |
| * davecheney hates prereqs so much | 05:15 | |
| wrtp | davecheney: sorry for stepping on your toes with the provisioner fix - i'd done the fix before i checked the bug and found it was InProgress | 05:16 |
| wrtp | davecheney: am just about to submit | 05:16 |
| davecheney | wrtp: no appology needed | 05:16 |
| davecheney | don't care, it's fixed :) | 05:16 |
| wrtp | :-) | 05:16 |
| davecheney | i have a secrets only branch to follow | 05:16 |
| wrtp | davecheney: it was great that you'd added the last "happens in the wild" comment to the bug report | 05:16 |
| wrtp | davecheney: 'cos otherwise i wouldn't have found the other firewaller bug | 05:17 |
| davecheney | wrtp: also, nice use of the watcher inside the provisioner test | 05:17 |
| wrtp | davecheney: it was a little more involved than perhaps it should be, but i hate fuckin' timeouts | 05:18 |
| wrtp | davecheney: we have far too many tests that rely on waiting for arbitrary time periods | 05:19 |
| davecheney | wrtp: 110% agree, see my whinge about https://bugs.launchpad.net/juju-core/+bug/1037421 | 05:20 |
| wrtp | davecheney: yeah. | 05:20 |
| wrtp | davecheney: i wonder if we should standardise on using debug log messages to determine "actively doing nothing" | 05:20 |
| wrtp | davecheney: because that's usually what we're testing when we timeout | 05:21 |
| davecheney | wrtp: not sure, like it says in the ticket, i haven't looked into _why_ this happens at all | 05:21 |
| wrtp | davecheney: presence may be different | 05:21 |
| wrtp | davecheney: i looked at the timing of the presence tests BTW, and it's not that there's one test that's slow - there are many slow tests there. | 05:22 |
| davecheney | wrtp: 0% cpu usage while it is waiting, i smell timeout | 05:22 |
| wrtp | davecheney: definitely | 05:22 |
| wrtp | davecheney: just doing final test after merging trunk, then will submit | 05:27 |
| davecheney | kk | 05:27 |
| wrtp | davecheney: the other thing to make tests faster: it would be great if all the tests could use the same state server. | 05:27 |
| davecheney | wrtp: i cant' see how we could do that in zk, but with mongo probably | 05:29 |
| wrtp | davecheney: apparently it is possible in zk (you can do the equivalent of chroot) but mongo is more important now. | 05:30 |
| wrtp | davecheney: submitted | 05:30 |
| davecheney | wrtp: i guess with mgo it would be some sort of unique prefix on the table names | 05:31 |
| wrtp | davecheney: i guess. | 05:32 |
| wrtp | davecheney: BTW do you think that https://codereview.appspot.com/6499056/ is superficial enough to submit without niemeyer's LGTM? | 05:32 |
| davecheney | wrtp: you have my permission to throw me under the bus if niemeyer objects | 05:33 |
| davecheney | i've been submitting build fixes without waiting | 05:33 |
| wrtp | davecheney: i didn't see any buses in lisbon | 05:34 |
| davecheney | you never say LONGO BUS ? | 05:34 |
| wrtp | davecheney: oh yeah, one thing: there was a test in firewaller_test that looked like a debugging remnant. see line 81 of https://codereview.appspot.com/6499056/diff/5001/worker/firewaller/firewaller_test.go | 05:34 |
| wrtp | davecheney: i left it in, but was it there for a particular reason? | 05:35 |
| davecheney | wrtp: dunno, i didn't write it (i think) | 05:35 |
| wrtp | davecheney: actually i came from the airport on a bus. you may be in danger. | 05:35 |
| wrtp | davecheney: frank thought you did actually | 05:35 |
| davecheney | s/say/saw | 05:35 |
| wrtp | davecheney: that bit of it anyway | 05:35 |
| davecheney | wrtp: fuck it, it's called refactoring | 05:36 |
| wrtp | davecheney: yeah. i didn't want to leave out tests that actually checked functionality. | 05:36 |
| wrtp | davecheney: but that just tests that the instance has been started, which we test elsewhere | 05:36 |
| davecheney | # launchpad.net/juju-core/juju | 05:38 |
| davecheney | ./conn.go:95: env.Get undefined (type *config.Config has no field or method Get) | 05:38 |
| davecheney | ./conn.go:96: env.Set undefined (type *config.Config has no field or method Set) | 05:38 |
| davecheney | ./conn.go:99: env.Write undefined (type *config.Config has no field or method Write) | 05:38 |
| davecheney | sadface | 05:38 |
| davecheney | i was using those | 05:38 |
| wrtp | davecheney: we could add Get. | 05:44 |
| wrtp | davecheney: i don't think the other two are appropriate though | 05:45 |
| davecheney | wrtp: what CL changed this | 05:45 |
| davecheney | i'll get my learn on | 05:45 |
| wrtp | davecheney: in fact, i wonder if the lack of Write is a problem | 05:45 |
| wrtp | davecheney: it's been going to happen for ages | 05:45 |
| wrtp | davecheney: frank did it at niemeyer's request | 05:45 |
| wrtp | davecheney: right, you're officially at risk | 05:49 |
| davecheney | wrtp: jolly good | 05:52 |
| davecheney | wrtp: any idea why this happens ? | 06:12 |
| davecheney | ubuntu@server-15347:~/go/src/pkg/net/http$ godoc launchpad.net/juju-core/environs/ec2 | 06:12 |
| davecheney | PACKAGE | 06:12 |
| davecheney | package ec2 import "launchpad.net/juju-core/environs/ec2" | 06:12 |
| wrtp | interesting | 06:12 |
| davecheney | tested from a clean checkout | 06:12 |
| wrtp | davecheney: yes, happens for me too | 06:13 |
| wrtp | davecheney: but works fine on dummy, for example | 06:14 |
| davecheney | how odd | 06:14 |
| * wrtp refuses to get distracted :-) | 06:15 | |
| davecheney | wrtp: i'll lot a bug to figure out WTF that is happening | 06:17 |
| davecheney | can I test the waters with this CL, http://codereview.appspot.com/6492065/ | 06:28 |
| davecheney | I was playing on canonistack today | 06:29 |
| davecheney | it is very possible to do a three line install of juju from source from scratch | 06:29 |
| davecheney | has anyone else run into this issue btw, https://bugs.launchpad.net/juju-core/+bug/1044164 | 06:34 |
| wrtp | davecheney: i think that's a good idea | 06:43 |
| wrtp | davecheney: i think it's a pity that go get doesn't have a way of asking for testing deps too | 06:43 |
| davecheney | wrtp: it's kind of a hack | 06:44 |
| davecheney | but it works | 06:44 |
| davecheney | and is reasonably self contained | 06:44 |
| wrtp | davecheney: given the current limitations of go get, i think it's reasonable. | 06:45 |
| wrtp | davecheney: i've fixed the issue you had above BTW | 06:45 |
| wrtp | davecheney: (it happens all the time for me) | 06:45 |
| davecheney | godoc ? | 06:45 |
| davecheney | or AWS | 06:45 |
| wrtp | davecheney: sig mismatch | 06:45 |
| davecheney | do I need to update goamz ? | 06:46 |
| wrtp | davecheney: i can't remember if i submitted or not | 06:46 |
| wrtp | davecheney: hmm, maybe i never even proposed it | 06:47 |
| wrtp | davecheney: ha, i never even committed the change! | 06:48 |
| wrtp | davecheney: short story: goamz is totally broken currently | 06:48 |
| wrtp | davecheney: fix: add / to the urls | 06:48 |
| * davecheney facepalm | 06:48 | |
| davecheney | ah yes | 06:49 |
| davecheney | bucket.s3.amazon.com is a host | 06:49 |
| davecheney | bucket.s3.amazon.com/ is a url | 06:49 |
| davecheney | or something like that | 06:49 |
| wrtp | davecheney: it signs the path, which should be "/" but is "" | 06:50 |
| davecheney | right | 06:50 |
| davecheney | the root of a URL is always tricky | 06:50 |
| davecheney | well it's not tricky | 06:50 |
| davecheney | it's very simple | 06:50 |
| davecheney | but somehow always catches things out | 06:50 |
| wrtp | davecheney: URLs are fundamentally broken | 06:51 |
| davecheney | wrtp: no argument there | 06:51 |
| wrtp | davecheney: we just hobble around the brokenness | 06:51 |
| wrtp | davecheney: any sequence of slashes should be a separator | 06:51 |
| wrtp | davecheney: like in unix paths | 06:51 |
| wrtp | davecheney: mind you, even in unix it's a bit broken: i think cat /etc/passwd/ should work | 06:52 |
| davecheney | wrtp: not sure I can follow you there | 06:52 |
| davecheney | is a collection | 06:52 |
| davecheney | /index is a resource | 06:53 |
| davecheney | / is a collection | 06:53 |
| wrtp | davecheney: i don't understand that | 06:53 |
| davecheney | put it this way | 06:53 |
| davecheney | / is a dir | 06:54 |
| davecheney | actually never mind | 06:54 |
| davecheney | iw as talking cack | 06:54 |
| wrtp | davecheney: the model becomes really simple if trailing slash (or several) make no semantic difference. | 06:55 |
| wrtp | davecheney: with root being a special case of course, so that we get relative paths. | 06:55 |
| davecheney | wrtp: what does cat /etc/ do ? | 06:57 |
| wrtp | davecheney: depends on the system | 06:58 |
| davecheney | :) | 06:58 |
| davecheney | you've used solaris | 06:58 |
| wrtp | davecheney: once upon a time it gave you the contents of the dir in binary format | 06:58 |
| wrtp | davecheney: (it still does in plan 9, and i think that's ok) | 06:58 |
| davecheney | wrtp: i kinda agree | 06:58 |
| wrtp | davecheney: but the point is {operation /etc/} is the same as {operation /etc} | 06:58 |
| davecheney | i'm not sure if I want /etc and /etc/ to *be* the same thing | 06:59 |
| wrtp | davecheney: they are | 06:59 |
| davecheney | well currently /etc/ isn't, it's an error | 06:59 |
| wrtp | davecheney: no it's not - it's just fine | 06:59 |
| wrtp | davecheney: ls /etc/ | 06:59 |
| wrtp | % ls /etc/ | md5sum | 06:59 |
| wrtp | 910833b8b809292bd85de24a9536df74 - | 07:00 |
| wrtp | % ls /etc | md5sum | 07:00 |
| wrtp | 910833b8b809292bd85de24a9536df74 - | 07:00 |
| davecheney | but then we get to the issue of ls /etc/passwd/ | 07:00 |
| wrtp | davecheney: /etc/passwd/ is just an error because something in the kernel is trying to be clever | 07:00 |
| davecheney | i try not to be clever, to many things have gone wrong | 07:00 |
| wrtp | davecheney: or probably because of the way the namec works internally | 07:00 |
| wrtp | s/the/that/ | 07:00 |
| davecheney | hurry up ec2 | 07:01 |
| wrtp | if it's still called namec any more | 07:01 |
| davecheney | i wanna propose and get to my friends leaving drinks | 07:01 |
| davecheney | namec is the function ? | 07:01 |
| wrtp | in plan 9 you could do cat /etc/passwd/ if /etc/passwd existed :-) | 07:01 |
| wrtp | davecheney: yeah, it's the kernel name lookup function, or used to be | 07:02 |
| wrtp | davecheney: probably not under linux | 07:02 |
| davecheney | damn open sores | 07:02 |
| wrtp | i may well be remembering very badly :-) | 07:02 |
| wrtp | davecheney: my memory is indeed faulty. it's namei. http://minnie.tuhs.org/cgi-bin/utree.pl?file=V7/usr/sys/sys/nami.c | 07:08 |
| davecheney | later lads | 07:09 |
| wrtp | davecheney: cheerio | 07:09 |
| TheMue | morning | 08:32 |
| Aram | moin. | 08:45 |
| TheMue | Aram: hi | 09:09 |
| TheMue | Aram: how are the watchers doing? | 09:31 |
| Aram | I ma fighting bugs. | 09:31 |
| TheMue | Aram: if i can help you let me know. | 09:33 |
| TheMue | Aram: i'm interested in how they are done now. so any paste is welcome. | 09:33 |
| Aram | ok, let me bzr push | 09:33 |
| wrtp | Aram, TheMue, fwereade__: morning | 09:34 |
| Aram | yo. | 09:34 |
| fwereade__ | wrtp, hey | 09:34 |
| TheMue | wrtp: good morning, too | 09:34 |
| fwereade__ | hey everyone else too :) | 09:34 |
| TheMue | fwereade__: and also you ;) | 09:34 |
| wrtp | i've actually been around for over 4 hours this morning, just forgot to say hello earlier! | 09:34 |
| === wrtp is now known as rogpeppe | ||
| Aram | TheMue: lp:~aramh/juju-core/62-mstate-watchers-mux | 09:35 |
| Aram | look in watchserver.go | 09:35 |
| Aram | there are bugs, but that's the general idea | 09:35 |
| TheMue | Aram: great, thx | 09:35 |
| Aram | some types will change, e.g. will use a chan []genericDoc instead of chan genericDoc, some loops will change from m x n to n x m etc. | 09:39 |
| Aram | but now I am debugging the thing first. | 09:39 |
| Aram | ah, there will be e generic unmarshalling function using reflection | 09:40 |
| Aram | rogpeppe: I forgot how can I run a single test. | 09:42 |
| rogpeppe | Aram: -gocheck.f pattern | 09:42 |
| Aram | rogpeppe: thanks, how do I find this information if I need it again? | 09:42 |
| rogpeppe | Aram: run the test binary with -help | 09:43 |
| Aram | ah yes, I was running go test itself with -help | 09:43 |
| TheMue | for standard tests my editor scans the file and provides a shortcut to a menu where all or individual tests can be run. that's nice. | 09:44 |
| rogpeppe | TheMue: do you remember talking about the possibility of using a single watcher for the EnvironConfig for both provisioner and firewaller? | 09:49 |
| rogpeppe | TheMue: it looks like it should be ok to me, but i'm not entirely sure. | 09:49 |
| Aram | btw, in the new watcher implementation, there is a single real watcher. a watch server demultiplexer, so to speak. | 09:49 |
| Aram | it's only one of those per session. | 09:49 |
| TheMue | rogpeppe: yeah, we once talked about it. i don't remember anymore why we chosen watchers for each. | 09:50 |
| rogpeppe | Aram: that sounds like a good move | 09:50 |
| Aram | it gets the data and distributes it to interested parties | 09:50 |
| rogpeppe | Aram: i was hoping for something like that | 09:50 |
| rogpeppe | Aram: in this case though, it's more about saving code than watchers per se | 09:50 |
| TheMue | rogpeppe: they should reference the same environ, so that one SetConfig() should be enought. | 09:50 |
| rogpeppe | TheMue: exactly | 09:50 |
| TheMue | rogpeppe: but there are TWO "should" in that sentence ;) | 09:50 |
| rogpeppe | TheMue: i think i'll do it. shouldn't be hard. and it removes some moving parts (and hence tests) from the firewaller and the provisioner. | 09:51 |
| TheMue | rogpeppe: +1 | 09:52 |
| rogpeppe | func NewProvisioner(environ environs.Environ, st *state.State) *Provisioner | 09:52 |
| TheMue | rogpeppe: it simply sounds more logical | 09:52 |
| rogpeppe | fwereade__, Aram: does that sound reasonable to you? | 09:52 |
| * fwereade__ reads back for context | 09:53 | |
| rogpeppe | fwereade__, Aram: using a single shared environs.Environ for the various workers, and having a separate process watching it and updating it when necessary. | 09:53 |
| fwereade__ | rogpeppe, +1 | 09:53 |
| Aram | lgtm | 09:53 |
| TheMue | Aram: i'm no friend of the reflection based unmarshal idea. ok, it may save a few lines, but we only talk about four internal docs, so your actual way seems fine to me. | 10:00 |
| Aram | yes! the fix I dreamt last night was correct! | 10:29 |
| Aram | dreaming fixes has happened before, but not in a long time. | 10:29 |
| Aram | I tried it in the morning, and thought it didn't work, but my fix was wrong because the zero value of interface{} is nil, not 0. | 10:30 |
| Aram | but what I have dreamt was correct :). | 10:30 |
| fwereade__ | Aram, nice :) | 10:31 |
| Aram | "panic: interface conversion: interface is int, not mstate.Life" | 10:33 |
| Aram | TheMue: ^ ^ this is the reason I want to use reflection, not because it produces less code. | 10:33 |
| Aram | but because the code is necessarily correct and doesn't break when you change the data model. | 10:34 |
| fwereade__ | conversely, I just spent all frickin' morning delving deeeeep into the guts of the charm manager, trying to figure out what's going on, and I *just* realised it was a poorly merged test :( | 10:34 |
| rogpeppe | fwereade__: my fault? | 10:38 |
| fwereade__ | rogpeppe, nope, 100% mine | 10:39 |
| rogpeppe | fwereade__: my commiserations | 10:39 |
| rogpeppe | fwereade__: i hate it when that happens | 10:39 |
| Aram | YES! everything works. | 10:39 |
| rogpeppe | fwereade__: the bright side is you usually come out with a better understanding of everything goingon | 10:39 |
| Aram | fixed the damn thing. | 10:39 |
| rogpeppe | fwereade__: and probably made some fixes while you were doing it | 10:39 |
| fwereade__ | rogpeppe, sadly in this case all I have done is come up with a series of ever-more-batshit theories about how git works, and I fear they may pollute my brain for a while ;) | 10:40 |
| rogpeppe | fwereade__: oh well | 10:40 |
| Aram | TheMue: you might want to pull again, to see working watchers. | 10:40 |
| Aram | tests pass now | 10:40 |
| * fwereade__ cheers at Aram | 10:41 | |
| Aram | it's by far the most complex piece of machinery in mstate, sadly. | 10:41 |
| Aram | but I am happy with it from many perspectives. | 10:42 |
| TheMue | Aram: great | 10:52 |
| TheMue | Aram: and yes, the server is a little monster ;) | 10:52 |
| rogpeppe | Aram: which piece is this? | 10:52 |
| Aram | rogpeppe: lp:~aramh/juju-core/62-mstate-watchers-mux | 10:52 |
| rogpeppe | Aram: i will have a look :-) | 10:53 |
| Aram | watchserver.go is the real thing, watcher.go is just a quick hack to be able to validate the design. | 10:53 |
| rogpeppe | environ watchers ripped out of firewaller and provisioner. all tests pass (some new tests needed though). am happy. | 10:53 |
| TheMue | Aram: btw, I'm just doing the EnvironConfig change for mstate | 10:54 |
| Aram | TheMue: what change? | 10:54 |
| TheMue | rogpeppe: great., i hope https://codereview.appspot.com/6488057/ will then still run too | 10:54 |
| TheMue | Aram: state.EnvironConfig now returns an environs/config.Config and there's an additional SetEnvironConfig | 10:55 |
| Aram | ok. | 10:55 |
| TheMue | Aram: test is already working here | 10:55 |
| rogpeppe | have we got a meeting now? | 11:01 |
| davecheney | waiting on gustavo and mramm | 11:02 |
| rogpeppe | davecheney: just a quick query | 11:02 |
| mramm | I'm on my way | 11:02 |
| niemeyer | Morning all | 11:02 |
| niemeyer | Do we have invites sent yet? | 11:02 |
| mramm | not yet | 11:02 |
| mramm | that i see | 11:02 |
| davecheney | rogpeppe:mmm | 11:02 |
| mramm | inviting everybody now | 11:02 |
| rogpeppe | davecheney: i decided that it would be a good thing if the provisioner didn't listen for environ config changes itself | 11:03 |
| rogpeppe | davecheney: so something else listens and changes the environ; everyone uses that environ | 11:03 |
| rogpeppe | davecheney: does that sound reasonable to you? | 11:03 |
| davecheney | rogpeppe: why the reason for the change ? | 11:03 |
| rogpeppe | davecheney: i was going to have to have exactly the same code in three places | 11:04 |
| davecheney | it sounds like my beloved proxy environ idea :) | 11:04 |
| rogpeppe | davecheney: no need for a proxy i think | 11:04 |
| mramm | https://plus.google.com/hangouts/_/4ca5fa037ece34849caa79686808bc9e75cfae19 | 11:04 |
| Aram | ah, meeting today? | 11:04 |
| rogpeppe | Aram: yup | 11:05 |
| mramm | https://launchpad.net/juju-core/+milestone/1.3 | 11:06 |
| rogpeppe | fwereade__: meeting? | 11:06 |
| fwereade__ | rogpeppe, whoops, ty | 11:06 |
| === Aram2 is now known as Aram | ||
| davecheney | Aram: https://wiki.canonical.com/UbuntuEngineering/Sprints/JujuCoreIntegration | 11:48 |
| * Aram goes to lunch | 11:58 | |
| TheMue | lunchtime | 12:00 |
| niemeyer | case change, ok := <-fw.environWatcher.Changes(): | 12:06 |
| niemeyer | err := fw.environ.SetConfig(change) | 12:06 |
| rogpeppe | niemeyer: http://paste.ubuntu.com/1177699/ | 12:07 |
| === ChanServ changed the topic of #juju-dev to: Milestone goals: 1) mstate in, state out 2) uniter working with simple charms; 3) upgrade-juju working | ||
| davecheney | night guys | 12:22 |
| niemeyer | rogpeppe: http://pastebin.ubuntu.com/1177736/ | 12:35 |
| niemeyer | rogpeppe: fw.environ = environ | 12:46 |
| niemeyer | rogpeppe: worker.SetEnvironConfig(fw.environ, config) | 12:46 |
| niemeyer | rogpeppe: https://codereview.appspot.com/6488057/diff/1/worker/provisioner/provisioner_test.go | 12:49 |
| niemeyer | rogpeppe: case fw.environTest <- fw.environ: // do nothing | 12:52 |
| niemeyer | rogpeppe: someworker.SetEnvironWatcher | 12:53 |
| niemeyer | rogpeppe: someworker.SetEnvironObserver | 12:54 |
| niemeyer | rogpeppe: someworker.SetEnvironObserver(ch) | 12:54 |
| niemeyer | rogpeppe: case fw.environObserver <- fw.environ: // do nothing | 12:54 |
| rogpeppe | niemeyer: http://paste.ubuntu.com/1177767/ | 13:05 |
| rogpeppe | niemeyer: perhaps you could mention to TheMue the thoughts you had on the worker tests, re: the above provisioner_test.go CL. | 13:11 |
| niemeyer | rogpeppe: Will do | 13:11 |
| niemeyer | Will get some breakfast first, though! | 13:14 |
| rogpeppe | niemeyer: are we concerned about what might happen if an upgrade is done concurrently with an environment setting change? | 13:49 |
| niemeyer | rogpeppe: In which sense? | 13:49 |
| rogpeppe | niemeyer: would it be acceptable for the upgrade request to be lost, for example? | 13:49 |
| rogpeppe | (in that case) | 13:49 |
| niemeyer | rogpeppe: Are you alluding to the race in SetEnvironConfig? | 13:51 |
| rogpeppe | niemeyer: yeah. | 13:51 |
| niemeyer | rogpeppe: Not a problem we have to worry for a while, IMO | 13:51 |
| rogpeppe | niemeyer: well, the race that happens when you must do: EnvironConfig followed by SetEnvironConfig. | 13:51 |
| niemeyer | rogpeppe: yeah | 13:51 |
| rogpeppe | niemeyer: ok, i thought so, but just wanted to check explicitly | 13:51 |
| rogpeppe | niemeyer: thanks | 13:52 |
| niemeyer | rogpeppe: We should fix that at some point, probably by having a method that enables applying deltas safely, but this is a nice problem for us to have when things actually work | 13:52 |
| niemeyer | rogpeppe: In practice, fiddling with the env is rare enough that it won't be an issue anytime soon | 13:52 |
| rogpeppe | niemeyer: definite agreement there. and fiddling with the env *concurrently* is even rarer. | 13:53 |
| niemeyer | rogpeppe: Yeha | 13:53 |
| fwereade__ | niemeyer, before I polish this properly, can I get another pre-review sanity check on the charm upgrades please? https://codereview.appspot.com/6488062 | 14:00 |
| niemeyer | fwereade__: Sure | 14:02 |
| niemeyer | fwereade__: I'll just finish something and will be with you | 14:02 |
| fwereade__ | niemeyer, cheers | 14:02 |
| niemeyer | Flights booked | 14:21 |
| niemeyer | fwereade__: Haven't reviewed it entirely, but sent some pre-review comments | 14:29 |
| fwereade__ | niemeyer, thanks, that's what I was after -- and sorry to keep bugging you with this one | 14:29 |
| niemeyer | fwereade__: It still looks like you're fighting with the awkwardness of having the charm manager handling responsibilities which ought to be elsewhere | 14:30 |
| fwereade__ | niemeyer, it seems excessive to me to have to keep *another* state file around to allow the uniter to keep track of what operation it's performing | 14:31 |
| fwereade__ | niemeyer, it feels to me that being able to determine restart state purely from charm state and hook state is a good thing | 14:32 |
| fwereade__ | niemeyer, charm state + hook state + uniter state feels wrong -- especially since IMO charm state and hook state are useful simplifying features | 14:32 |
| fwereade__ | niemeyer, but once we're dealing with uniter state as well they actually become redundant and make life more complex | 14:32 |
| fwereade__ | niemeyer, or have I misunderstod something? | 14:33 |
| fwereade__ | niemeyer, whereas just making sure that the charm op and the hook op overlap STM like the clearest way to ensure that the appropriate ops are followed by appropriate hooks | 14:34 |
| niemeyer | / charm directory, it returns ErrConflict. cbSuccess will be called before | 14:34 |
| niemeyer | / status is set from st back to Deployed, to give clients the opportunity | 14:34 |
| niemeyer | / to complete dependent actions. | 14:34 |
| niemeyer | fwereade__: This is a hack | 14:34 |
| niemeyer | fwereade__: This is a charm.Manager.. | 14:35 |
| niemeyer | fwereade__: It has deployed the charm | 14:35 |
| niemeyer | fwereade__: There are no "dependent actions" | 14:35 |
| niemeyer | fwereade__: It has done it | 14:35 |
| niemeyer | fwereade__: No conflicts, no problems.. it's finished its job | 14:35 |
| niemeyer | fwereade__: It doesn't depend on anything else to say it has finished its job either | 14:35 |
| niemeyer | fwereade__: We can talk about different ways in which we can solve that, but first thing is to understand what we're doing and why | 14:36 |
| fwereade__ | niemeyer, ok -- what I am trying to do is ensure that a charm operation is always followed by the apropriate hook | 14:37 |
| fwereade__ | niemeyer, ISTM that the two ways to do that are to overlap the operations, so the charm op ends after the hook op has begun; or to add *another* layer of state that's specific to the uniter, which feels redundant | 14:38 |
| niemeyer | fwereade__: So what you mean is that you'd rather do the whole upgrade again than acknowledge the fact we can tell the merging has been done? | 14:39 |
| fwereade__ | niemeyer, er, I don't think that's what I'm saying at all | 14:40 |
| niemeyer | fwereade__: That's what's in the code | 14:40 |
| niemeyer | fwereade__: If cbSuccess fails, we never set the status to deployed, despite the fact it *is deployed* | 14:40 |
| fwereade__ | niemeyer, so what? same thing happens if writing "Deployed" fails | 14:41 |
| niemeyer | <niemeyer> fwereade__: So what you mean is that you'd rather do the whole upgrade again than acknowledge the fact we can tell the merging has been done? | 14:41 |
| fwereade__ | niemeyer, are yu telling me I should be parsing git logs or something? | 14:42 |
| niemeyer | fwereade__: No.. I'm telling that we shouldn't have a cbSuccess that prevents us from acknowledging the merge has been done, for purposes that are completely external to the concerns of the Manager | 14:42 |
| niemeyer | fwereade__: Divide and conquer.. the Manager is doing a lot | 14:43 |
| niemeyer | fwereade__: We should have a pleasant API to work with, that makes sense by itself | 14:43 |
| niemeyer | fwereade__: I don't buy that we should be breaking abstractions and injecting that kind of weirdness because we can save a file from being written to disk elsewhere | 14:43 |
| fwereade__ | niemeyer, I am trying, but I don't see the middle ground between what I suggest and dropping independent charm and hook state entirely | 14:45 |
| fwereade__ | niemeyer, if I've got a super-state that tells me how to interpret charm and hook state, doesn't it end up noticeably simpler to simply write every operation out in just one place? ...but then we lose many of the benefits of separating out /charm and /hook | 14:47 |
| fwereade__ | niemeyer, or...... hum, I think I see | 14:47 |
| fwereade__ | niemeyer, the uniter state is basically independent of hook state and exists purely to manage the transition from charm op to associated hook? | 14:48 |
| niemeyer | fwereade__: Possibly. I'm not even entirely sure (yet) we need an extra state, to be honest | 14:49 |
| fwereade__ | niemeyer, ok -- the problem I am trying to avoid is: write-charm; set-deployed; whoops-we-crashed-before-run-hook | 14:50 |
| niemeyer | fwereade__: If the uniter decides it needs to upgrade, what happens within the uniter itself? | 14:50 |
| niemeyer | fwereade__: Yeah, I kind of see it.. I just don't have as good a view on the details as you do | 14:50 |
| niemeyer | fwereade__: The basic semantics I imagine for the manager, in isolation is: write-charm-and-url, set-deployed | 14:51 |
| niemeyer | fwereade__: If there is a crash, the uniter should have a mechanism that tells it was going to over a charm upgrade.. it will try to write the charm again, and the manager will go "Ah, don't worry, that's what I have" | 14:52 |
| niemeyer | fwereade__: What is that mechanism? | 14:53 |
| fwereade__ | niemeyer, the mechanism ATM is charm.ReadState | 14:53 |
| niemeyer | fwereade__: Okay, maybe we can still keep it like that | 14:53 |
| niemeyer | fwereade__: If there's really nothing else we need in terms of uniter information to make that decision | 14:54 |
| fwereade__ | niemeyer, well... ok -- but the fact that the charm is deployed is not in itself enough for the uniter to know that it has completed all relevant operations (ie running a ollowup hook) | 14:55 |
| niemeyer | fwereade__: Yeah, there's a missing state | 14:55 |
| fwereade__ | niemeyer, so it's uniter state? or are you thinking of something else? | 14:55 |
| fwereade__ | niemeyer, or a missing state within an existing set of states? | 14:56 |
| niemeyer | fwereade__: No, it may be fine to do that within the manager if that makes it simple and doesn't disturb the manager itself | 14:56 |
| niemeyer | fwereade__: E.g. | 14:56 |
| niemeyer | fwereade__: Installing, Installed, InstalledAck, Upgrading, Upgraded, UpgradedAck | 14:57 |
| fwereade__ | niemeyer, ah, ok, so the Manager is responsible or writing most o those states but the Uniter writes the Ack states afterwards? | 14:58 |
| niemeyer | fwereade__: yeah | 14:58 |
| niemeyer | fwereade__: Or even, Ack()s :-) | 14:58 |
| fwereade__ | niemeyer, honestly that feels like a step back from what I originally proposed -- it smears the charm-state-writing responsibility into 2 places instead of one, and that eels to me like it's harder to ollow? | 14:59 |
| fwereade__ | hm, my F key appears to be flaky, sorry | 14:59 |
| niemeyer | fwereade__: I'd be glad to understand why you think that's the case | 14:59 |
| fwereade__ | niemeyer, well, beforehand I had a single func (changeCharm) that did all the writing of charm state | 15:01 |
| niemeyer | fwereade__: :-) | 15:01 |
| niemeyer | fwereade__: I hope that's not what you're proposing now :) | 15:01 |
| fwereade__ | niemeyer, feels analogous to hook state, which while it's in 2 funcs is at least all in the same package | 15:01 |
| niemeyer | fwereade__: (as a single function of 300 lines might look bad) | 15:01 |
| fwereade__ | niemeyer, I'm talking specifically about the charm state file | 15:01 |
| niemeyer | fwereade__: I feel like part of the reason we're having so much trouble is precisely that file | 15:02 |
| niemeyer | fwereade__: I'd like to kill the idea that we ReadState and WriteState | 15:02 |
| niemeyer | fwereade__: We have a Manager, that manipulates a charm directory | 15:02 |
| niemeyer | fwereade__: Anyone that is not the Manager should talk to the Manager to change its state | 15:02 |
| fwereade__ | niemeyer, ah! if we can move all that state-writing into uniter itself, I'd be entirely happy | 15:03 |
| niemeyer | fwereade__: LOL | 15:03 |
| niemeyer | fwereade__: I was thinking the opposite :-) | 15:03 |
| niemeyer | fwereade__: But, tell me about that idea | 15:03 |
| fwereade__ | niemeyer, fuzzily, I think I'm thinking that the charm state is actually uniter operation state | 15:04 |
| niemeyer | fwereade__: Ok | 15:04 |
| niemeyer | fwereade__: Makes sense | 15:04 |
| niemeyer | fwereade__: What would the Manager look like in such a world? | 15:05 |
| fwereade__ | niemeyer, I think the manager would be more of a Deployer | 15:05 |
| fwereade__ | niemeyer, ...except maybe not quite... | 15:05 |
| fwereade__ | niemeyer, aside: when I originally moved the charm dir out of the manager, it really did feel like a step forward, but I ended up not going anywhere very good from there | 15:07 |
| niemeyer | fwereade__: Yeah ,the problem is drawing the line at the right place.. you probably were onto a good direction in some sense, but the line got blurred somehow | 15:07 |
| niemeyer | fwereade__: The idea of a deployer makes sense to me.. | 15:08 |
| niemeyer | fwereade__: The only state we need within the deployer is probably the URL that is actually on disk (what we have as readURL) | 15:09 |
| fwereade__ | niemeyer, yeah, exactly | 15:09 |
| fwereade__ | niemeyer, ok, so assuming we were able to separate out a charm dir, a deployer, and a charm-operation-state file | 15:09 |
| niemeyer | fwereade__: In the uniter state, I suspect we'll still need extra states, though | 15:10 |
| niemeyer | fwereade__: Which some of the causation of the discussion above | 15:10 |
| niemeyer | fwereade__: Since we need to track "need to deploy => deployed and need to run hook => run hook" | 15:10 |
| fwereade__ | niemeyer, I *think* that works OK though -- once we're deployed, yay, we're deployed -- but the state is still the uniter's responsibility to recover on unexpected abort | 15:11 |
| niemeyer | fwereade__: Yep | 15:11 |
| niemeyer | fwereade__: So State, ReadState and WriteState are all gone..? | 15:11 |
| fwereade__ | niemeyer, well, I think they are divorced from Manager anyway | 15:12 |
| niemeyer | fwereade__: Yep | 15:12 |
| fwereade__ | niemeyer, they still feel pretty "charm"y | 15:12 |
| niemeyer | fwereade__: Not to me | 15:12 |
| niemeyer | fwereade__: Anymore than the unit is charmy, anyway | 15:12 |
| niemeyer | fwereade__: "I need to run hook", as an example | 15:12 |
| fwereade__ | niemeyer, I can see it both ways | 15:12 |
| niemeyer | fwereade__: Is pretty unity :) | 15:13 |
| fwereade__ | niemeyer, yeah, indeed :) | 15:13 |
| fwereade__ | niemeyer, ok, so, as a rough sketch: charm.Deployer + charm.GitDir; then some unexported uniter state manipulation in changeCharm around Deployer operations? | 15:14 |
| fwereade__ | niemeyer, the readURL thing starts to feel pretty redundant now | 15:14 |
| niemeyer | fwereade__: +1 except for the last sentence | 15:16 |
| niemeyer | fwereade__: The URL has to be put as part of the deployed content if we want to be able to easily tell what's in the directory | 15:17 |
| niemeyer | fwereade__: The uniter state can't track what's *actually* there (after facing a revert, or merge, or whatever) | 15:17 |
| fwereade__ | niemeyer, hmm, I'll think about it a bit more... ah, ok, we could quite easily have a *missing* operation state file meaning "deployed, check the charm dir" | 15:17 |
| fwereade__ | niemeyer, ok, I think maybe I have direction again | 15:18 |
| fwereade__ | niemeyer, cheers | 15:18 |
| niemeyer | fwereade__: I don't see that as something entirely neat.. if we have state, we can easily name it.. | 15:18 |
| niemeyer | fwereade__: Having the same URL both in the uniter state and in the charm is a great sign of things being right :) | 15:19 |
| fwereade__ | niemeyer, ok, I think my resistance may come from not being able to figure out how I should react if they don't match :) | 15:19 |
| niemeyer | fwereade__: They serve different purposes, anyway | 15:19 |
| niemeyer | fwereade__: Example scenario: | 15:20 |
| niemeyer | fwereade__: We're running an upgrade from revision 10 to 11 | 15:20 |
| niemeyer | fwereade__: uniter state says we're in revision 10, trying to install 11 | 15:21 |
| niemeyer | fwereade__: we ask the deployer to upgrade, and get a conflict | 15:21 |
| niemeyer | fwereade__: unit goes into error mode | 15:21 |
| niemeyer | fwereade__: user logs into the machine.. fiddles around, reverts | 15:22 |
| niemeyer | fwereade__: user calls resolved | 15:22 |
| niemeyer | fwereade__: uniter catches up, learns that directory has revision 10.. doesn't call upgrade hook, goes back into working mode | 15:22 |
| niemeyer | fwereade__: End of story point. | 15:22 |
| fwereade__ | niemeyer, feels like the next stepis actually "goto10" | 15:23 |
| niemeyer | fwereade__: ? | 15:24 |
| fwereade__ | niemeyer, uniter returns to working mode; detects an upgrade, and... | 15:24 |
| niemeyer | fwereade__: That's another story | 15:24 |
| niemeyer | fwereade__: At the end of the story above, it gets back onto the main loop, and can do anything it usually would | 15:25 |
| niemeyer | fwereade__: Including upgrading again, sure.. perhaps not even to the same version. | 15:25 |
| rogpeppe | niemeyer: this might be something like we discussed: https://codereview.appspot.com/6501072 | 15:27 |
| fwereade__ | niemeyer, I don't see the value of that story, because what *will* happen is another upgrade, and we can easily and automatically handle a revert-and-upgrade-to-new-version ourselves, even from an error state | 15:27 |
| fwereade__ | niemeyer, without a separate actually-don't-upgrade-this mechanism, I don't see why a user would ever need or want to revert | 15:28 |
| niemeyer | fwereade__: The value of that story is that the only way to make it possible is by knowing what's actually inside the directory | 15:28 |
| niemeyer | fwereade__: Ah, maybe it's that's the disagreement.. I totally see why people (or I :-) would like to resolve a conflict, and then revert back to the previous state | 15:29 |
| niemeyer | fwereade__: Because what was done was wrong | 15:29 |
| niemeyer | fwereade__: Even then, we're diving into too much detail here | 15:29 |
| fwereade__ | niemeyer, are we talking about one specific unit, or a whole service? | 15:29 |
| niemeyer | fwereade__: The core of my argument is that there's value in knowing what a given revision of the charm directory actually contains | 15:29 |
| niemeyer | fwereade__: I was hoping that this wouldn't be very controversial | 15:30 |
| fwereade__ | niemeyer, neither keep-the-url-in-the-charm-dir nor keep-the-url-in-a-state-file are individually controversial in my mind | 15:30 |
| niemeyer | rogpeppe: Cheers, will look | 15:30 |
| fwereade__ | niemeyer, keeping *both* of them still makes me nervous | 15:30 |
| fwereade__ | niemeyer, because I'm not sure that we can do anything useful with disagreements | 15:31 |
| niemeyer | fwereade__: Okay, I'm happy to have it just within the charm directory while we don't figure a reason to have it outside | 15:31 |
| fwereade__ | niemeyer, hence my contention that what we're after is *operation* state, not charm state, and we shouldn't have a URL in operation state when we're not operating | 15:31 |
| niemeyer | fwereade__: I don't know how you'll manage to tell what version you're supposed to upgrade to before the upgrade actually happens, but that's part of the "while we don't figure a reason" part | 15:32 |
| niemeyer | fwereade__: Okay, that makes sense | 15:32 |
| fwereade__ | niemeyer, if I go one step further and say we shouldn't have *any* operation state when we're not operating, are you still comfortable? | 15:33 |
| fwereade__ | niemeyer, blast, sorry, brb | 15:35 |
| niemeyer | fwereade__: Yeah, I'd definitely not complain if you made that work nicely | 15:35 |
| fwereade__ | niemeyer, cool, I'll have another crack at it | 15:40 |
| fwereade__ | niemeyer, sorry this bit's taking so long | 15:40 |
| rogpeppe | flight to lisbon booked | 15:52 |
| niemeyer | fwereade__: No problem at all, I appreciated that conversation | 15:57 |
| niemeyer | rogpeppe: Reviewed, good stuff | 15:57 |
| rogpeppe | niemeyer: thanks | 15:57 |
| fwereade__ | niemeyer, a pleasure, as always :) | 15:58 |
| rogpeppe | niemeyer: i thought about Errer, but couldn't quite bring myself - it just looks like a spelling mistake. still, if you think it's ok... | 15:58 |
| niemeyer | rogpeppe: It crossed my mind, but it still feels better than Errorer (which I'm probably unable to pronounce :-) | 15:59 |
| rogpeppe | fwereade__: o fellow english speaker: what do you think: Errer or Errorer ? (as a name for interface{Err() error}) | 16:02 |
| rogpeppe | fwereade__: or some alternative? | 16:02 |
| fwereade__ | rogpeppe, Errer is an ugly word, but also seems like a legitimate "that which errs", so I think I'd go with that | 16:03 |
| Aram | rename Err to Error and use the builtin error interface? | 16:03 |
| rogpeppe | Aram: that would be kinda cool but... do we want all watchers to be errors ? | 16:04 |
| Aram | rogpeppe: ^ | 16:04 |
| rogpeppe | Aram: it would also be a far reaching change | 16:04 |
| rogpeppe | Aram: tomb, for instance, is an external package that uses Err() | 16:05 |
| niemeyer | rogpeppe, Aram: Nope, Err is the proper name for that method, and it's actually a standard convention | 16:05 |
| niemeyer | rogpeppe, Aram: Err is seen in things that report an "error" (the interface); they are not errors themselves | 16:06 |
| rogpeppe | niemeyer: +1 having looked in the go source | 16:06 |
| Aram | white:pkg$ pwd | 16:06 |
| Aram | /home/aram/go/src/pkg | 16:06 |
| Aram | white:pkg$ csh '\.Err\(' | wc -l | 16:06 |
| Aram | 21 | 16:06 |
| Aram | it's only used in database and exp/html | 16:06 |
| niemeyer | Aram: Yeah, but it's an agreed-to convention, and I actually pushed the patch prior to Go 1 to make it consistent | 16:07 |
| rogpeppe | Aram: yeah - it wouldn't be right anyway because Error (error's method) returns a string not an error | 16:07 |
| rogpeppe | Aram: which we definitely don't want to do | 16:07 |
| Aram | actually the very few .Err() in the go tree return error, not string. | 16:09 |
| Aram | so while I agree that changing to error is not a good idea, calling three usages of an Err in 400,000 LOCs hardly counts as convention, especially since our Err has a different signature :) | 16:09 |
| niemeyer | rogpeppe: https://codereview.appspot.com/6501069/ reviewed too | 16:12 |
| rogpeppe | niemeyer: ta! | 16:12 |
| niemeyer | rogpeppe: Thank you! | 16:13 |
| rogpeppe | niemeyer: i put the numbering in because it was so painful counting through 13 tests to find which one had failed. | 16:15 |
| rogpeppe | niemeyer: the alternative is to put the comment in the struct | 16:15 |
| rogpeppe | niemeyer: which is probably better, come to think of it | 16:15 |
| niemeyer | rogpeppe: Yes, there are multiple ways to solve that.. putting the comment as a summary in the struct is a generally a great one | 16:15 |
| rogpeppe | niemeyer: i'll go with that then | 16:15 |
| niemeyer | rogpeppe: Thanks! | 16:15 |
| niemeyer | Stepping out for lunch | 16:16 |
| Aram | yay, I can use chan []map[string]interface{} as a map key | 16:18 |
| * Aram wished Put wasn't near Undo in acme. | 16:25 | |
| rogpeppe | Aram: i don't get bitten by that very much, though i wish Put didn't appear under the mouse when you get to the end of a load of Redos... | 16:35 |
| Aram | yeah, that too | 16:35 |
| rogpeppe | Aram: i have never thought of a good way to make it better though | 16:35 |
| rogpeppe | yay, ec2 is fixed again! | 16:35 |
| niemeyer | Aram: Gosh, do we need that as a map key? :) | 17:20 |
| Aram | yeah... | 17:20 |
| niemeyer | Aram: Why? | 17:20 |
| Aram | it's not that bad, it's only chan []genericDoc, genericDoc is map[string]interface{} because it represents any mongodb document (like bson.M). we need it because this is how client watchers register with the watch server. | 17:22 |
| Aram | the code is actually quite clear, even if the key is complex. | 17:23 |
| Aram | the key is what is is because it is the way the server sends event to clients. | 17:23 |
| Aram | clients expect slices of documents. | 17:23 |
| niemeyer | Aram: Hmm.. that sounds a bit suspect | 17:23 |
| niemeyer | Aram: Why do we have a genericDoc in lower level watcher? | 17:24 |
| niemeyer | Aram: That wasn't in the design we talked about | 17:24 |
| Aram | what is a lower level watcher? | 17:24 |
| niemeyer | Aram: The design we talked about, which I thought you've been working on | 17:24 |
| Aram | I have. | 17:25 |
| niemeyer | Aram: Well, that wasn't there | 17:25 |
| niemeyer | Aram: We don't work with genericDocs in mstate so far | 17:25 |
| niemeyer | Aram: The fact we can deal with actual values instead is quite convenient | 17:25 |
| niemeyer | Aram: I'm concerned that what we talked about was simpler than what seems to be taking to get it in place | 17:26 |
| Aram | there is a single entity doing all the queries for all the watchers. watchers have different documents, it has to use a map to be able to get data before converting it to a document. | 17:26 |
| niemeyer | Aram: That's not what is in the design | 17:27 |
| niemeyer | https://gist.github.com/e1453bb97426081a12e5 | 17:27 |
| niemeyer | Aram: This is the design | 17:27 |
| Aram | "A Watcher can watch any number of documents and collection" that implies arbitrary documents, hence maps instead of structs. | 17:28 |
| niemeyer | Aram: You don't have to agree with it, and it's fine to go in a different direction, but when I go over the trouble of doing that after we stumbled upon several issues, I do expect feedback before we change gears | 17:28 |
| niemeyer | Aram: Where's genericDoc in this design? | 17:28 |
| Aram | the fact that you didn't mention genericDoc doesn't mean I can't use it, you didn't mention error or string or int either. genericDoc is just a name I gave for a map, I can change it to bson.M if you want. | 17:30 |
| Aram | because it is essentially bson.M, not only in representation, but in concept as well. | 17:30 |
| niemeyer | Aram: I don't care about the name.. I care about the fact I can't see a "generic map" or however you wanna call it in this design | 17:30 |
| niemeyer | Aram: We simply do not *load* the document | 17:30 |
| Aram | surely someone loads documents as the events delivered by watchers are full entities that have the full document inside them. | 17:32 |
| niemeyer | Aram: I'm talking about the design in this page, not about whatever you're implementing on top of it | 17:32 |
| niemeyer | Aram: This is a building block that we need, and agreed upon | 17:32 |
| niemeyer | Aram: You seem to be talking about stuff that is not in this design, despite the fact we don't have this integrated or up for review anywhere' | 17:33 |
| niemeyer | Aram: We seem stuck despite all my efforts, and I'd like to understand why | 17:33 |
| * rogpeppe loves all it when 90 lines of new test code all passes after one minor initial glitch. | 17:39 | |
| rogpeppe | somehow the initial glitch makes it even better, because you have positive verification that the test is actually running. | 17:39 |
| niemeyer | rogpeppe: True | 17:40 |
| niemeyer | rogpeppe: When I get a straight pass I'll generally tweak things a bit to make sure it's *really* passing :) | 17:40 |
| rogpeppe | niemeyer: i often leave a "this cannot pass" piece of code in the test just to make sure | 17:41 |
| rogpeppe | niemeyer: easier when you've got an ErrorMatches test | 17:41 |
| niemeyer | rogpeppe: +1, done that before as well | 17:42 |
| rogpeppe | 5 minutes and 21 seconds to upload the tools that time. | 18:04 |
| rogpeppe | (running live upgrading test BTW) | 18:05 |
| rogpeppe | niemeyer: friday's last gasp: https://codereview.appspot.com/6490067/ | 18:15 |
| rogpeppe | niemeyer: it passes live tests! | 18:15 |
| rogpeppe | niemeyer: and that should get us roughly back to where we were... | 18:16 |
| rogpeppe | and with that, i'm off to practice drinking port in preparation for Lisbon | 18:16 |
| rogpeppe | fwereade__, mramm, niemeyer: have a great weekend! | 18:16 |
| niemeyer | rogpeppe: Superb! | 18:17 |
| niemeyer | rogpeppe: Thanks so much | 18:18 |
| niemeyer | rogpeppe: and have a brilliant weekend too | 18:18 |
| mramm | rogpeppe: have a fantastic weekend, and get good and practiced up on the port drinking! | 18:27 |
| rogpeppe | mramm: darn, it's running out. need more supplies! | 18:32 |
| mramm | heading to a late lunch, back in a bit | 18:48 |
| niemeyer | PASS: presence_test.go:148: PresenceSuite.TestScale 1.978s | 21:56 |
| niemeyer | 1000 pingers starting, pinging, half being killed, all of them observed.. 2 seconds. Woohay. | 21:56 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!