[
https://issues.apache.org/jira/browse/HBASE-12454?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14208684#comment-14208684
]
Sergey Shelukhin edited comment on HBASE-12454 at 11/12/14 9:09 PM:
--------------------------------------------------------------------
Some clarification. Variable name is indeed not good... however I think it is
right to be set before compact call.
"Physically", the end goal is to call finishCompactionRequest.
If compact is called at all on HStore, it would call finishCompactionRequest in
the finally block (the code above try... finally is not supposed to fail, I
guess it could also be included in a separate try... finally). If it wasn't
called, then finishCompactionRequest is called in case of failure, via
cancelRequestedCompaction
"Logically", we have to cancel compactions that we selected but didn't start;
if we start a compaction on the store we assume it does appropriate cleanup for
failure, so we don't need to cancel it. Perhaps comments could be added to that
effect and variable renamed "didStartCompaction"
Moving it after the call seems incorrect cause in that case
finishCompactionRequest can be called twice, in the finally in HStore::compact,
and via cancel....
was (Author: sershe):
Some clarification. Variable name is indeed not good... however I think it is
right to be set before compact call.
"Physically", the end goal is to call finishCompactionRequest.
If compact is called at all on HStore, it would call finishCompactionRequest in
a finally block (the code above try... finally is not supposed to fail, I guess
it could also be included in a separate try... finally). If it wasn't called,
then call finishCompactionRequest is called in case of failure via
cancelRequestedCompaction
"Logically", we have to cancel compactions that we selected but didn't start;
if we start a compaction on the store we assume it does appropriate cleanup for
failure, so we don't need to cancel it. Perhaps comments could be added to that
effect and variable renamed "didStartCompaction"
Moving it after the call seems incorrect cause in that case cancel (and thus
finishCompactionRequest) can be called twice.
> Setting didPerformCompaction early in HRegion#compact
> -----------------------------------------------------
>
> Key: HBASE-12454
> URL: https://issues.apache.org/jira/browse/HBASE-12454
> Project: HBase
> Issue Type: Bug
> Affects Versions: 0.98.8
> Reporter: Andrew Purtell
> Assignee: Andrew Purtell
> Priority: Minor
> Fix For: 2.0.0, 0.98.9, 0.99.2
>
> Attachments: HBASE-12454-0.98.patch, HBASE-12454.patch,
> HBASE-12454.patch, HBASE-12454.patch
>
>
> It appears we are setting 'didPerformCompaction' to "true" before attempting
> the compaction in HRegion#compact. If Store#compact throws an exception or is
> interrupted, we won't call Store#cancelRequestedCompaction in the last
> finally block of the method as it looks like we should.
> {code}
> ---
> a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
> +++
> b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
> @@ -1540,58 +1540,58 @@ public class HRegion implements HeapSize,
> PropagatingConfigurationObserver {
> return false;
> }
>
> status = TaskMonitor.get().createStatus("Compacting " + store + " in "
> + this);
> if (this.closed.get()) {
> String msg = "Skipping compaction on " + this + " because closed";
> LOG.debug(msg);
> status.abort(msg);
> return false;
> }
> boolean wasStateSet = false;
> try {
> synchronized (writestate) {
> if (writestate.writesEnabled) {
> wasStateSet = true;
> ++writestate.compacting;
> } else {
> String msg = "NOT compacting region " + this + ". Writes
> disabled.";
> LOG.info(msg);
> status.abort(msg);
> return false;
> }
> }
> LOG.info("Starting compaction on " + store + " in region " + this
> + (compaction.getRequest().isOffPeak()?" as an off-peak
> compaction":""));
> doRegionCompactionPrep();
> try {
> status.setStatus("Compacting store " + store);
> - didPerformCompaction = true;
> store.compact(compaction);
> + didPerformCompaction = true;
> } catch (InterruptedIOException iioe) {
> String msg = "compaction interrupted";
> LOG.info(msg, iioe);
> status.abort(msg);
> return false;
> }
> } finally {
> if (wasStateSet) {
> synchronized (writestate) {
> --writestate.compacting;
> if (writestate.compacting <= 0) {
> writestate.notifyAll();
> }
> }
> }
> }
> status.markComplete("Compaction complete");
> return true;
> } finally {
> try {
> if (!didPerformCompaction)
> store.cancelRequestedCompaction(compaction); <-----
> if (status != null) status.cleanup();
> } finally {
> lock.readLock().unlock();
> }
> }
> }{code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
