perrito666ericsnow: 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 reconnectoin00:38
ericsnowperrito666: yuck then00:39
perrito666and quick answer on the issue of the typecasting of client00:39
perrito666which is very interesting00:39
perrito666I originally did not typecast that00:39
perrito666but 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.FacadeCall00:40
perrito666bc facade is not public00:40
ericsnowperrito666: FWIW, I covered the entire patch in a single review this time! ;)00:40
perrito666I am adding those as comments because these are very interesting questions you posed00:40
=== kadams54-away is now known as kadams54
ericsnowah, the facade field (yuck)00:41
ericsnowhopefully 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:42
perrito666ericsnow: well if we where to nuke the interface all would be fixed :p00:43
ericsnowhaving a user of the API client pass in a function that outputs a new API client doesn't smell right00:43
ericsnowperrito666: we need the interface for testing00:43
perrito666ericsnow: we can have the interface be part of the other backups00:44
perrito666I dont feel right having a public facade of sorts00:44
ericsnowperrito666: I gotta run but I'll probably check back in a little while in case you have more clarifications you want on my review00:46
perrito666ericsnow: I am dropping issues but the explanation is in the comment below wihch I did not yet commit00:55
perrito666bear with me plz :) I am cooking00:55
=== kadams54 is now known as kadams54-away
=== kadams54 is now known as kadams54-away
=== kadams54-away is now known as kadams54
perrito666ericsnow: lol, review mails get cutted by gmail02:44
perrito666ericsnow: 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 provider02:46
perrito666there is no practical reason for restore to work on local02:46
ericsnowunit tests02:46
perrito666and no, the juju script actually stops the db02:46
perrito666ericsnow: I can add unit tsts and make things work on those, but I am purposedly not changing things to work in local02:47
perrito666to be honest it is VERY dangerous for restore to run on local02:47
perrito666ericsnow: btw, I need a bit of your help02:47
ericsnowvery true02:47
perrito666I want to answer one of your comments but I need to rubber duck the answer with you before02:47
ericsnowso you know I'm trying to be helpful with the reviews, in case it seems like anything else :)02:48
perrito666so, regarding the order of prepare, upload, restore, finish02:48
perrito666sure, that is why I try to answer in a way that the review can be used as a doc later02:48
perrito666so, the reason is02:48
perrito666lets assume a succesful run02:48
perrito666you call prepare, which can be likened to a sqldb begin transaction02:49
perrito666then, you upload02:49
perrito666then you restore from the uploaded file02:49
perrito666so far good?02:49
perrito666so, if anyone, after I decided to restore, did juju deploy blah. blah will be lost02:50
perrito666so I accept it can fail in the upload02:51
perrito666but there is a strong intention to actually destroy that state server02:51
perrito666so, given the nature of this process, it is an aceptable trade off to actually freeze any possible action from other users02:52
perrito666I prefer to have the user be mad because it gets postponed02:52
perrito666to the user be mad because I just deleted his deployed workload02:52
ericsnowthat's partly why I'm suggesting that Upload not be called within Restore() at all (only in the command)02:52
perrito666ericsnow: we are not syncing here02:53
perrito666if upload takes 2 hs02:53
perrito666and it does not fail02:53
ericsnowah, I think I see what you mean02:53
perrito666anything done in those 2 hs will be lost02:53
perrito666I do agree it is one of the strangest concepts of restore02:53
perrito666It took a lot of discussion with william loking for possible failure points in restore to realize that this could happen02:54
ericsnowbut if they run restore, why would they expect anything other than what they backed up at some other time  to be restored?02:54
ericsnowthere's no backup getting created when restore runs02:55
perrito666ericsnow: think multi user02:55
perrito666A begins a restore, B wants to deploy02:55
perrito666A and B do not comunicate well02:56
ericsnowI see what you mean, but I'm still not convinced prepare-before-upload is the right choice02:59
ericsnowI gotta go pretty soon, but we can pick this up on Monday02:59
perrito666ericsnow: Ill be back here on friday02:59
ericsnowit may help to get a fresh perspective from someone else too02:59
ericsnowdang it02:59
ericsnowI'm still mostly sick and now my family is really sick :(03:00
jw4fwereade_: per our discussion earlier -- http://reviews.vapour.ws/r/521/04:36
=== Spads_ is now known as Spads

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!