----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60875/#review180588 -----------------------------------------------------------
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. geode-core/src/main/java/org/apache/geode/internal/cache/BackupLock.java Lines 70 (patched) <https://reviews.apache.org/r/60875/#comment255790> I think the backup thread still needs to get the lock here. Perhaps other threads can't modify the disk files, but I think you still need to protect all access to the state of DiskInitFile and BackupLock. For example, backupThread is not volatile, so without getting the lock here we may actually see a state value for that variable. That's just one variable, I think DiskInitFile has a lot more state than that. geode-core/src/main/java/org/apache/geode/internal/cache/DiskInitFile.java Line 384 (original), 389 (patched) <https://reviews.apache.org/r/60875/#comment255792> 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. - Dan Smith 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 > >
