[
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:07 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
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.
was (Author: sershe):
Some clarification. Variable name is indeed not good... however I think it is
right to be set before compact call.
The end goal is to call finishCompactionRequest.
If compact is called at all on HStore, it would do so 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 it is called
in case of failure via cancelRequestedCompaction
The logical explanation is that we have to cancel compactions that we selected
but didn't start; if we start compaction on the store we assume it does
appropriate cleanup. 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)
