On Tue, Nov 22, 2016 at 3:17 PM, Vladimir Rodionov <vladrodio...@gmail.com> wrote:
> >> and/or he answered most of the review feedback > > No, questions are still open, but I do not see any blockers and we have > HBASE-16940 to address these questions. > > Agree. No blockers but stuff that should be dealt with (No one will pay me any attention once merge goes in -- smile). St.Ack > On Tue, Nov 22, 2016 at 3:04 PM, Devaraj Das <d...@hortonworks.com> wrote: > > > Hi Stack, hats off to you for spending so much time on this! Thanks! From > > my understanding, Vlad has raised follow-up jiras for the issues you > > raised, and/or he answered most of the review feedback. So, do you think > we > > could do a merge vote now? > > Devaraj. > > ________________________________________ > > From: Vladimir Rodionov <vladrodio...@gmail.com> > > Sent: Monday, November 21, 2016 8:34 PM > > To: dev@hbase.apache.org > > Subject: Re: [DISCUSSION] Merge Backup / Restore - Branch HBASE-7912 > > > > >> I have spent a good bit of time reviewing and testing this feature. I > > would > > >> like my review and concerns addressed and I'd like it to be clear how; > > >> either explicit follow-on issues, pointers to where in the patch or > doc > > my > > >> remarks have been catered to, etc. Until then, I am against commit. > > > > Stack, mega patch review comments will be addressed in the dedicated > JIRA: > > HBASE-16940 > > I have open several other JIRAs to address your other comments (not on > > review board). > > > > Details are here (end of the thread): > > https://issues.apache.org/jira/browse/HBASE-14123 > > > > Let me know what else should we do to move merge forward. > > > > -Vlad > > > > > > On Fri, Nov 18, 2016 at 4:54 PM, Stack <st...@duboce.net> wrote: > > > > > On Fri, Nov 18, 2016 at 3:53 PM, Ted Yu <yuzhih...@gmail.com> wrote: > > > > > > > Thanks, Matteo. > > > > > > > > bq. restore is not clear if given an incremental id it will do the > full > > > > restore from full up to that point or if i need to apply manually > > > > everything > > > > > > > > The restore takes into consideration of the dependent backup(s). > > > > So there is no need to apply preceding backup(s) manually. > > > > > > > > > > > I ask this question on the issue. It is not clear from the usage or doc > > how > > > to run a restore from incremental. Can you fix in doc and usage how so > I > > > can be clear and try it. Currently I am stuck verifying a round trip > > backup > > > restore made of incrementals. > > > > > > Thanks, > > > S > > > > > > > > > > > > > On Fri, Nov 18, 2016 at 3:48 PM, Matteo Bertozzi < > > > theo.berto...@gmail.com> > > > > wrote: > > > > > > > > > I did one last pass to the mega patch. I don't see anything major > > that > > > > > should block the merge. > > > > > > > > > > - most of the code is isolated in the backup package > > > > > - all the backup code is client side > > > > > - there are few changes to the server side, mainly for cleaners, > wal > > > > > rolling and similar (which is ok) > > > > > - there is a good number of tests, and an integration test > > > > > > > > > > the code seems to have still some left overs from the old > > > implementation, > > > > > and some stuff needs a cleanup. but I don't think this should be > used > > > as > > > > an > > > > > argument to block the merge. I think the guys will keep working on > > this > > > > and > > > > > they may also get help of others once the patch is in master. > > > > > > > > > > I still have my concerns about the current limitations, but these > are > > > > > things already planned for phase 3, so some of this stuff may even > be > > > in > > > > > the final 2.0. > > > > > but as long as we have a "current limitations" section in the user > > > guide > > > > > mentioning important stuff like the ones below, I'm ok with it. > > > > > - if you write to the table with Durability.SKIP_WALS your data > will > > > not > > > > > be in the incremental-backup > > > > > - if you bulkload files that data will not be in the incremental > > > backup > > > > > (HBASE-14417) > > > > > - the incremental backup will not only contains the data of the > > table > > > > you > > > > > specified but also the regions from other tables that are on the > same > > > set > > > > > of RSs (HBASE-14141) ...maybe a note about security around this > topic > > > > > - the incremental backup will not contains just the "latest row" > > > between > > > > > backup A and B, but it will also contains all the updates occurred > in > > > > > between. but the restore does not allow you to restore up to a > > certain > > > > > point in time, the restore will always be up to the "latest backup > > > > point". > > > > > - you should limit the number of "incremental" up to N (or maybe > > > SIZE), > > > > to > > > > > avoid replay time becoming the bottleneck. (HBASE-14135) > > > > > > > > > > I'll be ok even with the above not being in the final 2.0, > > > > > but i'd like to see as blocker for the final 2.0 (not the merge) > > > > > - the backup code moved in an hbase-backup module > > > > > - and some more work around tools, especially to try to unify and > > make > > > > > simple the backup experience (simple example: in some case there > is a > > > > > backup_id argument in others a backupId argument. or things like.. > > > > restore > > > > > is not clear if given an incremental id it will do the full restore > > > from > > > > > full up to that point or if i need to apply manually everything). > > > > > > > > > > in conclusion, I think we can open a merge vote. I'll be +1 on it, > > and > > > I > > > > > think we should try to reject -1 with just a "code cleanup" > > motivation, > > > > > since there will still be work going on on the code after the > merge. > > > > > > > > > > Matteo > > > > > > > > > > > > > > > On Sun, Nov 6, 2016 at 10:54 PM, Devaraj Das <d...@hortonworks.com > > > > > > wrote: > > > > > > > > > > > Stack and others, anything else on the patch? Merge to master > now? > > > > > > > > > > > > > > > > > > > > >
