GEODE-9990: turn DiskAccessException into CacheClosedException#7334
GEODE-9990: turn DiskAccessException into CacheClosedException#7334jmelchio merged 4 commits intoapache:developfrom
Conversation
jmelchio
requested review from
agingade,
dschneider-pivotal,
gesterzhou,
kirklund,
pivotal-eshu and
upthewaterspout
jmelchio
force-pushed
the
geode-9990
branch
2 times, most recently
from
2d92258 to
fa0b210
Compare
- when DiskInitFile is in closed state and DiskStoreImpl is closed or closing - catch DiskAccessException in PRHARedundancyProvider and turn into CacheClosedException if cache closing is in progress - change CreateBucketMessage to handle DiskAccessException as cause of ReplyException
jmelchio
requested review from
BenjaminPerryRoss,
DonalEvans,
boglesby,
jchen21,
mhansonp and
nabarunnag
as code owners
geode-core/src/main/java/org/apache/geode/internal/cache/DiskInitFile.java
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java
Outdated
Show resolved
Hide resolved
DonalEvans
left a comment
DonalEvans
left a comment
There was a problem hiding this comment.
This PR is lacking any tests to validate the fix. Would it be possible to add a DUnit test to show that with these changes, a remote DiskAccessException due to the cache closing during bucket creation does not result in the initiating member seeing that DiskAccessException? If that proves too complex to set up reliably in a DUnit test, at the very least unit test coverage should be added to validate the new behaviour.
| parent.getCache().getCancelCriterion().checkCancelInProgress(); | ||
|
|
||
| if (parent.isClosed() || parent.isClosing()) { | ||
| throw new CacheClosedException("The disk store is closed or closing"); |
There was a problem hiding this comment.
I am not sure why CacheClosedException is thrown here. If the cache is closed, I assume checkCancelInProgress will throw CacheClosedException.
There was a problem hiding this comment.
The cache is not closed yet. But is going to be closed by the exception with disk-store. Also the cache close happens in the async thread, that will take some time to get initiated.
There was a problem hiding this comment.
Right. Looking into cancelInProgress() in DiskStoreImpl.Stopper, it only checksisClosed(). But it doesn't check isClosing(). It is good to have if (parent.isClosed() || parent.isClosing()) here.
There was a problem hiding this comment.
I wonder if we should use OplogCancelledException instead of CacheClosedException here?
|
some unit test to test the updated code path would be nice |
- refactor to reduce code duplication - remove surplus logging - unit test additons for PRHARedundancyProvider
| parent.getCache().getCancelCriterion().checkCancelInProgress(); | ||
|
|
||
| if (parent.isClosed() || parent.isClosing()) { | ||
| throw new CacheClosedException("The disk store is closed or closing"); |
There was a problem hiding this comment.
Right. Looking into cancelInProgress() in DiskStoreImpl.Stopper, it only checksisClosed(). But it doesn't check isClosing(). It is good to have if (parent.isClosed() || parent.isClosing()) here.
onichols-pivotal
left a comment
onichols-pivotal
left a comment
There was a problem hiding this comment.
As per https://cwiki.apache.org/confluence/display/GEODE/Commit+Message+Format, first commit summary should be under 30 characters (not including bug# or pr#) (yours is 50 characters). If you amend your initial commit now and force-push, you can benefit from review feedback on your commit message, and you won't have to remember to fix it later when you merge.
…e#7334) * GEODE-9990: turn DiskAccessException into CacheClosedException - when DiskInitFile is in closed state and DiskStoreImpl is closed or closing - catch DiskAccessException in PRHARedundancyProvider and turn into CacheClosedException if cache closing is in progress - change CreateBucketMessage to handle DiskAccessException as cause of ReplyException (cherry picked from commit a98197b)
#7374) * GEODE-9990: turn DiskAccessException into CacheClosedException - when DiskInitFile is in closed state and DiskStoreImpl is closed or closing - catch DiskAccessException in PRHARedundancyProvider and turn into CacheClosedException if cache closing is in progress - change CreateBucketMessage to handle DiskAccessException as cause of ReplyException (cherry picked from commit a98197b)
…e#7334) * GEODE-9990: turn DiskAccessException into CacheClosedException - when DiskInitFile is in closed state and DiskStoreImpl is closed or closing - catch DiskAccessException in PRHARedundancyProvider and turn into CacheClosedException if cache closing is in progress - change CreateBucketMessage to handle DiskAccessException as cause of ReplyException (cherry picked from commit a98197b)
#7379) * GEODE-9990: turn DiskAccessException into CacheClosedException - when DiskInitFile is in closed state and DiskStoreImpl is closed or closing - catch DiskAccessException in PRHARedundancyProvider and turn into CacheClosedException if cache closing is in progress - change CreateBucketMessage to handle DiskAccessException as cause of ReplyException (cherry picked from commit a98197b)
…e#7334) * GEODE-9990: turn DiskAccessException into CacheClosedException - when DiskInitFile is in closed state and DiskStoreImpl is closed or closing - catch DiskAccessException in PRHARedundancyProvider and turn into CacheClosedException if cache closing is in progress - change CreateBucketMessage to handle DiskAccessException as cause of ReplyException
9 participants
closing
CacheClosedException if cache closing is in progress
ReplyException