| === Guest__ is now known as smaddock | ||
| rogpeppe | fwereade: morning guv'nor | 06:55 |
|---|---|---|
| fwereade | heya rogpeppe | 07:15 |
| rogpeppe | fwereade: looks like my upgrader proposal was premature. still, it all works; i can drag it out later :-) | 07:16 |
| fwereade | rogpeppe, yeah, I think it's a viable long-term direction but maybe not time for it yet | 07:25 |
| fwereade | popping out for a mo, bbs | 09:42 |
| Aram | morning | 10:19 |
| rogpeppe | Aram: hiya | 10:22 |
| fwereade | Aram, heyhey | 10:23 |
| TheMue | Aram: Moin. | 10:28 |
| fwereade | rogpeppe, TheMue: I would very much appreciate your thoughts on https://codereview.appspot.com/6304068 | 11:27 |
| fwereade | rogpeppe, TheMue: everything important is in the description, the code should be a really quick skim/verify if you agree with the problem statement | 11:27 |
| * rogpeppe looks | 11:28 | |
| rogpeppe | fwereade: in exchange, a few minor cleanups in the same file: https://codereview.appspot.com/6295069/ | 11:32 |
| fwereade | rogpeppe, LGTM | 11:33 |
| fwereade | rogpeppe, I'd call that a trivial, just merge it | 11:36 |
| rogpeppe | fwereade: that would certainly make our lives easier w.r.t. your branch | 11:37 |
| fwereade | rogpeppe, I think it'd be justified even if that weren't the case ;) | 11:37 |
| fwereade | rogpeppe, does the logic seem sound to you? | 11:37 |
| fwereade | rogpeppe, it's really just about making sure everything happens on the loop goroutine | 11:38 |
| rogpeppe | fwereade: i think so. am still absorbing. | 11:38 |
| rogpeppe | fwereade: small thing. i wonder if instead of having those closures all down the code, something like this might read a little better: http://paste.ubuntu.com/1037042/ | 11:42 |
| fwereade | rogpeppe, I don't think we want to defer a Stop, which is what that effectively seems to do | 11:43 |
| fwereade | rogpeppe, we want to be sure that that code is called OAOO | 11:44 |
| rogpeppe | fwereade: OAOO? | 11:44 |
| fwereade | rogpeppe, Once And Only Once | 11:44 |
| rogpeppe | fwereade: i think that code is exactly equivalent to yours, no? | 11:44 |
| fwereade | rogpeppe, can you paste an example implementation of the Stop method? | 11:45 |
| rogpeppe | fwereade: it's already implemented by all the sub-watchers | 11:45 |
| fwereade | rogpeppe, ok, then our code is definitely not doing the same thing | 11:46 |
| rogpeppe | fwereade: really? | 11:46 |
| fwereade | rogpeppe, yours will wait until the tomb is dead, on the main goroutine, before the main goroutine calls tomb.Done() | 11:46 |
| fwereade | rogpeppe, assuming the implementations stay as they are | 11:47 |
| fwereade | rogpeppe, my approach ensures the cleanup code is called OAOO after the main loop has exited | 11:48 |
| rogpeppe | fwereade: i must be missing something. AFAICS they're trivially identical. | 11:48 |
| fwereade | rogpeppe, the problem is in allowing any cleanup code to run on an arbitrary goroutine -- the only stuff that's safe is Kill() and Wait() | 11:48 |
| fwereade | rogpeppe, all the Stop implementations return tomb.Wait() | 11:48 |
| rogpeppe | fwereade: your deferred closure does exactly the same thing as my three deferred function calls, no? | 11:48 |
| fwereade | rogpeppe, if they don;t do that then clients won;t be able to extract an error (unless we add a Wait method to each watcher) | 11:49 |
| fwereade | rogpeppe, but if they do use tomb.Wait(), then it's certainly not OK to call them on the loop goroutine, because that's the goroutine that is meant to signal that cleanup is complete in the first place | 11:50 |
| rogpeppe | fwereade: i don't understand. i think both piece of code are trivially (potentially even by the compiler) transformable into each other. | 11:51 |
| fwereade | rogpeppe, stop me when I say something incorrect: | 11:51 |
| fwereade | rogpeppe, the Stop methods do the following: tomb.Kill(nil), stop the watcher, and wait on tomb completion before returning | 11:52 |
| rogpeppe | fwereade: here are the steps of the transformation: http://paste.ubuntu.com/1037058/ | 11:53 |
| rogpeppe | fwereade: your code is calling Stop too | 11:53 |
| fwereade | rogpeppe, oh really? | 11:53 |
| rogpeppe | fwereade: e.g. line 51 | 11:53 |
| fwereade | rogpeppe, oh balls I totally misread it | 11:54 |
| rogpeppe | fwereade: np | 11:54 |
| fwereade | rogpeppe, missed a .watcher | 11:54 |
| rogpeppe | fwereade: thought so | 11:54 |
| rogpeppe | maybe stopWatcher could be stopSubwatcher | 11:55 |
| fwereade | rogpeppe, nah, I think it'd be clear anyway | 11:56 |
| fwereade | rogpeppe, still trying to figure out whether it feels like a clear improvement... I'll implement it and see how I feel ;p | 11:57 |
| rogpeppe | fwereade: yeah, i'm not sure either. | 11:57 |
| fwereade | rogpeppe, my crystal ball tells me that I will use it again soon, so on that basis I think it's worth it | 11:58 |
| rogpeppe | fwereade: it was just a reaction to seeing the same closure in all of them | 11:58 |
| rogpeppe | fwereade: i'm already about to use it | 11:58 |
| rogpeppe | fwereade: possible comment on stopWatcher: | 11:58 |
| rogpeppe | // stopWatcher stops a watcher and propagates | 11:58 |
| rogpeppe | // the error to the given tomb if nessary. | 11:58 |
| rogpeppe | f | 11:58 |
| fwereade | rogpeppe, perfect | 11:59 |
| * rogpeppe likes ad hoc interfaces | 11:59 | |
| fwereade | rogpeppe, how should i integrate that if you're implementing it right now? | 11:59 |
| rogpeppe | fwereade: i'm not implementing that. i'm implementing MachineUnitsWatcher | 11:59 |
| rogpeppe | fwereade: and i'll merge it with your changes before submitting it | 12:00 |
| fwereade | rogpeppe, excellent | 12:00 |
| fwereade | rogpeppe, cheers | 12:00 |
| rogpeppe | fwereade: np. thanks for cleaning this up. | 12:00 |
| fwereade | rogpeppe, a pleasure :) | 12:00 |
| TheMue | Just jumped in from lunch. Together with the watchStopper/stopWatcher() both times LGTM, looks good. | 12:06 |
| fwereade | TheMue, thnks | 12:10 |
| rogpeppe | fwereade: submitted. i feel a little bit naughty. | 12:12 |
| fwereade | rogpeppe, I think that one is absolutely legitimate :) | 12:13 |
| TheMue | rogpeppe: *lol* | 12:17 |
| hazmat | rogpeppe, what's the support like for bzr revisions of tools? | 12:27 |
| hazmat | rogpeppe, does that lead major.minor.patch-revno form? | 12:27 |
| rogpeppe | hazmat: the plan is to use major.minor.patch | 12:27 |
| rogpeppe | hazmat: but you get a local repo that you can push stuff to | 12:28 |
| rogpeppe | hazmat: so you build the binaries locally and push them to the storage for a given juju environment. then it'll use them. | 12:29 |
| rogpeppe | hazmat: does that answer your question? | 12:30 |
| hazmat | rogpeppe, indeed it does.. but it seems a bit manual for dev cycling on an env, it sounds like its effectively using the releases url as a namespace for priv dev via local release repo | 12:31 |
| hazmat | but that's pretty minor given the simplicity | 12:31 |
| rogpeppe | hazmat: there's an flag to bootstrap which causes it to automatically build and upload the binaries before bootstrapping | 12:32 |
| hazmat | rogpeppe, upload the binaries to the env or to the release url? | 12:32 |
| hazmat | s/env/provider storage | 12:32 |
| rogpeppe | hazmat: to the env. there's no fixed release url - it's environment-configurable. | 12:33 |
| hazmat | rogpeppe, sure its configurable, but there not comparable without a release url per se | 12:33 |
| hazmat | s/their | 12:33 |
| hazmat | rogpeppe, so without that flag it would default to using the release url ? | 12:34 |
| rogpeppe | they're? :-) | 12:34 |
| rogpeppe | hazmat: currently there's no default. there's a "public-bucket" field in the ec2 config which would to the s3 bucket containing the public release | 12:35 |
| rogpeppe | s/would to/would point to/ | 12:35 |
| rogpeppe | hazmat: i'd guess that when we provide one, we'll change the default to point to it | 12:37 |
| hazmat | rogpeppe, how are you determining the version when you upload tools on bootstrap? | 12:37 |
| rogpeppe | hazmat: it's taken from Current in the version package. | 12:38 |
| rogpeppe | hazmat: but the version is actually irrelevant for bootstrap because the logic will always choose a version from the private storage by preference, and the private storage starts empty. | 12:39 |
| hazmat | rogpeppe, ic, won't the priv storage will start populated though with the tools uploaded by bootstrap, ie has the one to be used | 12:41 |
| rogpeppe | hazmat: destroy-environment deletes everything from the private storage, including uploaded tools. | 12:42 |
| rogpeppe | hazmat: (if i've parsed your sentence correctly) | 12:43 |
| hazmat | rogpeppe, sure.. i was referencing bootstrap --tools would upload the current toolset to provider storage before the machine came up, so it would be using the only extant version there | 12:43 |
| rogpeppe | hazmat: that's right. | 12:44 |
| rogpeppe | hazmat: i think the flag is "--upload-tools" | 12:45 |
| rogpeppe | hazmat: "why not support both? ie. machine monitoring global version and local version setting." | 12:52 |
| rogpeppe | hazmat: would the local version setting be optional? | 12:52 |
| niemeyer | Gooood morning jujuers | 12:52 |
| rogpeppe | niemeyer: yo! | 12:52 |
| TheMue | niemeyer: Moin | 12:56 |
| rogpeppe | niemeyer: any thoughts on https://codereview.appspot.com/6307072/ ? | 13:26 |
| * niemeyer looks | 13:27 | |
| niemeyer | rogpeppe: LGTM | 13:38 |
| rogpeppe | niemeyer: lovely, thanks. | 13:38 |
| niemeyer | rogpeppe: np, thank you | 13:38 |
| hazmat | rogpeppe, yes | 13:41 |
| rogpeppe | niemeyer: this could also use a look, while you're here: https://codereview.appspot.com/6305063/ | 13:41 |
| hazmat | rogpeppe, you could effect a global change via one node change, a local change via one node change. no mass changes nesc. | 13:42 |
| rogpeppe | hazmat: it means each machine agent would need to watch two things, but it would probably work ok, yeah. | 13:43 |
| niemeyer | rogpeppe: Done | 13:45 |
| rogpeppe | niemeyer: ta | 13:46 |
| niemeyer | rogpeppe: I'd prefer to have the set(upgrade=true) operation being extremely fast.. | 13:48 |
| niemeyer | rogpeppe: Enough for us to not worry about updating several entries at once | 13:48 |
| rogpeppe | niemeyer: could be thousands if we've got that number of machines | 13:49 |
| niemeyer | rogpeppe: Updating thousands of values in a database is extremely fast these days | 13:49 |
| rogpeppe | niemeyer: each one incurs a round trip. but i'll do them concurrently, which should amleliorate that. | 13:50 |
| rogpeppe | s/aml/am/ | 13:50 |
| niemeyer | rogpeppe: You'r assuming an implementation :) | 13:51 |
| niemeyer | rogpeppe: I hope we can fix that | 13:51 |
| rogpeppe | niemeyer: depends on the interface in State, i think. | 13:51 |
| niemeyer | rogpeppe: Well, if it's so painful, let's get started with global only | 13:51 |
| rogpeppe | niemeyer: i don't think it's painful. i'm just wary of it... | 13:52 |
| niemeyer | rogpeppe: We're not making use of the fact it's per unit right now either way | 13:52 |
| niemeyer | rogpeppe: Ok, global only is cool for now then | 13:52 |
| rogpeppe | niemeyer: i think it'll be relatively easy to add local machine versions later if we decide to | 13:52 |
| niemeyer | rogpeppe: Okay, sounds good.. sorry for diverting your efforts | 13:53 |
| rogpeppe | niemeyer: np. | 13:53 |
| niemeyer | rogpeppe: I hope by the time we want to have it local, the backend is fast enough for us not to worry about this | 13:53 |
| rogpeppe | niemeyer: BTW the upgrader implementation works, and is stashed away for future reference :-) | 13:53 |
| niemeyer | rogpeppe: collection.update(upgrade=true) is a blast | 13:53 |
| fwereade | niemeyer, are you currently reviewing convenient-relation-interface or add-unit-relation? if so, sorry: I'm about to -wip them, I think I have a better arrangement | 14:24 |
| TheMue | niemeyer: In https://codereview.appspot.com/6296064 I concentrated on state.go. | 14:24 |
| niemeyer | fwereade: I'm not.. just reviewing mails before stepping out for lunch.. there's still time | 14:25 |
| niemeyer | fwereade: thanks for the note | 14:25 |
| fwereade | niemeyer, nah, you don't need to look at those for a bit; I should make the s/UnitRelation/RelationUnit/ change at least | 14:25 |
| fwereade | niemeyer, and they may change in more interesting ways | 14:26 |
| niemeyer | fwereade: Does that make sense? It felt right yesterday, but I wasn't entirely sure | 14:26 |
| fwereade | niemeyer, I *think* it does, but I'm still exploring | 14:27 |
| fwereade | niemeyer, if it turns out to be crack I can just repropose the original branches | 14:27 |
| niemeyer | fwereade: I think the convention also makes sense for that tiny type we recently introduced: ServiceRelation => RelationService | 14:33 |
| niemeyer | fwereade: I recall struggling with the same kind of issue thre | 14:33 |
| niemeyer | there | 14:33 |
| niemeyer | fwereade: We did reword the docs a bit to make it clear, but didn't realize the ordering made a difference at the time | 14:34 |
| fwereade | niemeyer, yeah, that is definitely on the cards | 14:34 |
| niemeyer | TheMue: Will have to think about this one some more.. | 14:41 |
| niemeyer | TheMue: The repetition of logic there obviously needs some love.. still thinking about what would be the best approach | 14:41 |
| TheMue | niemeyer: Yes, I dislike it too and played with the idea of nesting it more. But that's also not "beautiful". | 14:42 |
| TheMue | niemeyer: The early return is typical for Go, but it looks strange. | 14:43 |
| TheMue | niemeyer: Having individual error types getting maybe a specifier, like a name, and the included error would make it look better. But logic would be the same, code would be more. | 14:44 |
| niemeyer | TheMue: I don't think that's an issue.. there are multiple ways to preserve that | 14:44 |
| niemeyer | TheMue: I'm just pondering about the best one | 14:44 |
| TheMue | niemeyer: We very often use value, err := ... and need the value later. This often leads to the structure. | 14:47 |
| niemeyer | TheMue: I understand, but the structure is fine | 14:48 |
| niemeyer | TheMue: We can avoid the repetition without touching it | 14:48 |
| rogpeppe | fwereade: ping | 14:48 |
| niemeyer | TheMue: For example: | 14:48 |
| fwereade | rogpeppe, pong | 14:48 |
| niemeyer | defer errorContext(&err, "can't add service %q: %v", name) | 14:48 |
| rogpeppe | fwereade: "cannot assign subordinate units directly to machines" | 14:48 |
| fwereade | rogpeppe, yes | 14:49 |
| rogpeppe | fwereade: how *do* we assign a subordinate unit to a machine? | 14:49 |
| TheMue | niemeyer: Hehe, thought about this too. | 14:49 |
| niemeyer | TheMue: >:) | 14:49 |
| fwereade | rogpeppe, we assign it to a unit, surely? | 14:49 |
| rogpeppe | fwereade: using AssignToMachine? | 14:49 |
| fwereade | rogpeppe, no, certainly not | 14:50 |
| rogpeppe | fwereade: so... how then? | 14:50 |
| fwereade | rogpeppe, we AddUnitSubordinateTo() | 14:50 |
| niemeyer | rogpeppe: Subordinate units must necessarily be in the same machine of their principal | 14:50 |
| fwereade | rogpeppe, I'm not sure where the responsibility lies in python for handling that though | 14:51 |
| rogpeppe | fwereade: ah, so subordinate units don't have a Machine set in their topology node? | 14:51 |
| TheMue | niemeyer: And err is a named return value. In case it's nil when the message returns errorContext() will let it stay nil, otherwise it will "pimp" the error. | 14:51 |
| niemeyer | TheMue: Yep | 14:51 |
| rogpeppe | fwereade: that means my just-submitted Machine.Units implementation is wrong | 14:51 |
| TheMue | niemeyer: OK, will add it. | 14:52 |
| fwereade | rogpeppe, sorry, not sure I saw that one; how so? | 14:52 |
| niemeyer | rogpeppe: Ah, curious | 14:52 |
| niemeyer | rogpeppe, fwereade: Worth looking at both ends in that regard.. I certainly didn't pay attention to that myself | 14:52 |
| rogpeppe | fwereade: you LGTM'd it... | 14:53 |
| rogpeppe | fwereade: it's wrong because it doesn't return subordinate units | 14:53 |
| rogpeppe | fwereade: i assumed that all units on a machine had Machine=that-machine | 14:53 |
| rogpeppe | fwereade: but if that's not true for subordinates, i'll have to do some more delving | 14:54 |
| rogpeppe | fwereade: i *think* that it would be good if the unit machine was set in the topology regardless of whether it's subordinate or not. | 14:54 |
| fwereade | rogpeppe, no doubt it will click the second I see it, but I can't parse its location out of my emails... CL please? | 14:55 |
| rogpeppe | fwereade: https://codereview.appspot.com/6307072 | 14:55 |
| fwereade | rogpeppe, there is probably a case to be made for that though | 14:55 |
| * fwereade reads and thinks | 14:55 | |
| rogpeppe | fwereade: that would mean, i think, that AddUnitSubordinateTo would automatically assign the subordinate unit to the principal's machine if there is one, and that AssignToMachine would also assign any subordinate units to that machine. | 14:56 |
| niemeyer | rogpeppe: I find that sensible too | 14:56 |
| niemeyer | rogpeppe: Although.. hmm | 14:57 |
| niemeyer | Yeah, no, that sounds good indeed | 14:57 |
| rogpeppe | niemeyer: ok, sounds like a plan. | 14:58 |
| fwereade | rogpeppe, niemeyer: the difficulty is kinda in thinking of Units as meaning Containers | 14:58 |
| niemeyer | We have evidence of algorithms being written incorrectly based on lack of a sound assumption | 14:58 |
| niemeyer | fwereade: Yeah, but they're not really | 14:58 |
| fwereade | niemeyer, indeed so | 14:58 |
| niemeyer | fwereade: The fact we're having to think this way is a hint rogpeppe is right | 14:58 |
| fwereade | niemeyer, agree Units should actually be all units, not all principal units | 14:58 |
| rogpeppe | fwereade: +1 | 14:58 |
| fwereade | rogpeppe, nice catch, ty | 14:59 |
| rogpeppe | fwereade: ok, i'll submit that as a fix first, before submitting machine-units-watcher | 14:59 |
| rogpeppe | fwereade: it was your catch - you wrote the error message that stopped my test running! | 15:00 |
| fwereade | rogpeppe, ah, I missed that detail | 15:00 |
| * fwereade feels smug | 15:00 | |
| rogpeppe | "cannot assign subordinate units directly to machines" | 15:00 |
| rogpeppe | ... and rightly so :-) | 15:00 |
| niemeyer | I'll head to lunch.. may take a bit more time today as it's Valentines here so we'll go to a proper place to have lunch. Back soon for more fun. | 15:28 |
| rogpeppe | niemeyer: have a good one! | 15:35 |
| rogpeppe | fwereade: i think that topology.AddUnit should fail if the principal unit no longer exists. does that sound right to you? | 15:36 |
| fwereade | rogpeppe, yeah, that sounds correct | 15:36 |
| rogpeppe | fwereade: ok, i'll fix that | 15:36 |
| rogpeppe | fwereade: thanks | 15:37 |
| fwereade | anyone: | 15:38 |
| fwereade | Rietveld: https://codereview.appspot.com/6300085 | 15:38 |
| fwereade | error: Failed to run "bzr merge": exit status 3 | 15:38 |
| fwereade | ----- | 15:38 |
| fwereade | bzr: ERROR: Cannot lock LockDir(chroot-80383440:///%2Bbranch/juju-core/.bzr/branch/lock): Transport operation not possible: readonly transport | 15:38 |
| fwereade | ----- | 15:38 |
| fwereade | ? | 15:38 |
| rogpeppe | fwereade: does it happen repeatedly? | 15:38 |
| fwereade | rogpeppe, yeah | 15:38 |
| rogpeppe | fwereade: running lbox submit? | 15:39 |
| fwereade | rogpeppe, indeed so | 15:39 |
| rogpeppe | fwereade: what does the output of lbox submit -v look like? (i'm wondering if it's something to do with lp:juju-core vs lp:juju-core/juju) | 15:40 |
| fwereade | rogpeppe, http://paste.ubuntu.com/1037422/ seems to be the relevant bit | 15:43 |
| rogpeppe | fwereade: hmm, odd. let's see the whole thing. i doubt i can help though, i'm afraid. | 15:45 |
| fwereade | rogpeppe, http://paste.ubuntu.com/1037428/ | 15:46 |
| fwereade | rogpeppe, but wait, yes, I see a bare lp:juju/core in there | 15:46 |
| fwereade | rogpeppe, lp:juju-core | 15:47 |
| rogpeppe | fwereade: that might be your problem. you might want to delete the merge proposal and try again, with an explict --for lp:juju-core/juju | 15:47 |
| fwereade | rogpeppe, sounds good, thanks | 15:47 |
| rogpeppe | fwereade: if in doubt, reinstall Windows. | 15:48 |
| fwereade | rogpeppe, looks like I'll be reinstalling windows :/ | 15:52 |
| rogpeppe | fwereade: that didn't help then | 15:52 |
| fwereade | rogpeppe, I'll try with a new branch from juju-core/juju and merge the original branch into that, see if it helps | 15:53 |
| twobottux | aujuju: Is juju specific to ubuntu OS on EC2 <http://askubuntu.com/questions/149952/is-juju-specific-to-ubuntu-os-on-ec2> | 15:53 |
| fwereade | rogpeppe, bah, doesn't even work when I do that | 16:00 |
| rogpeppe | fwereade: darn. i'd start googling for "bzr readonly transport" and maybe have a delve into the source. | 16:02 |
| rogpeppe | fwereade: ... and thereby waste the rest of the day | 16:02 |
| fwereade | rogpeppe, yeah, I'm going to do the stuff that's currently right on my mind and figure that out a bit later | 16:02 |
| rogpeppe | fwereade: yeah, and niemeyer may be back by then | 16:03 |
| TheMue | rogpeppe: I understand your concerns that just adding a prefix string with a "nice description" of the method name doesn't add much new context informations. | 17:07 |
| TheMue | rogpeppe: But having a chain of those calls may the error string contain more and more information (a kind of error stack). | 17:08 |
| rogpeppe | TheMue: that's going to happen either way, AFAICS. | 17:08 |
| TheMue | rogpeppe: It's only bad analyzable. | 17:09 |
| rogpeppe | TheMue: i can't parse that | 17:09 |
| TheMue | rogpeppe: So a kind of struct {msg, err} would maybe help more. | 17:09 |
| rogpeppe | TheMue: what i'm saying is orthogonal to analysability | 17:10 |
| rogpeppe | TheMue, fwereade: subordinate units machine assignment. https://codereview.appspot.com/6299073 | 17:10 |
| rogpeppe | niemeyer: ^ | 17:11 |
| TheMue | rogpeppe: How would your solution look like? | 17:12 |
| niemeyer | Yo | 17:12 |
| rogpeppe | TheMue: my message described the error messages i'd use in that function | 17:12 |
| rogpeppe | niemeyer: yi | 17:13 |
| niemeyer | rogpeppe: You have question from bcsaller there | 17:15 |
| rogpeppe | niemeyer: yeah, i'm just looking at it | 17:15 |
| rogpeppe | niemeyer: i *think* we'd decided that the MA *was* responsible for all unit deployments. | 17:16 |
| niemeyer | rogpeppe: That's not what we agreed to, no | 17:16 |
| rogpeppe | niemeyer: ok, i must've misunderstood | 17:16 |
| bcsaller | rogpeppe: thats hard to pull off anyway, supose the container is LXC, you don't have easy access to its internals from the MA, only from the UA inside that container | 17:17 |
| rogpeppe | bcsaller: very true. hmm. | 17:17 |
| bcsaller | there are ways around that, but its tricky and I didn't think it was worth it | 17:18 |
| bcsaller | rogpeppe: that said understanding the timing issue where the subs are added to late for that call to matter is also important | 17:19 |
| TheMue | niemeyer: The defer/error interface message sadly doesn't work, you can't set a new err in that method. | 17:20 |
| rogpeppe | bcsaller: i'm not quite sure what you mean by "too late for that call to matter". matter to what? | 17:20 |
| bcsaller | to make any change | 17:20 |
| bcsaller | the principal is deployed and assigned, then a day later they add a monitoring subordinate | 17:21 |
| TheMue | niemeyer: I've got a somewhat scruffy idea using an own helper error type. | 17:21 |
| bcsaller | if I read it properly the principal was suppose to tag its sub | 17:21 |
| bcsaller | with the machine id, but that won't happen | 17:21 |
| bcsaller | rogpeppe: ^ | 17:22 |
| TheMue | niemeyer: But it's indeed not beautiful. | 17:22 |
| Aram | did the email I sent to juju-dev went through? | 17:22 |
| rogpeppe | bcsaller: i don't see why not. if the principal is assigned, the subsid will be assigned when it's added | 17:22 |
| rogpeppe | bcsaller: but there is a bigger issue here. perhaps Machine.Units should not return subsidiary units, as the MA doesn't care about them. | 17:23 |
| rogpeppe | bcsaller: in which case all that branch is crack | 17:23 |
| bcsaller | rogpeppe: agreed, it should not, its not the thing managing them | 17:24 |
| niemeyer | TheMue: Yes, you actually can set an error in that method | 17:31 |
| niemeyer | s/method/function/ | 17:31 |
| rogpeppe | Aram: i haven't seen anything | 17:31 |
| rogpeppe | Aram: if you're referring to a message you've sent just recently | 17:32 |
| rogpeppe | [18:16:16] <rogpeppe> niemeyer: i *think* we'd decided that the MA *was* responsible for all unit deployments. | 17:32 |
| Aram | rogpeppe: yes, I got a moderator approval needed bounce. | 17:32 |
| rogpeppe | oh yeah, we'd actually agreed the precise opposites | 17:32 |
| rogpeppe | opposite | 17:32 |
| rogpeppe | niemeyer: so it looks like that branch is rubbish - Machine.Units shouldn't return subsidiary units; do you agree? then we can have a Unit.WatchSubsidiaries method later that the unit agent can use, perhaps. | 17:34 |
| rogpeppe | Aram: did you send it from your canonical.com address? | 17:34 |
| Aram | yes, but I used my alias instead of my real email, gah, resending. | 17:35 |
| TheMue | niemeyer: err can be set, then it's passed as reference to the deferred errorContext(). There err (which is *error) can be set, but when returning the old err is still valid. If err would be e.g. a structure I could modify a field, yes, or a slice value or map. | 17:35 |
| Aram | rogpeppe: this time it worked | 17:36 |
| TheMue | niemeyer: Deferring a func() {} and modify err in there through errorContext() with a return value should help. Wouldn't be so elegant. But less repeating than today. | 17:37 |
| niemeyer | TheMue: Sorry, I'm missing what you mean | 17:37 |
| niemeyer | TheMue: The exact signature I suggested should work, without any further hacks | 17:38 |
| niemeyer | TheMue: Why is it not working? | 17:38 |
| niemeyer | rogpeppe: Subordinates | 17:39 |
| niemeyer | rogpeppe: Why is it rubbish? | 17:39 |
| rogpeppe | niemeyer: because there's no need for the machine agent to see subordinate units. and that's what Machine.Units and Machine.UnitsWatcher is for. | 17:40 |
| TheMue | niemeyer: You pass an interface X by reference. You could call the interfaces methods or if it would be a struct you could modify the fields. But if you set the argument the scope is only inside the function. | 17:40 |
| TheMue | niemeyer: errorContext(err *error) { err = fmt.Errorf("foo") } runs (ok, you need a temp variable to create a reference), but after the call of errorContext(&e) that e stays the same. | 17:42 |
| rogpeppe | niemeyer: i've got to go, i'm afraid. i'll leave the branch out for review, but i'm pretty sure now that it's a wrong direction. | 17:43 |
| niemeyer | TheMue: Your implementation seems to be bogus somehow | 17:45 |
| niemeyer | TheMue: Can you please replicate your idea here: http://play.golang.org/ | 17:45 |
| TheMue | niemeyer: A defer func() { err = errorContext(err, "Foo %q", id) } would help. | 17:45 |
| TheMue | niemeyer: For sure. | 17:45 |
| niemeyer | TheMue: Thanks | 17:45 |
| niemeyer | rogpeppe: There is a method in the unit.. IsSubordinate() | 17:45 |
| niemeyer | Or IsPrincipal | 17:45 |
| rogpeppe | niemeyer: that re-reads the topology each time, and it's unnecessary. | 17:46 |
| rogpeppe | niemeyer: the thing watching a machine is never going to want to see subordinates | 17:46 |
| rogpeppe | AFAICS | 17:46 |
| TheMue | niemeyer: http://play.golang.org/p/s5yfet7UL2 | 17:49 |
| niemeyer | rogpeppe: We can cache that information | 17:49 |
| niemeyer | rogpeppe: A unit is either subordinate or not, for its whole life | 17:50 |
| rogpeppe | niemeyer: we could call the methods Machine.PrincipalUnits and Machine.PrincipalUnitsWatcher if we wanted to be clearer | 17:50 |
| niemeyer | rogpeppe: Well, we can also rename the methods | 17:50 |
| niemeyer | rogpeppe: To suit what we need | 17:50 |
| TheMue | niemeyer: Alternative follows. | 17:50 |
| rogpeppe | niemeyer: jinx | 17:50 |
| niemeyer | rogpeppe: Yeah | 17:50 |
| niemeyer | rogpeppe: I'll review the code and provide you some feedback so that we can either move forward or have some better conversation tomorrow | 17:51 |
| niemeyer | rogpeppe: Btw, we have to sort out the branch location too | 17:51 |
| niemeyer | rogpeppe: I'm fine with renaming it after we get some of those branches in, if we feel like so | 17:51 |
| rogpeppe | niemeyer: what's wrong with the branch location? | 17:51 |
| niemeyer | rogpeppe: The stuff we talked in the ML | 17:52 |
| rogpeppe | niemeyer: ah yeah. i definitely think it should be juju-core/trunk. | 17:52 |
| TheMue | niemeyer: http://play.golang.org/p/aFA7LtLhoR works. | 17:52 |
| niemeyer | rogpeppe: Okay, I think juju-core/stuff would be nice too.. | 17:52 |
| rogpeppe | niemeyer: -1 | 17:52 |
| niemeyer | Hm? | 17:53 |
| rogpeppe | niemeyer: "stuff" implies random shit | 17:53 |
| niemeyer | rogpeppe: stuff as in {environs,juju,testing,cmd,...} | 17:53 |
| rogpeppe | niemeyer: definitely! | 17:53 |
| niemeyer | :) | 17:54 |
| rogpeppe | niemeyer: i'd expect it to be just the same as goamz etc | 17:54 |
| rogpeppe | niemeyer: i was surprised when it wasn't | 17:54 |
| Aram | me too | 17:54 |
| Aram | that juju seemed redundant | 17:55 |
| niemeyer | TheMue: http://play.golang.org/p/aFA7LtLhoR | 17:55 |
| rogpeppe | anyway, really gotta go now! happy evening/day all. | 17:55 |
| niemeyer | rogpeppe: Thanks for the day, and have a great evening | 17:55 |
| Aram | enjoy | 17:55 |
| TheMue | niemeyer: Yes, that's my second solution. | 17:56 |
| niemeyer | TheMue: Sorry, I fail | 17:56 |
| niemeyer | TheMue: http://play.golang.org/p/yg1N6xnJyo | 17:56 |
| TheMue | niemeyer: Ah, nice, the dereferencing of the argument has been new to me. Thx, great info. | 17:58 |
| niemeyer | TheMue: Yeah, pointers.. | 17:59 |
| niemeyer | TheMue: Java "saves" you from that side of things | 17:59 |
| Aram | yeah, everything is a pointer. | 18:00 |
| niemeyer | (because *everything* is a pointer!) | 18:00 |
| niemeyer | Aram: Jinx :) | 18:00 |
| TheMue | niemeyer: Never have used them much. Not only Java, but also Erlang or Smalltalk. | 18:00 |
| niemeyer | TheMue: Erlang is a veeeery different beast :) | 18:00 |
| TheMue | niemeyer: Erlang indeeeeeeeeeeeeeeeed. But I like it, yeah. | 18:01 |
| TheMue | niemeyer: And Smalltalk also passes everything by reference. | 18:01 |
| niemeyer | TheMue: Passing by reference has a different meaning than what you imply, I suspect | 18:01 |
| TheMue | niemeyer: It's different, yes. | 18:02 |
| Aram | ALGOL passes variables not by value or reference, but by name, sort of like a macro. | 18:02 |
| niemeyer | TheMue: C++ has by-reference semantics, and it sucks | 18:02 |
| TheMue | niemeyer: Only done C++ a bit in the late 80s. | 18:03 |
| niemeyer | Aram: Oh, never read about that | 18:03 |
| TheMue | niemeyer: But it hasn't been my language. During this time I more used the good old Turbo Pascal. ;) | 18:04 |
| Aram | long time ago I introduced a Pascal friend to C, and he loved it because it wasn't really type safe like Pascal. | 18:05 |
| Aram | you could do lots of stupid tricks. | 18:05 |
| TheMue | Aram: I loved Smalltalk, but it's sadly going deeper and deeper into history. And the implementations missing good concurrent VMs. There's only one experimental by IBM. | 18:07 |
| TheMue | So, have to leave for today. | 18:31 |
| TheMue | niemeyer: First changed methods look and feel good. | 18:31 |
| niemeyer | TheMue: Sweet! | 18:31 |
| TheMue | niemeyer: ;) | 18:31 |
| niemeyer | bcsaller: ping | 20:05 |
| bcsaller | niemeyer: whats up? | 20:16 |
| niemeyer | bcsaller: Heya | 20:16 |
| niemeyer | bcsaller: I was just looking at that branch from rog | 20:16 |
| niemeyer | bcsaller: Have you found an edge case where it would leave data in an inconsistent state? | 20:17 |
| niemeyer | Hmm | 20:17 |
| bcsaller | niemeyer: thats not what I was pointing out, it was just that the method seemed to be triggered before subordinates were bound to the principal unless I read it wrong | 20:18 |
| niemeyer | bcsaller: Is there a way to create a subordinate unit without binding it to a principal? | 20:18 |
| niemeyer | bcsaller: (unit, not service) | 20:19 |
| bcsaller | the service yes, instances only appear when the relation is satisfied, you can deploy the sub w/o a realation and only ZK is changed | 20:19 |
| bcsaller | niemeyer: no, sub units always have a principal, but can be added well after the establishment of the principal | 20:19 |
| niemeyer | bcsaller: Right, but his code path also covers that | 20:20 |
| niemeyer | bcsaller: In the sense that he's checking to see if the principal is assigned by the time the subordinate is added | 20:20 |
| niemeyer | bcsaller: Do you see another angle open? | 20:21 |
| bcsaller | the other thing I mentioned is that machine assigment is the watch trigger to deploying a UA. | 20:21 |
| niemeyer | Cool | 20:22 |
| niemeyer | bcsaller: There's a race there too I suppose | 20:22 |
| bcsaller | niemeyer: in the python version we proxy the machine id lookup to the principal for the subs and don't do the assignment | 20:23 |
| bcsaller | niemeyer: that might not be the best way, but its a possible path forward | 20:23 |
| niemeyer | if the unit is assigned to a machine while concurrently a subordinate unit is added to the principal, the subordinate will be added without a machine, and the principal will not notice the existence of the unit | 20:23 |
| niemeyer | Well, or maybe not if it's properly done within a retryChange transaction | 20:23 |
| niemeyer | bcsaller: It feels less fiddly for sure | 20:24 |
| bcsaller | niemeyer: in this case the principal is processing its relation change events and deciding if it needs to take action (deploy a subordinate) | 20:24 |
| bcsaller | but the MA isn't involved | 20:24 |
| bcsaller | which in the LXC everywhere future I think is important | 20:25 |
| === davechen1y is now known as davecheney | ||
| andrewsmedina | why the zookeeper will be replaced? | 23:04 |
| davecheney | andrewsmedina: for a number of reasons, one being it won't run on arm | 23:05 |
| andrewsmedina | davecheney: hmm | 23:09 |
| andrewsmedina | juju will run on arm? | 23:11 |
| davecheney | i hope so, arm is a supported ubuntu server platform | 23:12 |
| hazmat | SpamapS, +1 rest apis and soa | 23:21 |
| hazmat | its in the plans | 23:21 |
| hazmat | i believe | 23:21 |
| SpamapS | good :) | 23:27 |
| andrewsmedina | SpamapS == Clint Byrum ? | 23:33 |
| SpamapS | andrewsmedina: indeed :) | 23:34 |
| andrewsmedina | SpamapS: :) | 23:35 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!