| wrtp | mornin' al | 07:40 |
|---|---|---|
| wrtp | l | 07:40 |
| wrtp | where *is* al? | 07:40 |
| fwereade_ | wrtp, over the hills and far away ;p | 08:30 |
| fwereade_ | TheMue, were there meant to be some prereqs on your CLs? Just did one, moved on to the next, and saw the same changes... | 08:31 |
| wrtp | fwereade_ is back in Go-land, yay! | 08:31 |
| fwereade_ | wrtp, yeah, and I can almost remember what the hell I'm doing ;) | 08:38 |
| wrtp | fwereade_: bonus | 08:38 |
| TheMue | fwereade_, wrtp: moin | 08:40 |
| wrtp | TheMue: yo! | 08:40 |
| fwereade_ | TheMue, heyhey | 08:40 |
| TheMue | fwereade_: I would like you to not do something regarding those watchers. It's a sequence of branches based on each other. So I have to realize them bottom-up. | 08:41 |
| TheMue | fwereade_: Base is the enhancement of the content watcher (will go in again in a few moments) | 08:42 |
| fwereade_ | TheMue, ah, ok, sorry; I hope the comments on the one I did might be useful anyway | 08:44 |
| TheMue | fwereade_: Sure they will. Which one is it? I've not yet scanned them all. | 08:45 |
| fwereade_ | TheMue, sorry, got called away; it's https://codereview.appspot.com/6059047/ | 08:54 |
| TheMue | fwereade_: So, read it, thx. You also prefer "treaten" insted of treated"? *smile* | 08:59 |
| fwereade_ | TheMue, it's clearly superior ;p | 09:00 |
| fwereade_ | TheMue, but the tyranny of the majority wins | 09:00 |
| TheMue | fwereade_: Hehe, so maybe I should keep it. | 09:00 |
| TheMue | fwereade_: The name "loop" for the goroutines is a result of the general discussion about watches here. In my own code I typically call it backend() | 09:01 |
| TheMue | fwereade_: The idea of mooving all watches into an own file sounds good. I already have a watch_test.go | 09:03 |
| TheMue | fwereade_: unit.go is already quite big. | 09:03 |
| TheMue | fwereade_: What I don't get is the "force upgrade". Could you please explain? | 09:05 |
| niemeyer | Morning all | 09:05 |
| fwereade_ | TheMue, heh, I wasn't closely involved, but: | 09:06 |
| fwereade_ | (heya niemeyer) | 09:06 |
| fwereade_ | TheMue, a requirement that units be upgradeable in a non-started state came up from somewhere | 09:06 |
| fwereade_ | TheMue, you can now pass --force to juju upgrade-charm | 09:06 |
| fwereade_ | TheMue, and the UA interprets that as "upgrade regardless of unit state" | 09:07 |
| TheMue | niemeyer: moin | 09:07 |
| fwereade_ | TheMue, you'd have to take a look at the code to see exactly how it's done | 09:07 |
| fwereade_ | TheMue, lp:934350 | 09:08 |
| TheMue | fwereade_: ok, thx for the hint | 09:08 |
| niemeyer | The point of ugprades in non-started state comes from the fact upgrades are often used to fix the problem | 09:13 |
| TheMue | niemeyer: Good to know. Regarding the state I've yet only tested for the existence of the node for it in ZK. | 09:15 |
| TheMue | niemeyer: Could you take a look at http://paste.ubuntu.com/936632/. I fould a compromise between your and mine approach to keep the watcher test code compact. | 09:16 |
| niemeyer | TheMue: The approach I suggested isn't really mine.. it's a very common way to structure tests | 09:16 |
| niemeyer | TheMue: http://code.google.com/p/go-wiki/wiki/TableDrivenTests | 09:17 |
| niemeyer | TheMue: IMO, this is still cleaner | 09:18 |
| niemeyer | TheMue: I'm not as religious as other people are.. some actually go overboard and attempt to use this scheme even when it's not really adequate for the test at hand, or could be simplified if different | 09:19 |
| niemeyer | TheMue: Your case, though, is a pretty legitimate table | 09:20 |
| TheMue | niemeyer: OK, so I'll adopt them, even if I don't even find them more readable. | 09:20 |
| niemeyer | TheMue: Let me try to explain why it feels more readable then.. | 09:20 |
| niemeyer | TheMue: You actually iterate over a list of possibilities with some logic | 09:20 |
| niemeyer | TheMue: Instead of having the list of possibilities as a first class entity and having the logic iterating over that list, the test is inverted | 09:21 |
| TheMue | niemeyer: Yes, here a table is ok, but later I also have to test for timeouts and closed channels. And the code needed here is due to the structure almost a duplication of the test code above. This is IMHO needless and cluttering. | 09:22 |
| niemeyer | TheMue: It has a function with logic displaced, and then manually iterates over the list of possibilities | 09:22 |
| niemeyer | TheMue: The table may be richer | 09:22 |
| TheMue | niemeyer: Using an anonymous struct, yes. | 09:23 |
| niemeyer | TheMue: If you're iterating over a list of possibilities, it's still easier to read through if it is organized as a sequence of facts than if everything is mixed up with a stream of code | 09:23 |
| niemeyer | TheMue: It's a bit like having a higher level language to describe the test | 09:23 |
| TheMue | niemeyer: How about the call of watcher.Stop() inbetween? | 09:24 |
| niemeyer | TheMue: Again, I'm not religious about it, and in many cases this makes things worse.. this looks like a good chance to use it, though | 09:24 |
| niemeyer | TheMue: Maybe that call should be out of the iteration logic? | 09:24 |
| TheMue | niemeyer: That's what I meant above. There's iteration, then only one err assert, then again a test for timeout. The way I've tried it reads simply top-down (ok, you have to know the three arguments of the assertChange()). | 09:26 |
| niemeyer | TheMue: The fact you have a couple of lines afterwards isn't justification to not do an iteration as an actual iteration | 09:27 |
| niemeyer | TheMue: Note in my suggestion that the code within the loop is also simpler | 09:28 |
| niemeyer | TheMue: and so is the test after it | 09:28 |
| niemeyer | TheMue: The logic in the paste instead shoves a true/false pair that is unchanged over the whole iteration, and makes the loop more complex | 09:29 |
| TheMue | niemeyer: Yes, the iteration for the (just) 4 values is fine, but later I need two receives with timeout. Now a one-liner, then 4 to 5. | 09:29 |
| niemeyer | TheMue: Those are handled in the suggestion I sent you | 09:30 |
| niemeyer | TheMue: With something as trivial as | 09:30 |
| niemeyer | select { | 09:30 |
| niemeyer | case <-time.After(N): | 09:30 |
| niemeyer | t.Fatalf("boom") | 09:30 |
| niemeyer | } | 09:30 |
| niemeyer | TheMue: That's much better than assert(false) | 09:30 |
| TheMue | niemeyer: The new approach uses Fatal to show the timeout better. And still it's more code (and that twice) | 09:31 |
| niemeyer | TheMue: It uses fatal in a remote place that is disconnected from the actual logic because we've forced a loop into a sequence of equivalent function calls | 09:32 |
| TheMue | niemeyer: But let my try the table approach and then we'll see which one looks cleaner. | 09:32 |
| niemeyer | TheMue: Thanks | 09:32 |
| niemeyer | TheMue: As a general comment, more code isn't necessarily bad.. there are other factors that have to be taken into account | 09:33 |
| TheMue | niemeyer: Yes, readbility. And from my perspective the approach using a little helper is more readble. | 09:34 |
| niemeyer | TheMue: Ok.. I disagree, but I respect that | 09:34 |
| niemeyer | TheMue: Perhaps one of the reasons I feel different is that you have a comment there playing the role of code that should be readalbe | 09:35 |
| niemeyer | // Changes() has to be closed. | 09:35 |
| niemeyer | assertChange(false, nil, false) | 09:35 |
| niemeyer | select { | 09:35 |
| niemeyer | case ...: | 09:35 |
| niemeyer | c.Fatalf("Changes channel not closed") | 09:35 |
| niemeyer | } | 09:35 |
| niemeyer | .. | 09:36 |
| niemeyer | You need the comment because assertChange(false, nil, false), by itself, says nothing | 09:36 |
| TheMue | niemeyer: Hehe, yes, got me. In Smalltalk I wrote "self assertChange: … isClosed: true hasTimout: false." | 09:37 |
| TheMue | niemeyer: Here my former lang is more expressive. | 09:37 |
| niemeyer | TheMue: Indeed.. one might do the same with Python. The question is whether that'd be good or not. I the specific case we're debating, I still prefer dropping one layer because the helper function is unnecessary. | 09:39 |
| TheMue | niemeyer: Maybe it's unnecessary, yes. But many unnecessary things are useful. *smile* | 09:40 |
| niemeyer | In other words, isClosed and hasTimeout are both layers that one must look elsewhere to understand. | 09:41 |
| niemeyer | They don't pull their weight. | 09:41 |
| fwereade_ | early lunch, bbs | 09:45 |
| TheMue | niemeyer: Here is the new one: http://paste.ubuntu.com/936664/ | 09:52 |
| niemeyer | TheMue: This comment can go away: | 09:56 |
| niemeyer | // Receive the four changes create, content change, | 09:56 |
| niemeyer | // delete and create again. | 09:56 |
| niemeyer | TheMue: It's explicit now.. | 09:56 |
| niemeyer | TheMue: This one too, for the same reason: | 09:56 |
| niemeyer | // No more changes, expect a timout. | 09:56 |
| niemeyer | TheMue: Same about the last one.. | 09:57 |
| TheMue | niemeyer: So one sentence: Just remove all comments in the method. | 09:59 |
| niemeyer | TheMue: Sorry, I was reading through it, but yes.. this is the case.. no comments that duplicate the code please | 09:59 |
| TheMue | niemeyer: I would at least for the two last selects prefer to explain what they are concentrating on. E.g. why it makes sense to have an empty case <-timeAfter(). A kind of c.Assert(ImHappyBecauseImCalled). | 10:02 |
| niemeyer | TheMue: It makes sense to have an empty time.After because we expect it to happen | 10:02 |
| niemeyer | TheMue: We don't need comments for that | 10:03 |
| TheMue | niemeyer: OK, so et's hope that a new maintainer in several month will get it as easily too. | 10:04 |
| niemeyer | TheMue: If he doesn't get that easily, he shouldn't be working on juju | 10:04 |
| niemeyer | (or she) | 10:04 |
| TheMue | niemeyer: *lol* | 10:04 |
| TheMue | niemeyer: OK, good argument. | 10:04 |
| wrtp | TheMue: one advantage of structuring this as a table is that it makes it easy to have different time outs for the expected-timeout vs not-expected-timeout case. | 10:04 |
| niemeyer | TheMue: As a counterexample, this is the kind of comment that makes sense to have in the test: | 10:04 |
| niemeyer | TheMue: http://paste.ubuntu.com/936679/ | 10:04 |
| wrtp | TheMue: when you don't expect a timeout, it can be much longer because it doesn't interfere with the normal progress of the test | 10:05 |
| niemeyer | TheMue: Comments are not about what the code is doing.. they are about what we can't see | 10:05 |
| wrtp | TheMue: the final timeout should be shorter. | 10:05 |
| niemeyer | wrtp++ | 10:05 |
| TheMue | wrtp: The timeout could be an argument of a helper too. | 10:07 |
| niemeyer | ROTFL | 10:07 |
| niemeyer | I rest my case.. | 10:07 |
| wrtp | lol too | 10:07 |
| TheMue | So at least one success. ;) | 10:12 |
| TheMue | Lunchtime ... | 10:13 |
| niemeyer | TheMue: Enjoy | 10:13 |
| TheMue | niemeyer: so, https://codereview.appspot.com/6059044 is in | 10:53 |
| niemeyer | TheMue: LGTM, thanks! | 10:56 |
| TheMue | TheMue: Fine, thx. Will submit and merge it to go on with next one. It's the ConfigWatcher in state.go. What do you think about Williams idea of moving all watchers of the state package to a file named watchers.go? | 10:58 |
| niemeyer | TheMue: Sounds fine, but please do it in a separate CL, or we'll lose all comments now | 11:03 |
| TheMue | niemeyer: OK, will move the after all are in after an LGTM. | 11:04 |
| TheMue | s/the/them/ | 11:04 |
| mthaddon | niemeyer: got all the changes prepped for moving to config based, and with mongodb auth in place - may be a short service interruption while the changes are applied (will let you know as and when) | 11:08 |
| niemeyer | mthaddon: Sounds great | 11:08 |
| niemeyer | mthaddon: Thanks | 11:08 |
| mthaddon | np | 11:09 |
| mthaddon | niemeyer: ok, config updates being applied now - may be service interruption for a bit, will let you know when done | 11:38 |
| niemeyer | mthaddon: Cheers | 11:41 |
| mthaddon | niemeyer: the appserver processes are running, but not responding on port 8080 to our nagios checks | 11:47 |
| TheMue | niemeyer: here's some code diving before ocean diving -> https://codereview.appspot.com/6056047 | 11:48 |
| mthaddon | niemeyer: ah, I think this is because I've set api-addr: localhost:8080 | 11:48 |
| mthaddon | niemeyer: to listen on all interfaces, what should I set that to? 0.0.0.0:8080 ? | 11:48 |
| niemeyer | mthaddon: Yeah, that'll work | 11:49 |
| mthaddon | k, thx | 11:49 |
| niemeyer | TheMue: done :) | 11:52 |
| TheMue | niemeyer: Hmm, no notification here. Looks good so that I can move on? | 11:54 |
| niemeyer | TheMue: Please check it out | 11:55 |
| niemeyer | TheMue: I forgot to submit somehow | 11:55 |
| niemeyer | TheMue: It LGTM, but there's something to change | 11:55 |
| fwereade_ | niemeyer, I think I need some guidance re response on https://codereview.appspot.com/5756054/ | 11:56 |
| niemeyer | fwereade_: Sounds good.. I'll have some breakfast and we can discuss it | 11:57 |
| niemeyer | fwereade_: Just replied to the thread in the ML again, btw | 11:57 |
| fwereade_ | niemeyer, swet, tyvm | 11:57 |
| niemeyer | mthaddon: All good there? Can I get some breakfast? :) | 11:57 |
| TheMue | niemeyer: OK, thx. Will change that one point and then submit it. | 11:57 |
| mthaddon | niemeyer: yep, all looking good thx | 11:57 |
| mthaddon | niemeyer: go eat! | 11:57 |
| niemeyer | mthaddon: Cheers! Will run tests later | 11:58 |
| wrtp | niemeyer: you need to tag the tomb package (or remote all the existing tags) | 12:23 |
| wrtp | s/remote/remove/ | 12:23 |
| niemeyer | wrtp: Hmm, I believe the tags there are right | 12:30 |
| wrtp | niemeyer: i don't see a go1 tag | 12:30 |
| wrtp | niemeyer: i did go get -u and everything broke | 12:30 |
| niemeyer | wrtp: There isn't one.. that shouldn't be a problem, I believe | 12:30 |
| wrtp | niemeyer: try it | 12:31 |
| wrtp | niemeyer: it went back to release 12, i believe | 12:31 |
| niemeyer | [niemeyer@gopher ~]% GOPATH=$PWD/gopath go get launchpad.net/tomb | 12:32 |
| niemeyer | [niemeyer@gopher ~]% | 12:32 |
| wrtp | get -u | 12:32 |
| niemeyer | [niemeyer@gopher ~]% mkdir gopath | 12:32 |
| wrtp | and then try to build juju/go | 12:32 |
| wrtp | niemeyer: BTW i'm seeing a store test failure: http://paste.ubuntu.com/936825/ | 12:34 |
| niemeyer | Will have to increase the timing then.. there's a race there due to the background coutning | 12:35 |
| niemeyer | counting | 12:35 |
| niemeyer | wrtp: Can you please try removing your local tomb package? | 12:35 |
| niemeyer | wrtp: The tags are right | 12:36 |
| wrtp | niemeyer: that worked. maybe the go pull process didn't update the tags properly. | 12:37 |
| wrtp | gotta go to lunch | 12:37 |
| niemeyer | wrtp: That's what I imagined | 12:38 |
| niemeyer | mthaddon: Oh, ouch.. forgot a detail | 12:39 |
| niemeyer | mthaddon: Can you please update the nagios check with stats=0? | 12:40 |
| niemeyer | mthaddon: Btw, did you really intend to hit the store about twice a second with the nagios check? | 12:49 |
| niemeyer | fwereade_: ping | 12:52 |
| fwereade_ | niemeyer, pong | 12:52 |
| niemeyer | fwereade_: Heya | 12:52 |
| niemeyer | fwereade_: So.. | 12:52 |
| niemeyer | fwereade_: Quick experiment.. | 12:52 |
| niemeyer | fwereade_: Don't look at the code for a moment | 12:52 |
| fwereade_ | niemeyer, ok :) | 12:52 |
| niemeyer | fwereade_: What has to happen for something to actually be logged? | 12:52 |
| niemeyer | fwereade_: In terms of the command setup | 12:53 |
| fwereade_ | niemeyer, someone to call ctx.InitLogging? | 12:53 |
| niemeyer | fwereade_: More.. | 12:53 |
| fwereade_ | niemeyer, the flags to have been set such that InitLogging does something? | 12:54 |
| niemeyer | fwereade_: More.. | 12:54 |
| fwereade_ | niemeyer, someone else to call log.Printf/Debugf? | 12:55 |
| niemeyer | fwereade_: That too. | 12:55 |
| niemeyer | fwereade_: The log commands have to be provided, the command has to call InitLog.. Printf has to be called.. SetsLog has to be true.. | 12:55 |
| fwereade_ | niemeyer, ok, but the breakage occurs at the point where someone calls InitLog | 12:56 |
| niemeyer | fwereade_: In your latest comment, you demonstrate a long chain of recommendations | 12:56 |
| fwereade_ | niemeyer, that's the point at which the host process's logging setup is potentially borked | 12:56 |
| niemeyer | fwereade_: but all of them are actually going towards the same goal.. this is too simple a problem for the maze being put in place | 12:56 |
| niemeyer | fwereade_: I'm personally lost in that maze already.. I don't know why we have InitLog anymore, for example | 12:57 |
| niemeyer | fwereade_: I'm sure there's a good reason.. it's just left my consciousness | 12:57 |
| niemeyer | fwereade_: I'd appreciate if we could simplify what's there, rather than introducing additional flags that put logging behind yet another yes/no flag | 12:58 |
| fwereade_ | niemeyer, it's a little bit lost in the fog of time for me too, but I seem to recall feeling that the code was confirming it was a good idea when the pile of imports in supercommand dropped significantly | 12:59 |
| fwereade_ | niemeyer, I'm all about the simplicity, even if I don't always zero in on it perfectly without help ;) | 13:00 |
| niemeyer | fwereade_: Well, we're all guilty of displeasing the simplicity gods frequently :) | 13:01 |
| fwereade_ | niemeyer, it does seem that logging and subcommand selection are essentially orthogonal problems that are only implemented in the same place because it has hitherto been convenient | 13:02 |
| fwereade_ | niemeyer, and we're now in a situation where the one does not imply the other, and that therefore they should be broken up somehow | 13:02 |
| fwereade_ | niemeyer, a whole new type feels a bit heavyweight; a flag and a bunch of ifs (I agree) feel a bit ugly | 13:03 |
| niemeyer | fwereade_: I think a new type that handles the part of the problem of a supercommand preparing a subcommand to might actually simplify things | 13:04 |
| niemeyer | fwereade_: and make that logic more readalbe | 13:04 |
| fwereade_ | niemeyer, awesome; then I'll try that direction | 13:05 |
| niemeyer | fwereade_: It feels clever, and "saves a type" (?), but at the same time this isn't the first "Wait, what?" moment we reach with that logic | 13:05 |
| fwereade_ | niemeyer, I was worried you might see it as wanton code raviolation | 13:05 |
| niemeyer | fwereade_: It may be time to at least check how that'd look like | 13:05 |
| niemeyer | fwereade_: I suspect it will be simple by itself, and will also make the supercommand implementation more obvious | 13:06 |
| fwereade_ | niemeyer, yeah, I think so too | 13:06 |
| niemeyer | fwereade_: We basically need just a wrapper that handle half of what the supercommand does today, if I understand it well | 13:06 |
| niemeyer | handles | 13:06 |
| mthaddon | niemeyer: that's haproxy hitting it to confirm the service is up or not - is there a different URL we can use for that? | 13:06 |
| fwereade_ | niemeyer, exactly; thanks for the guidance :) | 13:06 |
| niemeyer | mthaddon: Cool.. I'm not too concerned about the traffic by itself | 13:07 |
| * fwereade_ is rather proud of "raviolation" | 13:07 | |
| niemeyer | mthaddon: At least not yet.. it's good that something is exercising a bit of load there | 13:07 |
| mthaddon | niemeyer: so in terms of updating the nagios check - you want stats=0 used how? | 13:07 |
| niemeyer | mthaddon: I just find unfortunate that we don't *actually* had 8000+ people interested in Jenkins in the last hour! ;-) | 13:07 |
| mthaddon | heh | 13:08 |
| niemeyer | mthaddon: This is an http query parameter | 13:08 |
| mthaddon | niemeyer: and still for the /charm/oneiric/jenkins URL? | 13:08 |
| niemeyer | mthaddon: stats=0 will disable the stats collection | 13:08 |
| niemeyer | mthaddon: Yeah | 13:09 |
| mthaddon | niemeyer: and we should update haproxy to use the same? | 13:09 |
| niemeyer | mthaddon: Yeah, please | 13:09 |
| niemeyer | 8.3k and counting, literally | 13:09 |
| mthaddon | k, thx | 13:09 |
| niemeyer | mthaddon: Then, let's clean up stats to avoid the bogus spike please | 13:10 |
| mthaddon | ok, I'll let you know when that's done and you can talk me through cleaning up the stats | 13:13 |
| niemeyer | TheMue: Any other branches for review? | 13:20 |
| TheMue | niemeyer: I'm doing the one with the NeedsUpgradeWatcher. | 13:21 |
| TheMue | niemeyer: You mentioned to refactor the test for the ConfigWatcher too. Here I have to think a bit. The ConfigNode has no public fields, only access methods. | 13:22 |
| TheMue | niemeyer: So I'm thinking how I best can test it via tables. | 13:23 |
| niemeyer | TheMue: Thanks, no worries.. just wanted to make sure I wasn't missing something | 13:25 |
| TheMue | niemeyer: Not yet, I'll notify you. | 13:25 |
| wrtp | TheMue: i just got a test failure: http://paste.ubuntu.com/936882/ | 13:26 |
| TheMue | wrtp: Oh, strange, here it has been ok (from inside the editor and from commandline just before submit). | 13:27 |
| wrtp | TheMue: i'll try pulling and testing again | 13:28 |
| wrtp | TheMue: i tried again (without pulling) and it succeeded. | 13:29 |
| TheMue | wrtp: Uuuh, even more strange. | 13:29 |
| wrtp | TheMue: it looks to me like that code is racy | 13:30 |
| wrtp | oh, maybe not | 13:30 |
| TheMue | wrtp: In method I use no goroutine | 13:30 |
| wrtp | i wonder what value got received. | 13:30 |
| wrtp | TheMue: WatchConfig starts a goroutine, no? | 13:30 |
| TheMue | wrtp: Yes. I meant inside the test to start changing values later. | 13:31 |
| wrtp | TheMue: i was thinking that we don't know exactly when the channel is closed. but it doesn't (shouldn't?) matter in fact. | 13:31 |
| wrtp | TheMue: slightly concerning, but it seems to pass consistently now. | 13:33 |
| TheMue | wrtp: Needs to get warm. *smile* | 13:33 |
| TheMue | wrtp: I'm currently in that test file and refactor the tests. So please wait a moment if possible, than I can check it too. | 13:34 |
| TheMue | wrtp: For tests you could also take and print the received change. There shouldn't been any one after the error. So I think the value will be nil, but the channel is still open. | 13:36 |
| wrtp | TheMue: yes, the test could print the received value | 13:36 |
| TheMue | wrtp: The 'illegal content error' to be more special. | 13:36 |
| TheMue | wrtp: Otherwise with the race condition it's hard to say "Hey, we know the channel will close soon, but not exactly when.". *sigh* | 13:40 |
| wrtp | TheMue: i don't think there's a race condition | 13:40 |
| wrtp | TheMue: if a value is received, there's an error | 13:40 |
| wrtp | TheMue: (because the watcher has sent a value when it should've) | 13:41 |
| TheMue | wrtp: I've got an idea, but I'm not yet sure. I'll take a look during test refactoring. | 13:44 |
| niemeyer | mthaddon: Just run a quick test with the store, btw.. | 13:59 |
| niemeyer | mthaddon: Can you please mail me logs when you have a moment? | 13:59 |
| mthaddon | niemeyer: which logs are you interested in? there's very little in charmd.log | 14:00 |
| niemeyer | mthaddon: That was the one.. if it has little, that's good | 14:01 |
| mthaddon | niemeyer: nothing in there since March | 14:02 |
| niemeyer | mthaddon: Super | 14:02 |
| niemeyer | mthaddon: Hmm.. can you do me a quick favor? | 14:04 |
| niemeyer | mthaddon: How many processors do we have in the frontend servers? | 14:05 |
| mthaddon | niemeyer: each one is 8 cores | 14:05 |
| niemeyer | mthaddon: What else run in them? | 14:05 |
| mthaddon | niemeyer: they both run the full stack, apache/haproxy/appserver/mongodb | 14:05 |
| niemeyer | mthaddon: Super | 14:06 |
| niemeyer | mthaddon: Can you please export an env var | 14:06 |
| niemeyer | mthaddon: For charmd, specifically | 14:06 |
| niemeyer | mthaddon: export GOMAXPROCS=4 | 14:06 |
| mthaddon | niemeyer: like this? https://pastebin.canonical.com/64567/ | 14:09 |
| niemeyer | mthaddon: Yeah, thanks | 14:10 |
| niemeyer | mthaddon: I mean, I think.. | 14:10 |
| niemeyer | mthaddon: Not versed in the syntax there | 14:11 |
| mthaddon | sure, I'll test it first, just checking that looks right in the upstarts script really | 14:11 |
| niemeyer | mthaddon: Yeah | 14:11 |
| niemeyer | mthaddon: This will allow the runtime to spread the goroutine across more cores | 14:12 |
| niemeyer | goroutines | 14:12 |
| mthaddon | k | 14:13 |
| niemeyer | mthaddon: Any luck there? Just want to run another test before we reset the data | 14:24 |
| mthaddon | niemeyer: just filtering through our puppet infrastructure atm - will let you know when it's been applied (currently pending a review from another member of IS) | 14:25 |
| niemeyer | mthaddon: Wow, cool | 14:25 |
| mthaddon | but it "dry-ran" clean, so shouldn't be too long | 14:26 |
| niemeyer | mthaddon: I'm heading out for lunch.. I'd appreciate if we could deploy those changes and then do the cleanup today still so that we can start collecting stats for real | 14:55 |
| mthaddon | niemeyer: sure | 14:56 |
| TheMue | niemeyer: https://codereview.appspot.com/6050053/ is in. | 15:26 |
| wrtp | just gonna reboot. | 15:38 |
| mthaddon | niemeyer: changes all applied - let me know what needs to be done to clean up the stats | 15:55 |
| rog | niemeyer: you said "the ssh identity is configurable" but it's not configurable from environments.yaml. should it be? | 16:03 |
| fwereade_ | gn all, might be back a bit later | 16:09 |
| niemeyer | mthaddon: Can start with: use juju; db.stat.counters.count() | 16:20 |
| niemeyer | fwereade_: Night! | 16:20 |
| mthaddon | niemeyer: 288 | 16:23 |
| niemeyer | mthaddon: Nice | 16:23 |
| niemeyer | Well below the 40k+ actual counts made | 16:23 |
| niemeyer | mthaddon: So we just have to clean that up | 16:24 |
| niemeyer | db.stat.counters.remove() | 16:24 |
| mthaddon | ok, done | 16:24 |
| niemeyer | mthaddon: Thanks! | 16:24 |
| mthaddon | that's it? | 16:24 |
| niemeyer | mthaddon: We're good then | 16:24 |
| niemeyer | mthaddon: Yeah | 16:24 |
| mthaddon | sweet, that was easy | 16:24 |
| niemeyer | mthaddon: Wait | 16:24 |
| niemeyer | mthaddon: Almost.. there's something wrong.. | 16:25 |
| niemeyer | mthaddon: Something is still poking at the URL without the stats=0 setting | 16:25 |
| mthaddon | hmm, I wonder if haproxy strips the query string... | 16:25 |
| niemeyer | mthaddon: It souldn't, at least | 16:26 |
| niemeyer | mthaddon: 200+ since reset already | 16:26 |
| mthaddon | must be haproxy then - I'll take a look - may need to quote the URL or something | 16:26 |
| niemeyer | mthaddon: Is the 0 after = a zero? | 16:28 |
| niemeyer | mthaddon: I know it's obvious, sorry, but just to be sure | 16:28 |
| mthaddon | yeah, is definitely a zero | 16:28 |
| TheMue | so, off for today, bank appointment. have a nice evening. | 16:30 |
| mthaddon | niemeyer: how are you checking the stats, just so I can confirm when I try a few possible fixes? | 16:30 |
| niemeyer | TheMue: Cheers | 16:30 |
| niemeyer | mthaddon: Sent | 16:30 |
| mthaddon | thx | 16:30 |
| niemeyer | mthaddon: Meanwhile, I'm checking to see if there's a bug in the code.. I had tests, but maybe the tests are broken | 16:31 |
| TheMue | niemeyer: you've seen, next proposal is in and tomorrow I'll continue. | 16:31 |
| mthaddon | k | 16:31 |
| niemeyer | TheMue: Sounds great, thanks | 16:31 |
| TheMue | niemeyer: np, and have fun at diving | 16:32 |
| niemeyer | TheMue: Cheers man.. will try to provide you some good feedback so you can move on tomorrow | 16:33 |
| TheMue | niemeyer: thx, bye | 16:33 |
| mthaddon | hmm, according to http://code.google.com/p/haproxy-docs/wiki/httpchk, query strings are permitted, and they don't say anything about needing to quote them | 16:33 |
| niemeyer | mthaddon: I believe there's a bug in the code.. the test is probably not good enough | 16:35 |
| mthaddon | ah, okay | 16:35 |
| niemeyer | mthaddon: Will fix it and ping you | 16:35 |
| niemeyer | mthaddon: (even if you're not around) | 16:35 |
| mthaddon | niemeyer: cool cool - will pick up the ping in the morning if it's past my EOD and can do a rollout whenever | 16:36 |
| niemeyer | mthaddon: Sounds great.. please repeat the remove after the deployment | 16:36 |
| mthaddon | yep yep | 16:37 |
| niemeyer | mthaddon: So we can get meaningful numbers from then on already | 16:37 |
| * mthaddon nods | 16:37 | |
| niemeyer | mthaddon: Thanks for your help today | 16:37 |
| mthaddon | np | 16:37 |
| niemeyer | Hah.. found the test bug.. the URL being pinged is wrong, which means we don't need stats=0 to not have counters.. :( | 16:38 |
| * niemeyer fixes it | 16:39 | |
| rog | [LOG] 38.27388 JUJU state: ssh error (will retry: true): ssh: Welcome to Ubuntu 11.10 (GNU/Linux 3.0.0-17-virtual i686) | 17:14 |
| rog | ha ha | 17:14 |
| niemeyer | rog: :-) | 17:15 |
| rog | niemeyer: i can't see how the python version stops the ssh session from going into interactive mode | 17:17 |
| niemeyer | rog: That's what one of the flags do | 17:18 |
| rog | niemeyer: i'm using all the same flags, i think | 17:18 |
| rog | niemeyer: for the record: "ssh" "-T" "-o" "StrictHostKeyChecking no" "-o" "PasswordAuthentication no" "-L" "localhost:37722:localhost:2181" "-p" "22" "ubuntu@ec2-174-129-123-190.compute-1.amazonaws.com" | 17:20 |
| niemeyer | rog: -N | 17:20 |
| rog | niemeyer: ah, thanks. the python version doesn't seem to use that flag though. strange. | 17:21 |
| niemeyer | rog: Maybe it's doing the unthinkable.. :-) | 17:22 |
| rog | niemeyer: maybe it's ignoring stdout. | 17:22 |
| niemeyer | rog: Right, maybe it *is* sitting idle with a shell.. which would be quite funny | 17:23 |
| rog | niemeyer: that would probably explaining it. | 17:23 |
| rog | niemeyer: yes, i think that's what must be happening. lol. | 17:23 |
| rog | niemeyer: funny, adding that option triggered a "Permissions 0664 for '/home/rog/src/go/src/launchpad.net/juju/go/state/sshtest/id_rsa' are too open." error. ssh *is* baroque. | 17:27 |
| niemeyer | rog: Woah | 17:28 |
| niemeyer | rog: Vhat geev | 17:28 |
| * rog hopes that bzr respects permissions. | 17:29 | |
| niemeyer | rog: Uh oh.. it doesn't | 17:29 |
| niemeyer | rog: +x only | 17:29 |
| rog | guess i'll add a chmod then | 17:29 |
| niemeyer | rog: Yeah, the charm tests do something like that already | 17:29 |
| niemeyer | rog: on init(), IIRC | 17:29 |
| niemeyer | rog: Would you mind to have a quick look at this: https://codereview.appspot.com/6072046 | 17:31 |
| rog | sure. | 17:32 |
| rog | niemeyer: BTW all tests just passed, using the ssh connect. | 17:32 |
| niemeyer | rog: Woohay | 17:32 |
| rog | which is highly satisfactory! | 17:32 |
| niemeyer | rog: I can imagine | 17:32 |
| niemeyer | I'm excited myself | 17:32 |
| niemeyer | This is a critical roadblock | 17:33 |
| niemeyer | rog: Btw, I'll reduce the 5 back to 2.. that wasn't the issue | 17:33 |
| rog | niemeyer: ECONTEXT | 17:34 |
| niemeyer | rog: The CL | 17:34 |
| rog | oh yeah, i see | 17:34 |
| niemeyer | rog: I've changed the timing there to attempt to cause an error, but I'll put that back | 17:34 |
| rog | :3 lol | 17:34 |
| niemeyer | rog: Seriously.. | 17:34 |
| rog | sorry i missed that! | 17:35 |
| niemeyer | rog: The best compilers wouldn't catch that.. | 17:35 |
| niemeyer | rog: Sorry that I wrote it! | 17:35 |
| niemeyer | :-) | 17:35 |
| rog | that's why we have reviewers. i failed! | 17:35 |
| niemeyer | rog: What's funny is that this is *exactly* a case where there was a non-obvious bug on the other side | 17:36 |
| rog | niemeyer: i think the basic problem was that the test was testing for the *absence* of something. | 17:36 |
| niemeyer | A pathetic test when we most need it | 17:36 |
| rog | niemeyer: (which it still does AFAICS) | 17:36 |
| niemeyer | rog: True | 17:36 |
| niemeyer | rog: yeah, but it works now | 17:36 |
| rog | lol | 17:36 |
| rog | until something changes | 17:36 |
| niemeyer | rog: Well.. there's nothing we can do about *that* :-) | 17:37 |
| rog | niemeyer: the test should really try an action that does something, test that the something succeeded, *and* check that no stats were taken | 17:38 |
| niemeyer | rog: That's exactly what it does right now | 17:38 |
| niemeyer | rog: Well, it's not testing that it succeeded | 17:38 |
| niemeyer | rog: That's a good idea | 17:38 |
| rog | niemeyer: yeah | 17:38 |
| rog | niemeyer: how come the GET of /charms/xxx was succeeding, BTW? | 17:39 |
| niemeyer | rog: It wasn't.. | 17:39 |
| rog | oh yeah, that was just the NewRequest error check | 17:40 |
| rog | so... that's my feedback | 17:40 |
| niemeyer | rog: Cheers.. already on the way | 17:40 |
| niemeyer | rog: Updated | 17:42 |
| rog | niemeyer: LGTM | 17:45 |
| niemeyer | rog: Cheers | 17:45 |
| niemeyer | rog: Was a good hint, thanks | 17:45 |
| rog | niemeyer: pleasure | 17:45 |
| niemeyer | mthaddon: Code is good to go on 122.. | 17:53 |
| niemeyer | Taking a short break | 17:53 |
| === rog is now known as Guest27265 | ||
| === Guest27265 is now known as rogpeppe | ||
| rogpeppe | niemeyer: https://codereview.appspot.com/6074044/ | 18:00 |
| niemeyer | rogpeppe: Cool, I'm sitting down on them now | 18:52 |
| rogpeppe | niemeyer: lovely, thanks | 18:52 |
| niemeyer | rogpeppe: re. the "just in case of what", that was to close the channel | 18:53 |
| niemeyer | rogpeppe: errorc | 18:54 |
| rogpeppe | niemeyer: why would closing it be a good thing? | 18:57 |
| niemeyer | rogpeppe: You're right, looking at how it's used there isn't a good reason indeed | 18:59 |
| niemeyer | rogpeppe: Should -N be added to the list of args? | 18:59 |
| rogpeppe | it is... i thought! | 19:00 |
| rogpeppe | oh, of course, that's in the next CL | 19:00 |
| rogpeppe | can we leave it until the next CL? | 19:00 |
| rogpeppe | it doesn't affect the tests | 19:00 |
| rogpeppe | niemeyer: ^ | 19:00 |
| niemeyer | rogpeppe: Yeah, that's fine | 19:01 |
| niemeyer | rogpeppe: Going through the testing right now, btw | 19:20 |
| rogpeppe | niemeyer: cool. | 19:20 |
| niemeyer | rogpeppe: Btw, looking at the issue people are having with the stock ssh package, our decision seems to be paying off | 19:32 |
| rogpeppe | niemeyer: yeah. | 19:32 |
| rogpeppe | niemeyer: plus, i hope we can move away from ssh entirely before long | 19:32 |
| niemeyer | rogpeppe: Indeed | 19:33 |
| rogpeppe | niemeyer: i'll be very happy when i delete this code :-) | 19:33 |
| niemeyer | rogpeppe: Me too :-) | 19:33 |
| niemeyer | rogpeppe: At bed last night I had that crazy thought that replacing zookeeper with mongodb might not be so hard and may make certain things easier on the transition to an HTTPS API | 19:35 |
| rogpeppe | niemeyer: does mongodb do non-polling notifications? | 19:35 |
| niemeyer | rogpeppe: Yeah, there's a little used trick for that, with capped collections | 19:35 |
| niemeyer | rogpeppe: One can tail a capped collection, basically | 19:36 |
| rogpeppe | niemeyer: have you got a reference? | 19:36 |
| rogpeppe | i'm not at all familiar with mongodb | 19:36 |
| niemeyer | rogpeppe: http://godoc.labix.org/mgo#Query.Tail | 19:36 |
| niemeyer | I hope that exists.. /me clicks :) | 19:36 |
| niemeyer | Yeah, it does | 19:36 |
| rogpeppe | hmm, sounds a bit like a hack to me | 19:37 |
| niemeyer | rogpeppe: Wow, that was quick :) | 19:37 |
| rogpeppe | niemeyer: seems like we'd be building functionality on top of a feature that really wasn't designed to be used that way | 19:37 |
| niemeyer | rogpeppe: I don't know what you mean.. tail was used exactly for that use case | 19:38 |
| niemeyer | s/used/created/ | 19:38 |
| rogpeppe | "Capped collections are not shard-able". is that a problem? | 19:38 |
| rogpeppe | niemeyer: perhaps it doesn't mean what i think it means | 19:39 |
| niemeyer | rogpeppe: No.. we'd definitely not use shards | 19:39 |
| niemeyer | rogpeppe: shard != replica set | 19:39 |
| rogpeppe | niemeyer: ah | 19:39 |
| rogpeppe | niemeyer: so you could have capped collection with a size of 1 and it would be roughly equivalent to a zk watch? | 19:40 |
| niemeyer | rogpeppe: We'd probably use something like 4 or 8k collections, and watch on specific patterns | 19:40 |
| niemeyer | rogpeppe: I *think* I can recreate exactly the same API we have in the zookeeper package on top of mongodb with some ease | 19:41 |
| rogpeppe | niemeyer: but... what would we gain? | 19:41 |
| niemeyer | rogpeppe: Quite a few things | 19:41 |
| niemeyer | - Non-RAM storage.. means we can drop the use of S3 for several things | 19:41 |
| rogpeppe | niemeyer: that would indeed be nice. | 19:42 |
| niemeyer | - SSL and authentication | 19:42 |
| niemeyer | - Automatic join and departure of cluster members (zk doesn't do that!) | 19:42 |
| rogpeppe | niemeyer: that would be irrelevant if we moved to an HTTPS API | 19:42 |
| niemeyer | rogpeppe: Not entirely.. we can move to HTTPS step by step.. | 19:43 |
| niemeyer | rogpeppe: We could switch over to mongodb first, without an HTTPS api, just by reimplemneting the zk package | 19:43 |
| niemeyer | rogpeppe: This is an easy switch over.. no changes to the state package | 19:43 |
| rogpeppe | niemeyer: so you think you can create a "mongo-zk" package that looks almost exactly like the zk package? | 19:44 |
| niemeyer | rogpeppe: Right.. at least almost-exactly-enough to our purposes | 19:44 |
| rogpeppe | niemeyer: that would be quite a cool thing | 19:44 |
| niemeyer | rogpeppe: So very low risk | 19:44 |
| niemeyer | Another one: | 19:45 |
| niemeyer | - Look ma', no Java! | 19:45 |
| niemeyer | :) | 19:45 |
| rogpeppe | niemeyer: now yer talkin'! | 19:45 |
| rogpeppe | niemeyer: is your mongodb client pure go? | 19:45 |
| niemeyer | rogpeppe: Yeah | 19:45 |
| rogpeppe | niemeyer: another benefit then: no cgo! | 19:45 |
| niemeyer | Yep! | 19:46 |
| niemeyer | Although that hasn't been much of an issue in practice | 19:46 |
| niemeyer | (Java is) | 19:46 |
| rogpeppe | niemeyer: so... you'd use a collection for each zk node? | 19:46 |
| niemeyer | rogpeppe: No.. a single collection to the whole FS structure, indexed on path | 19:47 |
| rogpeppe | niemeyer: ah, the mgo query defines the watch | 19:47 |
| niemeyer | rogpeppe: Right! | 19:47 |
| niemeyer | rogpeppe: and the watched collection would be a separate one | 19:48 |
| niemeyer | rogpeppe: fs.data and fs.events for example | 19:48 |
| rogpeppe | niemeyer: right | 19:49 |
| rogpeppe | niemeyer: how do you choose a good cap size? | 19:49 |
| rogpeppe | niemeyer: for fs.events, that is | 19:49 |
| niemeyer | rogpeppe: In practice, given what watches means for us today, pretty much anything non-absurdly small would do | 19:50 |
| niemeyer | rogpeppe: They must be large enough for the application to "breath" and still not miss events | 19:50 |
| rogpeppe | niemeyer: yeah, because historical events don't matter of course. | 19:52 |
| niemeyer | rogpeppe: We could pick something like 1M for example, and be pretty sure to never worry except perhaps on absurdly large cases | 19:52 |
| niemeyer | rogpeppe: Right | 19:52 |
| niemeyer | rogpeppe: Funny enough, we might actually avoid the polling we're introducing in the presence package with this mechanism | 19:57 |
| niemeyer | rogpeppe: Since the watching is less ephemeral than with zk proper | 19:57 |
| rogpeppe | niemeyer: i don't *think* so | 19:58 |
| rogpeppe | niemeyer: because we want to know when a client goes away | 19:58 |
| rogpeppe | niemeyer: so we *want* some kind of ephemerality | 19:58 |
| rogpeppe | niemeyer: which is what the presence package gives us | 19:58 |
| niemeyer | rogpeppe: Yeah, indeed.. we still need to update the note | 19:59 |
| niemeyer | nod | 19:59 |
| niemeyer | node | 19:59 |
| rogpeppe | niemeyer: but at least we *have* moved away from dependence on ephemeral nodes | 19:59 |
| rogpeppe | niemeyer: which is something you couldn't easily emulate, i think. | 19:59 |
| rogpeppe | niemeyer: so your event collection is just a set of (path, operation) tuples, right? | 20:00 |
| rogpeppe | niemeyer: and Tail is guaranteed to return events in order? | 20:00 |
| niemeyer | rogpeppe: Yeah | 20:01 |
| niemeyer | (to both) | 20:01 |
| niemeyer | rogpeppe: capped collections + tail was implemented to support the operation log of MongoDB itself | 20:02 |
| niemeyer | rogpeppe: Replication is based on that | 20:02 |
| niemeyer | rogpeppe: So we can be pretty comfortable that they'll pay attention to the feature working well | 20:02 |
| rogpeppe | niemeyer: of course, another plus point - we can use mongodb to implement juju logging | 20:02 |
| niemeyer | rogpeppe: True | 20:03 |
| niemeyer | rogpeppe: Not only juju, actually.. we might stream log for all the machines | 20:03 |
| rogpeppe | niemeyer: definitely. i've been wanting to do that. | 20:03 |
| niemeyer | Although, we should be careful with that | 20:04 |
| rogpeppe | niemeyer: indeed - we could swamp the network | 20:04 |
| rogpeppe | niemeyer: but for debugging purposes, i think it's crucial | 20:04 |
| niemeyer | rogpeppe: I wouldn't say the network, but the mongodb master for sure | 20:04 |
| rogpeppe | niemeyer: ah | 20:04 |
| rogpeppe | i'd be sorely tempted to streamline the "zk" API when redoing it | 20:06 |
| niemeyer | rogpeppe: Well, that's trivial to do after we're done migrating | 20:08 |
| niemeyer | rogpeppe: I mean, I'd be happy to have it improved too | 20:08 |
| niemeyer | rogpeppe: But would try to mimic it precisely first | 20:09 |
| niemeyer | rogpeppe: So we can play with the idea in a risk less fashion until we're certain we want to flip | 20:09 |
| niemeyer | rogpeppe: After flipping, we have no great attachment to it | 20:09 |
| niemeyer | rogpeppe: Review delivered! | 20:09 |
| rogpeppe | niemeyer: yeah, you're right | 20:10 |
| rogpeppe | niemeyer: brilliant, thanks! | 20:10 |
| rogpeppe | niemeyer: i expected more nasties there. thanks a lot. will fix tests in the morning. time to go now. | 20:14 |
| rogpeppe | niemeyer: i expect to see a working version of mongo-zk by then :-) | 20:14 |
| niemeyer | rogpeppe: Haha :) | 20:15 |
| niemeyer | rogpeppe: Given the problem, I quite like the approach | 20:16 |
| niemeyer | rogpeppe: I'd prefer to not have any of it, as you would | 20:17 |
| niemeyer | rogpeppe: Let's just try to make that stuff solid meanwhile, and as clean as possible | 20:17 |
| niemeyer | rogpeppe: In that sense, your branch feels like a win. Thanks. | 20:17 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!