[00:38] ericsnow: those are pretty good questions, quick answer on client thing, client closes connection after a call and it is not as smart a type as one would hope so I can not just ask for a reconnectoin [00:39] perrito666: yuck then [00:39] and quick answer on the issue of the typecasting of client [00:39] which is very interesting [00:39] I originally did not typecast that [00:40] but then I had problems because APIClient is an interface from another package and even though the actual object returned was a Client I was not able to call c.facade.FacadeCall [00:40] bc facade is not public [00:40] perrito666: FWIW, I covered the entire patch in a single review this time! ;) [00:40] I am adding those as comments because these are very interesting questions you posed === kadams54-away is now known as kadams54 [00:41] ah, the facade field (yuck) [00:42] hopefully we can find a way to address both but I'm okay with tabling those as long as you put TODO comments (and you agree they should be fixed, of course) [00:43] ericsnow: well if we where to nuke the interface all would be fixed :p [00:43] having a user of the API client pass in a function that outputs a new API client doesn't smell right [00:43] perrito666: we need the interface for testing [00:44] ericsnow: we can have the interface be part of the other backups [00:44] I dont feel right having a public facade of sorts [00:46] perrito666: I gotta run but I'll probably check back in a little while in case you have more clarifications you want on my review [00:55] ericsnow: I am dropping issues but the explanation is in the comment below wihch I did not yet commit [00:55] bear with me plz :) I am cooking === kadams54 is now known as kadams54-away === kadams54 is now known as kadams54-away === kadams54-away is now known as kadams54 [02:44] ericsnow: lol, review mails get cutted by gmail [02:44] :) [02:46] ericsnow: btw, I am explicitly not doing anything at all to help restore work in local provider, it is a broken provider and we should not be doing things for it to work there, instead, we should fix local provider [02:46] there is no practical reason for restore to work on local [02:46] unit tests [02:46] and no, the juju script actually stops the db [02:47] ericsnow: I can add unit tsts and make things work on those, but I am purposedly not changing things to work in local [02:47] to be honest it is VERY dangerous for restore to run on local [02:47] ericsnow: btw, I need a bit of your help [02:47] very true [02:47] I want to answer one of your comments but I need to rubber duck the answer with you before [02:48] so you know I'm trying to be helpful with the reviews, in case it seems like anything else :) [02:48] so, regarding the order of prepare, upload, restore, finish [02:48] yep [02:48] sure, that is why I try to answer in a way that the review can be used as a doc later [02:48] so, the reason is [02:48] lets assume a succesful run [02:49] you call prepare, which can be likened to a sqldb begin transaction [02:49] then, you upload [02:49] then you restore from the uploaded file [02:49] so far good? [02:50] so, if anyone, after I decided to restore, did juju deploy blah. blah will be lost [02:51] so I accept it can fail in the upload [02:51] but there is a strong intention to actually destroy that state server [02:51] right [02:52] so, given the nature of this process, it is an aceptable trade off to actually freeze any possible action from other users [02:52] I prefer to have the user be mad because it gets postponed [02:52] to the user be mad because I just deleted his deployed workload [02:52] that's partly why I'm suggesting that Upload not be called within Restore() at all (only in the command) [02:53] ericsnow: we are not syncing here [02:53] if upload takes 2 hs [02:53] and it does not fail [02:53] ah, I think I see what you mean [02:53] anything done in those 2 hs will be lost [02:53] I do agree it is one of the strangest concepts of restore [02:54] It took a lot of discussion with william loking for possible failure points in restore to realize that this could happen [02:54] but if they run restore, why would they expect anything other than what they backed up at some other time to be restored? [02:55] there's no backup getting created when restore runs [02:55] ericsnow: think multi user [02:55] A begins a restore, B wants to deploy [02:56] A and B do not comunicate well [02:56] :p [02:59] I see what you mean, but I'm still not convinced prepare-before-upload is the right choice [02:59] I gotta go pretty soon, but we can pick this up on Monday [02:59] ericsnow: Ill be back here on friday [02:59] it may help to get a fresh perspective from someone else too [02:59] dang it [03:00] I'm still mostly sick and now my family is really sick :( [03:00] ouch [04:36] fwereade_: per our discussion earlier -- http://reviews.vapour.ws/r/521/ === Spads_ is now known as Spads