geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dan Smith <dsm...@pivotal.io>
Subject Re: Review Request 60875: GEODE-2654: Backups can capture different members from different points in time
Date Fri, 14 Jul 2017 21:00:29 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message