From 791659b27efdbfc3b9d6785f026856e1ea8047aa Mon Sep 17 00:00:00 2001 From: Michael Stack Date: Tue, 21 Jun 2011 20:56:14 +0000 Subject: [PATCH] HBASE-2077 NullPointerException with an open scanner that expired causing an immediate region server shutdown -- part 2 git-svn-id: https://svn.apache.org/repos/asf/hbase/trunk@1138180 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES.txt | 2 + .../hbase/master/AssignmentManager.java | 2 +- .../hbase/regionserver/HRegionServer.java | 42 ++++++++------ .../hadoop/hbase/regionserver/Leases.java | 57 ++++++++++++------- 4 files changed, 64 insertions(+), 39 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 0cc3f27b596..d2acf053e50 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -343,6 +343,8 @@ Release 0.90.4 - Unreleased (Li Pi) HBASE-3988 Infinite loop for secondary master (Liyin Tang) HBASE-3995 HBASE-3946 broke TestMasterFailover + HBASE-2077 NullPointerException with an open scanner that expired causing + an immediate region server shutdown -- part 2. IMPROVEMENT diff --git a/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java b/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java index 3fe592530ef..a6f37572996 100644 --- a/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java +++ b/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java @@ -1783,7 +1783,7 @@ public class AssignmentManager extends ZooKeeperListener { Result result = region.getSecond(); // If region was in transition (was in zk) force it offline for reassign try { - // Process with existing RS shutdown code + // Process with existing RS shutdown code boolean assign = ServerShutdownHandler.processDeadRegion(regionInfo, result, this, this.catalogTracker); diff --git a/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java b/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java index d1479a2ebc7..bbfdcbe8fdc 100644 --- a/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java +++ b/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java @@ -1943,26 +1943,27 @@ public class HRegionServer implements HRegionInterface, HBaseRPCErrorHandler, } public Result[] next(final long scannerId, int nbRows) throws IOException { + String scannerName = String.valueOf(scannerId); + InternalScanner s = this.scanners.get(scannerName); + if (s == null) throw new UnknownScannerException("Name: " + scannerName); try { - String scannerName = String.valueOf(scannerId); - InternalScanner s = this.scanners.get(scannerName); - if (s == null) { - throw new UnknownScannerException("Name: " + scannerName); - } + checkOpen(); + } catch (IOException e) { + // If checkOpen failed, server not running or filesystem gone, + // cancel this lease; filesystem is gone or we're closing or something. try { - checkOpen(); - } catch (IOException e) { - // If checkOpen failed, server not running or filesystem gone, - // cancel this lease; filesystem is gone or we're closing or something. - try { - this.leases.cancelLease(scannerName); - } catch (LeaseException le) { - LOG.info("Server shutting down and client tried to access missing scanner " + - scannerName); - } - throw e; + this.leases.cancelLease(scannerName); + } catch (LeaseException le) { + LOG.info("Server shutting down and client tried to access missing scanner " + + scannerName); } - this.leases.renewLease(scannerName); + throw e; + } + Leases.Lease lease = null; + try { + // Remove lease while its being processed in server; protects against case + // where processing of request takes > lease expiration time. + lease = this.leases.removeLease(scannerName); List results = new ArrayList(nbRows); long currentScanResultSize = 0; List values = new ArrayList(); @@ -2024,10 +2025,15 @@ public class HRegionServer implements HRegionInterface, HBaseRPCErrorHandler, : results.toArray(new Result[0]); } catch (Throwable t) { if (t instanceof NotServingRegionException) { - String scannerName = String.valueOf(scannerId); this.scanners.remove(scannerName); } throw convertThrowableToIOE(cleanup(t)); + } finally { + // We're done. On way out readd the above removed lease. Adding resets + // expiration time on lease. + if (this.scanners.containsKey(scannerName)) { + if (lease != null) this.leases.addLease(lease); + } } } diff --git a/src/main/java/org/apache/hadoop/hbase/regionserver/Leases.java b/src/main/java/org/apache/hadoop/hbase/regionserver/Leases.java index 5a4b275f40d..8756efb222a 100644 --- a/src/main/java/org/apache/hadoop/hbase/regionserver/Leases.java +++ b/src/main/java/org/apache/hadoop/hbase/regionserver/Leases.java @@ -140,16 +140,24 @@ public class Leases extends Thread { */ public void createLease(String leaseName, final LeaseListener listener) throws LeaseStillHeldException { - if (stopRequested) { + addLease(new Lease(leaseName, listener)); + } + + /** + * Inserts lease. Resets expiration before insertion. + * @param lease + * @throws LeaseStillHeldException + */ + public void addLease(final Lease lease) throws LeaseStillHeldException { + if (this.stopRequested) { return; } - Lease lease = new Lease(leaseName, listener, - System.currentTimeMillis() + leasePeriod); + lease.setExpirationTime(System.currentTimeMillis() + this.leasePeriod); synchronized (leaseQueue) { - if (leases.containsKey(leaseName)) { - throw new LeaseStillHeldException(leaseName); + if (leases.containsKey(lease.getLeaseName())) { + throw new LeaseStillHeldException(lease.getLeaseName()); } - leases.put(leaseName, lease); + leases.put(lease.getLeaseName(), lease); leaseQueue.add(lease); } } @@ -189,7 +197,7 @@ public class Leases extends Thread { // in a corrupt leaseQueue. if (lease == null || !leaseQueue.remove(lease)) { throw new LeaseException("lease '" + leaseName + - "' does not exist or has already expired"); + "' does not exist or has already expired"); } lease.setExpirationTime(System.currentTimeMillis() + leasePeriod); leaseQueue.add(lease); @@ -198,26 +206,44 @@ public class Leases extends Thread { /** * Client explicitly cancels a lease. - * * @param leaseName name of lease * @throws LeaseException */ public void cancelLease(final String leaseName) throws LeaseException { + removeLease(leaseName); + } + + /** + * Remove named lease. + * Lease is removed from the list of leases and removed from the delay queue. + * Lease can be resinserted using {@link #addLease(Lease)} + * + * @param leaseName name of lease + * @throws LeaseException + * @return Removed lease + */ + Lease removeLease(final String leaseName) throws LeaseException { + Lease lease = null; synchronized (leaseQueue) { - Lease lease = leases.remove(leaseName); + lease = leases.remove(leaseName); if (lease == null) { throw new LeaseException("lease '" + leaseName + "' does not exist"); } leaseQueue.remove(lease); } + return lease; } /** This class tracks a single Lease. */ - private static class Lease implements Delayed { + static class Lease implements Delayed { private final String leaseName; private final LeaseListener listener; private long expirationTime; + Lease(final String leaseName, LeaseListener listener) { + this(leaseName, listener, 0); + } + Lease(final String leaseName, LeaseListener listener, long expirationTime) { this.leaseName = leaseName; this.listener = listener; @@ -269,14 +295,5 @@ public class Leases extends Thread { public void setExpirationTime(long expirationTime) { this.expirationTime = expirationTime; } - - /** - * Get the expiration time for that lease - * @return expiration time - */ - public long getExpirationTime() { - return this.expirationTime; - } - } -} +} \ No newline at end of file