[00:00] wallyworld, I may be blithering [00:00] we can always optimise if the approach we decide on becomes an issue [00:01] exactly [00:01] i guess i was taking the approach of don't create something until needed [00:01] since for many/most cases it won't be needed [00:02] so we are loading up the db since we don't want to deal with potentialy dirty docs [00:02] yeah, but then -- the different field sets of the varios documents actually have very distinct usage and observation patterns that lend themselves well to implementation as distinct types [00:02] oh, i want to use a distinct type - machineContainer [00:02] wallyworld, absolutely [00:03] but i was willing to pay the cost of dealing with potential stateness issues if it meant not having to create unnecessary ones [00:03] wallyworld, it is perfectly possible to create it lazily, but it's more hassle [00:03] wallyworld, you just need to do an existence query, and write a transaction based on that result, with an assertion that the condition still holds [00:04] wallyworld, if it fails, try again, at some point barf with ErrExcessiveContention [00:04] wallyworld, kinda tedious [00:05] yeah. would be nice to bake that logic into the framework [00:05] so optimistic locking becomes easy to use [00:06] wallyworld, (you also need to assert that the machine's in a state that allows for the desired operation -- eg, you can't add containers when it's not Alive) [00:06] wallyworld, yeah, I have some ideas floating around in that direction [00:07] i'm finding in general we are doing a lot of (repeated) boiler plate in juju :-( [00:08] wallyworld, I think there's fertile ground for consolidation of some of that [00:08] wallyworld, in a number of places in state [00:08] yes indeed [00:09] wallyworld, fwiw the reason machine ids ended up as strings was, er, at least partially informed by my unreasoning rage at have to copy and paste the EntityWatcher code to handle ints instead of strings [00:10] lol [00:10] if only go had generics [00:10] wallyworld, I came across the two things, they did different stuff, I spent a couple of hours analyzing their behaviour in enough detail to determine that the macroscopic effect of the two appraches was in fact equivalent [00:12] wallyworld, if I get a moment, SettingsWatcher and EnvironConfigWatcher would be candidates for replacement with EntityWatcher as well [00:12] sounds good [00:13] wallyworld, but I should sleep [00:13] yes you should. thanks for the input. appreciated [00:13] wallyworld, holiday tomorrow for me, enjoy your weekend :) [00:14] you too. i have a holiday on monday :-) [00:14] cool, enjoy [00:14] gn [00:14] hight [00:14] night === thumper-afk is now known as thumper === amithkk_ is now known as amithkk [02:42] grrrr [02:42] ffs [03:18] * thumper sighs [03:18] working out how to enfixorate this test [03:42] ha [03:42] cracked it [04:12] wallyworld: ping [04:12] hi [04:12] wallyworld: got time for EOW debrief? [04:12] sure [04:13] https://plus.google.com/hangouts/_/d0771f5fc18ee2fb1740aefa78b11db732ce9419?hl=en === tasdomas_afk is now known as tasdomas [07:49] mornin' all [09:12] TheMue, I'm on holiday, but... why are you adding an explicit TxnRevno field that you're not using? [09:16] TheMue: reviewed with some comments [09:16] fwereade__: Otherwise the watcher package doesn't work, it relies on it. [09:17] dimitern, fwiw the convention is that watchers always send an initial event representing "initial" state [09:17] dimitern, this applies to the single-entity watchers as much as the ones that send actual state [09:17] fwereade__: ah, ok, wasn't sure this is always the case [09:18] TheMue, oh really? [09:19] fwereade__: yes, that's why I had chosen a different approach first [09:19] TheMue: TxnRevno is always available on any document in a mgo collection, implicitly [09:20] TheMue: the same as the "_id" field [09:20] TheMue, dimitern: well, to be explicit, on any doc used by mgo/txn [09:20] TheMue, dimitern: it is *written* by that package [09:20] yep [09:20] fwereade__: but found it in machine, service, unit [09:21] TheMue: you only need to define it on the document when you need to read it for some reason - like the presence pingers do [09:21] TheMue, dimitern: and is exported in *some* of our entity documents, for use in single-entity watches [09:21] TheMue, dimitern: it might also be exported in certain cases to do coarse-grained optimistic locking, in which you assret the precise state of a full document against which you plan to make changes [09:22] fwereade__, dimitern: ah, good to know, will remove it. that led me into the wrong direction *hmpf* [09:22] TheMue, dimitern: but we have 0 cases currently in play where that is necessary [09:22] fwereade__: because so the WatchCollection() is indeed a great helper. [09:24] dimitern: btw, i'm almost hoarse (it's getting better), we yesterday won and are now in the finals [09:24] fwereade__: do I need to update the mongo package used by tests, because now in trunk I get this error: http://paste.ubuntu.com/5741365/ (i know that test jam and davecheney fixed relied on some retry/delay behavior that changed in mongo or maybe mgo?) [09:25] s/fixed/fixed a test that/ [09:26] dimitern: thx for your review. i forgot to destroy varnish too, to test the event [09:27] TheMue: np [09:36] TheMue, would be good to test coalescing [09:37] TheMue, destroy varnish and call Cleanup, and check that only one new change is sent after that [09:37] TheMue, for example [09:37] dimitern, hmm, yes, I think just update mgo [09:38] fwereade__: ah, cool [09:38] fwereade__: I was just looking to file a bug :) [09:38] fwereade__: yes, will do, thx for the review [09:38] TheMue, also maybe good to check the Stop behaviour, to observe the channel close, for completeness' sake [09:41] fwereade__: yup, will add it too. [09:51] another trunk test failure after updating mgo: http://paste.ubuntu.com/5741417/ [09:52] i think somebody complained about it on the list [10:11] I filed a bug 1188549 for it and sent mail to the list [10:11] _mup_: wtf? [10:11] https://bugs.launchpad.net/juju-core/+bug/1188549 [10:16] fwereade__: i think we should revert 1257 [10:17] fwereade__: due to the test failure above, unless i'm it's just me who's seeing it [10:17] dimitern, +1 [10:17] TheMue: can you pull trunk tip and run all tests on it please? [10:18] TheMue: to see if you can reproduce the bug above?\ [10:18] mgz: hey [10:20] dimitern, hmm, passes for me [10:20] fwereade__: ha.. so it might be only on my machine somehow [10:20] TheMue: please confirm ^^ [10:20] dimitern, oh wait sorry [10:21] dimitern, nope, I agree, it's broken [10:21] dimitern, +1 on reverting it, give me 1 min to peer quickly at the code [10:21] fwereade__: sure [10:22] dimitern: oh, sorry, had been on a different screen. one moment, will test [10:22] dimitern: hey! [10:25] mgz: so it turned out machine.WatchUnits is already implemented, which simplifies the deployer refactoring task [10:25] excellent [10:26] mgz: just compiling a short plan of steps what to do about it [10:28] dimitern, hey, looks kinda like crack to me [10:28] dimitern, revert away [10:28] fwereade__: ok, will do [10:29] fwereade__: one question about the deployer - you meant to remove it from the unit agent and leave it only in the machine agent, right? becasue i vaguely recall mentioning removing it from the uniter instead [10:30] dimitern, I meant agnt [10:30] fwereade__: yeah, ok [10:30] dimitern, but there's some special-case shutdown code in the uniter that we can also drop [10:31] fwereade__: i'll look into it [10:31] dimitern, because exiting the process will no longer take down the relevant Deployer [10:32] fwereade__: ah, sure we can't miss it then, once it's removed from UA [10:32] dimitern, and I *think* that the changes all go well together: switch the deployer type on machine agent, and delete a bunch of unit agent and uniter code at the same time [10:33] fwereade__: and in the first step, instead of passing a UnitsWatcher, we can just pass a machine id and call st.Machine(id).WatchUnits inside the loop, right? [10:34] dimitern, that sgtm I think [10:34] fwereade__: cool [10:38] dimitern: some problem here [10:39] dimitern: ouch, not some, same [10:39] dimitern: typo [10:39] TheMue: yeah, will revert it shortly [10:39] TheMue: thanks for checking [10:39] dimitern: yw [10:44] mgz, fwereade__: http://paste.ubuntu.com/5741545/ - some short stepwise plan for the deployer (each step is roughly a CL) [10:45] dimitern, (1) and (2) must land in the same CL [10:45] dimitern, otherwise you'll end up with double-deployed subordinate unit agents [10:46] dimitern, but the uniter shutdown code cleanup can come after, to keep things small and stepwise [10:47] dimitern, you'll need WatchUnits over that facade as well [10:48] hm, is the state between 2 and 3 working? [10:48] mgz, yes, because 1/2 are changes to state, and to the existing code, without hitting the API at all [10:48] ...that's not really the right question [10:49] why is it safe to remove the deployer task from the unit agent, what is happening instead? [10:50] dimitern, something like `WatchMachineUnits([]MachineId) ([]struct{watcherid, error}, error)` [10:51] mgz, Machine.WatchUnits reports changes to all a machine's units: both its principals and their subordinates [10:51] fwereade__: oh, sure [10:51] ah, I see [10:51] mgz, so the necessary synchronization is that we must remove the unit agent deployer at the same time we change deployer to always watch a machine's complete complement of units [10:51] so the previous change makes the machine agent responsible for both, which is why 1+2 need to be done together [10:51] mgz, hence 1/2 having to go together [10:52] mgz, exactly [10:52] mgz: dump question about bzr revers again - do I do bzr pull on trunk, then create a branch as usual, and do "bzr merge . -r1256..1257" if I need to revert only 1257? [10:52] -r1257..1256 [10:52] mgz, there's a bit of side work, in uniter/ModeShutdown() (or something like that) that causes the uniter to delay its own death until its subordinates are removed [10:53] mgz: oh, I keep forgetting the order and always seems weird [10:53] merging the *reverse* of the change, hence the bigger rev first [10:53] mgz, that can be removed safely once the other is done, but won't hurt anything if it persists after the switch [10:54] mgz, the effect of removing it will be that the principal unit agent dies a bit earlier, once it has no further duties to perform [10:54] mgz, and the uniter becomes a smidgen simpler [10:58] whoops, forgot the topic === danilos_ is now known as danilos === ChanServ changed the topic of #juju-dev to: https://juju.ubuntu.com | On-call reviewer: danilos | Bugs: 8 Critical, 71 High - https://bugs.launchpad.net/juju-core/ [10:59] fwereade__, mgz: updated the plan http://paste.ubuntu.com/5741571/ [11:00] dimitern, uniter shutdown code can and should be a separate step [11:01] dimitern, and 3 can be expanded a little into two [11:01] dimitern, writing a clone of the deployer and unit testing it against api rather than state is a perfectly good step [11:01] 3 [11:01] fwereade__: but if that shutdown code relies on having a deployer and it's gone, won't that be a problem? [11:01] dimitern, step 4 can be dropping the state version and integrating the api version [11:02] dimitern, its duties are taken over by the deployer in the machine agent [11:02] dimitern, so a relevant deployer is always available [11:02] dimitern, it's just running in a different process now [11:03] fwereade__: hmm, ok - i haven't looked at the code, sorry [11:04] dimitern, it's ModeTerminating [11:04] dimitern, the whole loop dickery can be straight-up dropped, and we can return ErrterminateAgent directly [11:04] dimitern, (once we've set StatusStopped) [11:05] I'd like to tackle the testing deployer clone with some pointers [11:06] mgz, really sorry, I have to go out to lunch now *but* if you ask questions now I might be able to drive-by them [11:06] mgz, will try to do a quick pass on anything you manage to type in the next 10 mins ;p [11:06] bbs [11:06] no problem, I'm sure dimitern will happily boss me around [11:06] (after that I'll actuallybe at lunch) [11:12] mgz: am i that bad? :) [11:12] you're that good! [11:18] :) [11:19] fwereade__, mgz: so this is the (somewhat) final plan http://paste.ubuntu.com/5741603/ [11:19] mgz: I'll appreciate an LGTM on this https://codereview.appspot.com/10046047/ [11:19] looking at both [11:22] mgz: step 3 might need to be split in 2 parts still - apiserver implementation and client api implementation [11:22] dimitern, mgz, +1 to that [11:23] dimitern, mgz: and I'm comfortable with following a general approach of unit tests for the server, integration tests for the client [11:24] fwereade__: +100 on that [11:25] dimitern, long-term we can get smarter -- more cheap client unit tests, fewer heavyweight integration tests -- but for now it's a sane convention [11:26] sure [11:26] * fwereade__ really lunching now [11:30] danilos, mgz: are we g+-ing today or mumble-ing for standup? [11:31] dimitern, hanging out I suppose :) [12:02] danilos: can you send me a link to your CL again please? [12:03] dimitern, sure, https://codereview.appspot.com/9876043/ [12:03] danilos: thanks [12:03] dimitern, I suspect yours is https://codereview.appspot.com/9937045/? [12:04] fwereade__: in case you're still around at some point, take a look at the https://codereview.appspot.com/9937045/ watchers stuff [12:04] danilos: yep [12:25] TheMue: you have a second review [12:35] danilos: reviewed as well [12:37] mgz: so how do you feel about the api stuff lately? does it make sense, and does it help to understand what'll be needed for the deployer? [12:37] dimitern, thanks [12:37] dimitern, you've got a review as well, so perfect timing :) [12:37] yeah, is much clearer to me now [12:38] danilos: thanks! [12:39] mgz: only the watcher stuff is a bit muddy, that why I really wanted to finish it of at both server and client side for the machiner, so it can be used as a model, but not sure if I'd manage [12:40] hi, do you guys have any tricks to the problem of having several versions of a local charm in the same directory? Like [12:40] ~/charms/precise/{postgresql,postgresql-fix-1234} [12:40] danilos: blast! that's not right at all, I should've looked [12:40] juju won't know which one to deploy, as both are called "postgresql", and the directory name is irrelevant [12:40] dimitern, the logging changes? [12:40] danilos: something went wrong with my long pipeline and trunk merges [12:40] danilos: yep [12:41] danilos: i'll fix it an repropose, sorry - the bits that were weird should be there :) [12:41] dimitern, right, I assumed as much, but wanted to put it out there just in case [12:41] dimitern, heh, cool [12:42] ahasenack, if they have different revisions, you can use those [12:42] ahasenack, otherwise, you could change the charm name in the metadata to distinguish them more clearly [12:43] ahasenack, otherwise you'll get one of the ones in the local repo with the highest revision [12:43] fwereade__: if I have 10k directories under ~/charms/precise, juju will look into */metadata.yaml, right? [12:43] ahasenack, yeah [12:43] fwereade__: ok [12:44] ahasenack, the local repo has always been a bit questionable, it's hard to fit it into a model that matches the charm store and we didn't really do it very nicely [12:44] the thing with changing the charm name is dangerous to forget, you have to eventually revert that before proposing [12:44] and revert revert when fixing up a review comment, etc [12:44] ok [12:44] ahasenack, yeah, understood -- I'm very open to suggestions that help us improve the experience [12:45] use the directory name would be my first suggestion, there can only be one charm inside it, use whatever you find in *that* metadata.yaml [12:45] but sure, there are ramifications I don't see [12:45] it's how I tried to use it the first time, and fell on my face :) [13:06] lunch [13:18] ahasenack, that does sound like a more naturally comprehensible model, indeed, but the main problem is that the charm metadata name is expected to match the charm url name, and charms are requested from the repo by charm url [13:19] that is the root of the problem [13:21] ahasenack, it may be the case that we could transparently fake something up, but I feel the bump-revision business is already somewhat hokey, and adding further layers of hokiness feels potentially, er, suboptimal [13:22] fwereade__: on that topic, what do you think about making this error: "juju deploy --repository /foo/bar mycharm" [13:22] i.e., without the local: prefix before mycharm [13:22] I don't know what it will deploy, honestly. I think it would deploy mycharm from the store [13:23] ahasenack, yeah, it would, and I see how it's very reasonable to infer that of course a local repository is intended in that case [13:28] ahasenack, what's your opinion on `juju deploy --repository /foo/bar cs:somecharm`? if we make the `local:` inference in the case you describe, this one feels like it becomes ill-formed, because a local repo cannot contain cs: charms [13:29] fwereade__: that command should fail [13:29] fwereade__: I think --repository is all that is needed to infer where the charm is [13:29] fwereade__: what are the possible values for --repository? [13:29] ahasenack, always interpreted as a path at the moment [13:29] ahasenack, probably never used otherwise [13:30] fwereade__: are "juju deploy mycharm" and "juju deploy cs:mycharm" the same? [13:31] ahasenack, yeah [13:31] fwereade__: what about "juju deploy local:mycharm", what does it do? [13:31] ahasenack, the inference process is as follows: [13:32] ahasenack, infer a url from the supplied string, using schema cs: (if not specified) and series default-series (if not specified) [13:32] ahasenack, infer a repo from the schema and the repo path, take from $JUJU_REPOSITORY if --repository is not specified [13:33] ahasenack, ask that repo about the inferred url; if the url was unrevisioned, pick the latest revision reported by the repo and add it to the url [13:33] ahasenack, use the charm thus uniquely identified by that url [13:34] ahasenack, (uniquely... yes for the charm store, not so much for local repos) === BradCrittenden is now known as bac [13:34] fwereade__: sounds complicated [13:34] fwereade__: I'm confused why we have two possible "sources" in the same command line [13:34] fwereade__: --repository, and the prefix for the charm name [13:34] fwereade__: bzr doesn't have any of that, for example, and I think we were at some point following the bzr url conventions [13:35] it's bzr branch /foo /bar [13:35] bzr branch lp:foo [13:35] bzr branch lp:~oioi/foo/bar [13:35] and so on [13:35] ahasenack, yeah, it's a lot of effort just to make `juju deploy wordpress` work [13:35] works with both local and remote [13:35] but, now we have to worry about backwards compatibility [13:36] so that's why I suggested to make the invalid combinations an error [13:36] ahasenack, yeah, I am cautiously with you on this [13:37] ahasenack, can I ask you to mail the juju list about it, just in case anyone's using it in some important way, before we agree it's definitely a bug and can be changed as you suggest? [13:37] sure [13:38] ahasenack, (fwiw I presume you don't ever actually put bundled charms in your local repos?) [13:38] ahasenack, hell even if you do we can make it sane that way [13:39] fwereade__: what is a bundled charm? [13:39] ahasenack, a zip file basically -- just a serialized charm dir [13:39] never heard of it, so no :) [13:39] ahasenack, the format you download charms from the store in vs the format you edit and develop them in [13:40] fwereade__: didn't even know there was a different way to grab charms other than bzr branch [13:41] ahasenack, there's not much point to it AFAICT but it is a public format [13:42] ahasenack, I think it's important to maintain the matching-names guarantee, though [13:44] ahasenack, so making a local repos that behaves no less insanely will be a little bit fiddly [13:44] ahasenack, almost certainly worth it though [13:45] fwereade__: I don't know, my personal feeling is that it's very restrictive and surprising [13:45] but any change here could have far reaching consequences [13:45] so I would just start with making the obvious incorrect combinations error out [13:45] maybe someone will jump in and explain what was the reasoning behind --repository and local:charm === wedgwood_away is now known as wedgwood [13:57] fwereade__: can you have another look at https://codereview.appspot.com/9937045/ please? [13:57] niemeyer: hey [13:58] niemeyer: i think _mup_ has been misbehaving again and not responding to bug # [13:59] rogpeppe: I don't want to get into another 3h argument about the watchers, but I think the idea about how watchers have to be implemented the same way in the server-side API is wrong [13:59] dimitern: how do you mean? [14:00] rogpeppe: we do need to do coalescing at client side, so that we don't drop events [14:00] rogpeppe: and as well as we do that, we can handle the initial events there [14:01] dimitern: have you got a link to the kanban hangout please? my browser is refusing to load google calendar [14:02] rogpeppe: https://plus.google.com/hangouts/_/539f4239bf2fd8f454b789d64cd7307166bc9083 [14:02] dimitern: i think that initial events and client-side coalescing are two independent issues [14:02] dimitern: thanks [14:07] dimitern: It was down [14:08] rogpeppe, dimitern: fwiw I would probably accept rog's flip-flop proposal for the client-side watcher wrappers [14:08] rogpeppe, dimitern: we can build in coalescence later if the slightly-surprising event delivery burstiness is a problem for client code [14:09] fwereade__: +1 [14:09] fwereade__: that would make client-side watchers *much* simpler too [14:09] rogpeppe, dimitern: if we've done them all right it won;t be [14:09] fwereade__: what's the flip-flop stuff? [14:10] rogpeppe, dimitern: I'm just a little twitchy about it because, well, there's bound to be somewhere where someone has made unwarranted assumptions about watcher behaviour [14:10] dimitern, always have one of in and out be nil [14:10] dimitern, so always deliver events with exactly the grouping they're initially received with [14:11] dimitern, this should not matter [14:11] dimitern, and probably doesn't [14:12] fwereade__: but how about the initial Changes call at server-side [14:12] dimitern, I'm comfortable with whatever convention we pick [14:14] dimitern, if the convention is that every server-side Watch method pulls the first event, and returns anything interesting in its results, and that every client-side watcher always deliver those results in the first event on its Changes channel, then I'm happy [14:15] dimitern, rogpeppe: since we've got the current Next behaviour we may as wll try to make use of it -- some use cases, like uniter.Filter, still abuse it, but we can worry about that when it becomes a problem [14:15] rogpeppe, fwereade__how about not calling changes at server-side, because we cannot accomodate all watchers (i.e. lifecycle one has non-empty changes) [14:16] dimitern: i'm not sure what you mean there [14:16] dimitern, the first event on a watcher is always delivered as soon as possible, and represents the base state relative to which change notifications will be delivered [14:16] fwereade__: i still think that sending the initial event back with the Watch request is a reasonable thing to do [14:16] rogpeppe, me too [14:16] s/the Watch request/the response to the Watch request/ [14:17] fwereade__: oh, good [14:17] fwereade__: i thought that was considered a problem [14:17] rogpeppe, I just hadn't thought of it [14:17] rogpeppe: we discussed client-side watchers to return struct{}{} instead of changes, and then after you're notified, you can get the actual changes from the api separately [14:17] rogpeppe, but it makes perfect sense, especially when we've got flip-flop client-side watchers [14:17] dimitern: let's not do that [14:18] rogpeppe, we don't even need to call Next until the initial result has actually been handed off to the watcher's client [14:18] dimitern: let's just do for {data := Next(); c <- data} [14:18] fwereade__: exactly [14:18] rogpeppe: so then instead, we do proper coalescing on client-side [14:18] dimitern, depends on the watcher [14:18] dimitern: no need [14:18] dimitern, lists of documents changing state need to be lists of ids [14:18] rogpeppe: you're arguing events won't be dropped? [14:18] dimitern: yes [14:19] dimitern: not with the above for loop [14:19] rogpeppe: i don't see how you can guarantee that [14:19] dimitern: how can an event be dropped? [14:19] rogpeppe: with the current client-side watchers you do [14:19] dimitern, we never read a new event to drop from the common watcher until we've delivered the previous event to the client [14:19] dimitern: yes, but they're being too clever [14:19] dimitern: we're gonna axe all that code [14:19] dimitern: and go *much* simpler [14:19] rogpeppe: ok then [14:20] dimitern: the server side does coalescing already. [14:20] rogpeppe: as long as we keep to the standards we agreed (please) about how server-side should have bulk ops and client-side only if needed [14:20] dimitern: i have a possible plan to add universal bulk ops to the rpc package [14:21] rogpeppe, that sounds pretty frickin' sweet to me [14:21] dimitern: that would mean that a) you'd only need to implement bulk ops when you could do so meaningfully and b) you'd get benefit from using bulk ops even if the server methods were implemented as singletons [14:21] rogpeppe: well, as long as it provides the same protocol with less code and better readibility, i'm +1 [14:21] fwereade__: pretty much [14:22] fwereade__: the idea is you'd send an rpc request with Ids: ["id1", "id2"] rather than Id: "id1" [14:22] rogpeppe: i don't get 1) and 2) [14:22] rogpeppe: seems too smart [14:23] fwereade__: then the rpc server would try to find a specific bulk request handler for the type, and fall back to single methods (concurrently) if there's none [14:23] rogpeppe: i have to see it in other words :) [14:23] dimitern: i think it might work ok [14:23] rogpeppe: where's the concurrency happening? server-side only? [14:23] rogpeppe, hmm, honestly, I think it's too speculative [14:23] dimitern: yes [14:24] fwereade__: it means quite a bit less work implementing server methods [14:24] fwereade__: for a relatively small piece of work in the rpc package [14:24] fwereade__: i *think* the trade-off is worth it [14:25] rogpeppe: will it also involve splitting the rpc package in more manageable chunks, which could be unit-tested separately? [14:26] dimitern: maybe. [14:26] dimitern: the rpc package isn't exactly large though [14:26] rogpeppe: I read that as "no" :) [14:26] rogpeppe: but anyway, we can do that later, no worries [14:26] dimitern: what do you think is unmanageable about the rpc package? [14:26] dimitern: i think it's tested reasonably well, no? [14:26] rogpeppe: i'm not getting back into the argument big vs. small packages [14:26] :) [14:26] dimitern: it's only 800 lines of code [14:27] rogpeppe, I would love to see the authentication behaviour split off [14:27] fwereade__: that's not part of the rpc package [14:27] rogpeppe, but I don't wish to force the issue now [14:27] rogpeppe: well, once we implement the separate login step in the authentication, which is not part of the rpc interface, we'll need to split parts of it related to marshalling to reuse them [14:27] rogpeppe, ofc, sorry, I don't have a quarrel with the rpc package itself [14:28] dimitern: i do actually have a CL somewhere that splits out the method introspection code so it can be used independently. [14:28] rogpeppe: great to hear [14:28] dimitern: those bits are already in a separate package [14:28] sorry bbiab [14:28] dimitern: the rpc package doesn't do any marshalling or unmarshalling [14:28] dimitern: rpc/jsoncodec does that [14:28] rogpeppe, anyway, please tell me a smidgen more: how are you planning to register the handlers? [14:29] fwereade__: which handlers? [14:29] rogpeppe, for bulk vs single operations [14:30] fwereade__: my plan was to treat some names specially - for instance types with a "Vector" suffix. [14:30] rogpeppe, you are talking about some sort of magic which I am prepared to consider, possibly, to be worth the effort, but I'd like to understand exactly what's involved :) [14:30] fwereade__: then methods on those would have to conform to the signature foo(args someStruct) (ret []resultStruct, errs []error) [14:31] fwereade__: where the params and result struct would need to match the non-bulk form of the operation if that's provided [14:31] rogpeppe, ok, so, that's how we implement a bulk API method in terms of single-entity methods [14:32] rogpeppe, where do we get the implementation for the bulk form? what happens if both single and bulk exist? [14:32] fwereade__: the bulk API method doesn't need to call the single-entity methods [14:33] fwereade__: for operations on a single id, the single form is chosen by preference, falling back to bulk form. for operations on many ids, the bulk form is chosen by preference, falling back to the single form called concurrently on all the ids. [14:33] fwereade__: we might want to call the bulk form preferentially in all cases though [14:34] rogpeppe, ok, that sounds nice tbh, I can imagine optimized single-use variants being useful [14:34] fwereade__: i'm not sure. the logic in the rpc packageis not hard though (a couple of map lookups) [14:34] fwereade__: cool [14:34] rogpeppe, but it lives or dies in my mind by clarity and comprehensibility of exactly what API methods will be provided by a given facade type [14:35] fwereade__: one problem i have with the current facade stuff is that it's only one type [14:35] fwereade__: but resources (watchers etc) don't fit within that. [14:35] rogpeppe, that one type is equivalent to State -- it's just a cut-down variant tuned to the client's needs [14:36] fwereade__: that one type doesn't include the watcher Next methods, for example [14:36] fwereade__: so it doesn't actually include the entire API surface area for the agent [14:37] fwereade__: one possibility i've been toying with is the idea of allowing an rpc server to dynamically change the current object being served. [14:37] rogpeppe, suggestion, half baked: Watcher facade, with Next(watcherId) and NextIds(watcherId) methods? [14:39] rogpeppe, such that everything authenticated can access that facade [14:39] fwereade__: not sure. i'd prefer to go more the direction of allowing a given agent to access a set of types rather than just one type [14:40] rogpeppe, the RegisterResource stuff on the server STM to be hinting in that direction -- there are common patterns that apply across all kinds of watcher [14:40] fwereade__: so the API served for the machine agent, say, might have just Machine and EntityWatcher [14:40] fwereade__: there's definitely something a little nicer lurking somewhere here [14:41]