> On July 14, 2017, 9 p.m., Dan Smith wrote: > > I like where you are going with this and I think this will make the backup > > a lot more consistent. I think what you are doing with the oplogs looks > > good. However, the changes made to DiskInitFile don't look like they are > > threadsafe to me. I have some comments below. > > > > I'd also like to see some tests associated with this change that prove the > > new behavior is working.
I misunderstood what the changes in the init file were doing, please disregard that comment! I'd still like to see some tests though. > On July 14, 2017, 9 p.m., Dan Smith wrote: > > geode-core/src/main/java/org/apache/geode/internal/cache/DiskInitFile.java > > Line 384 (original), 389 (patched) > > <https://reviews.apache.org/r/60875/diff/1/?file=1776816#file1776816line417> > > > > The fact that you have two different locks protecting the state of this > > class I think is confusing. What variables are protected by backupLock vs. > > what variables are protected by this new lock? I think it's much safer and > > more understandable for future developers if there is only a single lock.n > > > > In fact I can see that this is not safe because I can see that we are > > modifying drMapByName under one lock but getting the value of drMapByName > > under a different lock. Please disregard this comment! Darrel set me straight - all of the state is protected by the new lock, and the backup lock is just to block when a backup is happening. I was looking at this as an else condition. - Dan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60875/#review180588 ----------------------------------------------------------- On July 14, 2017, 5:40 p.m., Lynn Gallinat wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60875/ > ----------------------------------------------------------- > > (Updated July 14, 2017, 5:40 p.m.) > > > Review request for geode, anilkumar gingade, Darrel Schneider, and Dan Smith. > > > Repository: geode > > > Description > ------- > > An online backup was not taking a snapshot of a single point in time. The > solution is for operations that change the disk files to acquire the backup > lock, causing them to wait until backup has rolled the op logs. > > > Diffs > ----- > > geode-core/src/main/java/org/apache/geode/internal/cache/BackupLock.java > 4b4fb10 > geode-core/src/main/java/org/apache/geode/internal/cache/DiskInitFile.java > 0925d28 > geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java > 3e97d0e > geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java 5399d5a > > > Diff: https://reviews.apache.org/r/60875/diff/1/ > > > Testing > ------- > > Precheckin. > > > Thanks, > > Lynn Gallinat > >
