You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Lynn Gallinat <lg...@pivotal.io> on 2017/07/14 17:40:46 UTC
Review Request 60875: GEODE-2654: Backups can capture different
members from different points in time
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60875/
-----------------------------------------------------------
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
Re: Review Request 60875: GEODE-2654: Backups can capture different
members from different points in time
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60875/#review180603
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/BackupLock.java
Lines 72 (patched)
<https://reviews.apache.org/r/60875/#comment255810>
This whole backupThread thing is clunky and I think Dan is correct that with the way it is currently implemented it needs to have super.lock.
I think it would be better to have the method "setBackupThread(Thread)" to instead be "setBackupThread()" and just have it set a ThreadLocal<Boolean> to true that says the current thread is doing a backup. Add a method "boolean isCurrentThreadDoingBackup()" and it could just call this method. unlockForBackup would unset the thread local.
- Darrel Schneider
On July 14, 2017, 10:40 a.m., Lynn Gallinat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60875/
> -----------------------------------------------------------
>
> (Updated July 14, 2017, 10:40 a.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
>
>
Re: Review Request 60875: GEODE-2654: Backups can capture different
members from different points in time
Posted by Dan Smith <ds...@pivotal.io>.
> 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
>
>
Re: Review Request 60875: GEODE-2654: Backups can capture different
members from different points in time
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 60875: GEODE-2654: Backups can capture different
members from different points in time
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60875/#review181388
-----------------------------------------------------------
Ship it!
Ship It!
- Darrel Schneider
On July 25, 2017, 12:03 p.m., Lynn Gallinat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60875/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 12:03 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
> geode-core/src/test/java/org/apache/geode/internal/cache/persistence/BackupPrepareAndFinishMsgDUnitTest.java PRE-CREATION
> geode-core/src/test/resources/org/apache/geode/codeAnalysis/excludedClasses.txt db234e0
> geode-core/src/test/resources/org/apache/geode/codeAnalysis/sanctionedSerializables.txt e5a111c
>
>
> Diff: https://reviews.apache.org/r/60875/diff/2/
>
>
> Testing
> -------
>
> Precheckin.
>
>
> Thanks,
>
> Lynn Gallinat
>
>
Re: Review Request 60875: GEODE-2654: Backups can capture different
members from different points in time
Posted by Lynn Gallinat <lg...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60875/
-----------------------------------------------------------
(Updated July 25, 2017, 7:03 p.m.)
Review request for geode, anilkumar gingade, Darrel Schneider, and Dan Smith.
Changes
-------
This includes changes in response to review comments and a dunit test.
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 (updated)
-----
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
geode-core/src/test/java/org/apache/geode/internal/cache/persistence/BackupPrepareAndFinishMsgDUnitTest.java PRE-CREATION
geode-core/src/test/resources/org/apache/geode/codeAnalysis/excludedClasses.txt db234e0
geode-core/src/test/resources/org/apache/geode/codeAnalysis/sanctionedSerializables.txt e5a111c
Diff: https://reviews.apache.org/r/60875/diff/2/
Changes: https://reviews.apache.org/r/60875/diff/1-2/
Testing
-------
Precheckin.
Thanks,
Lynn Gallinat