Repository: phoenix Updated Branches: refs/heads/4.x-HBase-0.98 b43dc09f0 -> ed9acd505
PHOENIX-4131 UngroupedAggregateRegionObserver.preClose() and doPostScannerOpen() can deadlock Project: http://git-wip-us.apache.org/repos/asf/phoenix/repo Commit: http://git-wip-us.apache.org/repos/asf/phoenix/commit/ed9acd50 Tree: http://git-wip-us.apache.org/repos/asf/phoenix/tree/ed9acd50 Diff: http://git-wip-us.apache.org/repos/asf/phoenix/diff/ed9acd50 Branch: refs/heads/4.x-HBase-0.98 Commit: ed9acd5051a63b439f651d89fd7c95b490d62e37 Parents: b43dc09 Author: Samarth Jain <[email protected]> Authored: Thu Aug 31 17:23:11 2017 -0700 Committer: Samarth Jain <[email protected]> Committed: Thu Aug 31 17:23:11 2017 -0700 ---------------------------------------------------------------------- .../coprocessor/MetaDataEndpointImpl.java | 44 ++++---------------- .../UngroupedAggregateRegionObserver.java | 35 ++++++++++------ .../java/org/apache/phoenix/query/BaseTest.java | 7 ---- 3 files changed, 32 insertions(+), 54 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/phoenix/blob/ed9acd50/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java index dfdc28d..85cc706 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java @@ -555,10 +555,8 @@ public class MetaDataEndpointImpl extends MetaDataProtocol implements Coprocesso private PTable buildTable(byte[] key, ImmutableBytesPtr cacheKey, HRegion region, long clientTimeStamp) throws IOException, SQLException { Scan scan = MetaDataUtil.newTableRowsScan(key, MIN_TABLE_TIMESTAMP, clientTimeStamp); - RegionScanner scanner = region.getScanner(scan); - Cache<ImmutableBytesPtr,PMetaDataEntity> metaDataCache = GlobalCache.getInstance(this.env).getMetaDataCache(); - try { + try (RegionScanner scanner = region.getScanner(scan)) { PTable oldTable = (PTable)metaDataCache.getIfPresent(cacheKey); long tableTimeStamp = oldTable == null ? MIN_TABLE_TIMESTAMP-1 : oldTable.getTimeStamp(); PTable newTable; @@ -580,8 +578,6 @@ public class MetaDataEndpointImpl extends MetaDataProtocol implements Coprocesso metaDataCache.put(cacheKey, newTable); } return newTable; - } finally { - scanner.close(); } } @@ -598,13 +594,10 @@ public class MetaDataEndpointImpl extends MetaDataProtocol implements Coprocesso ScanRanges scanRanges = ScanRanges.createPointLookup(keyRanges); scanRanges.initializeScan(scan); scan.setFilter(scanRanges.getSkipScanFilter()); - - RegionScanner scanner = region.getScanner(scan); - Cache<ImmutableBytesPtr,PMetaDataEntity> metaDataCache = GlobalCache.getInstance(this.env).getMetaDataCache(); List<PFunction> functions = new ArrayList<PFunction>(); PFunction function = null; - try { + try (RegionScanner scanner = region.getScanner(scan)) { for(int i = 0; i< keys.size(); i++) { function = null; function = @@ -621,8 +614,6 @@ public class MetaDataEndpointImpl extends MetaDataProtocol implements Coprocesso functions.add(function); } return functions; - } finally { - scanner.close(); } } @@ -639,13 +630,10 @@ public class MetaDataEndpointImpl extends MetaDataProtocol implements Coprocesso ScanRanges scanRanges = ScanRanges.createPointLookup(keyRanges); scanRanges.initializeScan(scan); scan.setFilter(scanRanges.getSkipScanFilter()); - - RegionScanner scanner = region.getScanner(scan); - Cache<ImmutableBytesPtr, PMetaDataEntity> metaDataCache = GlobalCache.getInstance(this.env).getMetaDataCache(); List<PSchema> schemas = new ArrayList<PSchema>(); PSchema schema = null; - try { + try (RegionScanner scanner = region.getScanner(scan)) { for (int i = 0; i < keys.size(); i++) { schema = null; schema = getSchema(scanner, clientTimeStamp); @@ -654,8 +642,6 @@ public class MetaDataEndpointImpl extends MetaDataProtocol implements Coprocesso schemas.add(schema); } return schemas; - } finally { - scanner.close(); } } @@ -1704,14 +1690,12 @@ public class MetaDataEndpointImpl extends MetaDataProtocol implements Coprocesso // TableName systemCatalogTableName = region.getTableDesc().getTableName(); // HTableInterface hTable = env.getTable(systemCatalogTableName); // These deprecated calls work around the issue - HTableInterface hTable = ServerUtil.getHTableForCoprocessorScan(env, - region.getTableDesc().getTableName().getName()); - try { + try (HTableInterface hTable = ServerUtil.getHTableForCoprocessorScan(env, + region.getTableDesc().getTableName().getName())) { boolean allViewsInCurrentRegion = true; int numOfChildViews = 0; List<ViewInfo> viewInfoList = Lists.newArrayList(); - ResultScanner scanner = hTable.getScanner(scan); - try { + try (ResultScanner scanner = hTable.getScanner(scan)) { for (Result result = scanner.next(); (result != null); result = scanner.next()) { numOfChildViews++; ImmutableBytesWritable ptr = new ImmutableBytesWritable(); @@ -1733,11 +1717,7 @@ public class MetaDataEndpointImpl extends MetaDataProtocol implements Coprocesso tableViewFinderResult.setAllViewsNotInSingleRegion(); } return tableViewFinderResult; - } finally { - scanner.close(); } - } finally { - hTable.close(); } } @@ -1759,14 +1739,12 @@ public class MetaDataEndpointImpl extends MetaDataProtocol implements Coprocesso // TableName systemCatalogTableName = region.getTableDesc().getTableName(); // HTableInterface hTable = env.getTable(systemCatalogTableName); // These deprecated calls work around the issue - HTableInterface hTable = ServerUtil.getHTableForCoprocessorScan(env, - region.getTableDesc().getTableName().getName()); - try { + try (HTableInterface hTable = ServerUtil.getHTableForCoprocessorScan(env, + region.getTableDesc().getTableName().getName())) { boolean allViewsInCurrentRegion = true; int numOfChildViews = 0; List<ViewInfo> viewInfoList = Lists.newArrayList(); - ResultScanner scanner = hTable.getScanner(scan); - try { + try (ResultScanner scanner = hTable.getScanner(scan)) { for (Result result = scanner.next(); (result != null); result = scanner.next()) { numOfChildViews++; ImmutableBytesWritable ptr = new ImmutableBytesWritable(); @@ -1788,11 +1766,7 @@ public class MetaDataEndpointImpl extends MetaDataProtocol implements Coprocesso tableViewFinderResult.setAllViewsNotInSingleRegion(); } return tableViewFinderResult; - } finally { - scanner.close(); } - } finally { - hTable.close(); } } http://git-wip-us.apache.org/repos/asf/phoenix/blob/ed9acd50/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java index 2c2b18e..8fbcde6 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java @@ -478,6 +478,7 @@ public class UngroupedAggregateRegionObserver extends BaseScannerRegionObserver if(needToWrite) { synchronized (lock) { scansReferenceCount++; + lock.notifyAll(); } } region.startRegionOperation(); @@ -729,18 +730,27 @@ public class UngroupedAggregateRegionObserver extends BaseScannerRegionObserver } } } finally { - if(needToWrite) { + if (needToWrite) { synchronized (lock) { scansReferenceCount--; + if (scansReferenceCount < 0) { + logger.warn( + "Scan reference count went below zero. Something isn't correct. Resetting it back to zero"); + scansReferenceCount = 0; + } + lock.notifyAll(); } } - if (targetHTable != null) { - targetHTable.close(); - } try { - innerScanner.close(); + if (targetHTable != null) { + targetHTable.close(); + } } finally { - if (acquiredLock) region.closeRegionOperation(); + try { + innerScanner.close(); + } finally { + if (acquiredLock) region.closeRegionOperation(); + } } } if (logger.isDebugEnabled()) { @@ -957,7 +967,7 @@ public class UngroupedAggregateRegionObserver extends BaseScannerRegionObserver @Override public void close() throws IOException { - // no-op because we want to manage closing of the inner scanner ourselves. + innerScanner.close(); } @Override @@ -1015,7 +1025,8 @@ public class UngroupedAggregateRegionObserver extends BaseScannerRegionObserver @Override public void close() throws IOException { - // no-op because we want to manage closing of the inner scanner ourselves. + // No-op because we want to manage closing of the inner scanner ourselves. + // This happens inside StatsCollectionCallable. } @Override @@ -1175,7 +1186,7 @@ public class UngroupedAggregateRegionObserver extends BaseScannerRegionObserver // Don't allow splitting if operations need read and write to same region are going on in the // the coprocessors to avoid dead lock scenario. See PHOENIX-3111. synchronized (lock) { - if (scansReferenceCount != 0) { + if (scansReferenceCount > 0) { throw new IOException("Operations like local index building/delete/upsert select" + " might be going on so not allowing to split."); } @@ -1188,7 +1199,7 @@ public class UngroupedAggregateRegionObserver extends BaseScannerRegionObserver // Don't allow bulkload if operations need read and write to same region are going on in the // the coprocessors to avoid dead lock scenario. See PHOENIX-3111. synchronized (lock) { - if (scansReferenceCount != 0) { + if (scansReferenceCount > 0) { throw new DoNotRetryIOException("Operations like local index building/delete/upsert select" + " might be going on so not allowing to bulkload."); } @@ -1199,8 +1210,8 @@ public class UngroupedAggregateRegionObserver extends BaseScannerRegionObserver public void preClose(ObserverContext<RegionCoprocessorEnvironment> c, boolean abortRequested) throws IOException { synchronized (lock) { - while (scansReferenceCount != 0) { - isRegionClosing = true; + isRegionClosing = true; + while (scansReferenceCount > 0) { try { lock.wait(1000); } catch (InterruptedException e) { http://git-wip-us.apache.org/repos/asf/phoenix/blob/ed9acd50/phoenix-core/src/test/java/org/apache/phoenix/query/BaseTest.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/test/java/org/apache/phoenix/query/BaseTest.java b/phoenix-core/src/test/java/org/apache/phoenix/query/BaseTest.java index 9ba9607..8e5ecdf 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/query/BaseTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/query/BaseTest.java @@ -423,13 +423,6 @@ public abstract class BaseTest { if (!clusterInitialized) { url = setUpTestCluster(config, serverProps); clusterInitialized = true; - Runtime.getRuntime().addShutdownHook(new Thread() { - @Override - public void run() { - logger.info("SHUTDOWN: halting JVM now"); - Runtime.getRuntime().halt(0); - } - }); } return url; }
