| 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:38 |
|---|---|---|
| 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:39 |
| 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:40 |
| === kadams54-away is now known as kadams54 | ||
| ericsnow | ah, the facade field (yuck) | 00:41 |
| 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:42 |
| 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:43 |
| 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:44 |
| 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:46 |
| 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 | 00:55 |
| === kadams54 is now known as kadams54-away | ||
| === kadams54 is now known as kadams54-away | ||
| === kadams54-away is now known as kadams54 | ||
| perrito666 | ericsnow: lol, review mails get cutted by gmail | 02:44 |
| ericsnow | :) | 02:44 |
| 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:46 |
| 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:47 |
| 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:48 |
| 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:49 |
| perrito666 | so, if anyone, after I decided to restore, did juju deploy blah. blah will be lost | 02:50 |
| 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:51 |
| 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:52 |
| 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:53 |
| 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:54 |
| 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:55 |
| perrito666 | A and B do not comunicate well | 02:56 |
| perrito666 | :p | 02:56 |
| 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 | 02:59 |
| ericsnow | I'm still mostly sick and now my family is really sick :( | 03:00 |
| perrito666 | ouch | 03:00 |
| jw4 | fwereade_: 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!