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