[
https://issues.apache.org/jira/browse/HBASE-12454?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14206680#comment-14206680
]
Lars Hofhansl commented on HBASE-12454:
---------------------------------------
So thinking about this more. All that cancelRequestedCompaction does is
cleaning up filesToCompact. So it is actually safest to indicate that we need
to do this before we trigger the compaction. Otherwise the compaction sets that
member, and then might fail. So I think it is correct as is. (one can argue
about the naming and whether an extra state variable is needed here, but the
logic seems to be sound)
> 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
> Fix For: 2.0.0, 0.98.8, 0.99.2
>
> Attachments: 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)
