From 3801eee0361efeb128abcf5bf75133fbf664370e Mon Sep 17 00:00:00 2001 From: Todd Lipcon Date: Fri, 20 Jan 2012 07:25:59 +0000 Subject: [PATCH] HDFS-2810. Leases not getting renewed properly by clients. Contributed by Todd Lipcon. git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.23@1233793 13f79535-47bb-0310-9956-ffa450edef68 --- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt | 2 + .../org/apache/hadoop/hdfs/DFSClient.java | 10 ++- .../org/apache/hadoop/hdfs/LeaseRenewer.java | 10 ++- .../apache/hadoop/hdfs/TestLeaseRenewer.java | 81 ++++++++++++++++--- 4 files changed, 89 insertions(+), 14 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt index 2a67ebeba21..b75b7ac8419 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -159,6 +159,8 @@ Release 0.23.1 - UNRELEASED HDFS-2790. FSNamesystem.setTimes throws exception with wrong configuration name in the message. (Arpit Gupta via eli) + HDFS-2810. Leases not getting renewed properly by clients (todd) + Release 0.23.0 - 2011-11-01 INCOMPATIBLE CHANGES diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java index 53095b2ef05..6265d02e7a4 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java @@ -374,11 +374,17 @@ public class DFSClient implements java.io.Closeable { return clientRunning; } - /** Renew leases */ - void renewLease() throws IOException { + /** + * Renew leases. + * @return true if lease was renewed. May return false if this + * client has been closed or has no files open. + **/ + boolean renewLease() throws IOException { if (clientRunning && !isFilesBeingWrittenEmpty()) { namenode.renewLease(clientName); + return true; } + return false; } /** Abort and release resources held. Ignore all errors. */ diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/LeaseRenewer.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/LeaseRenewer.java index 14b9c9a3b72..862be0c184d 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/LeaseRenewer.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/LeaseRenewer.java @@ -67,7 +67,7 @@ import org.apache.hadoop.util.StringUtils; *

*/ class LeaseRenewer { - private static final Log LOG = LogFactory.getLog(LeaseRenewer.class); + static final Log LOG = LogFactory.getLog(LeaseRenewer.class); static final long LEASE_RENEWER_GRACE_DEFAULT = 60*1000L; static final long LEASE_RENEWER_SLEEP_DEFAULT = 1000L; @@ -407,7 +407,13 @@ class LeaseRenewer { final DFSClient c = copies.get(i); //skip if current client name is the same as the previous name. if (!c.getClientName().equals(previousName)) { - c.renewLease(); + if (!c.renewLease()) { + if (LOG.isDebugEnabled()) { + LOG.debug("Did not renew lease for client " + + c); + } + continue; + } previousName = c.getClientName(); if (LOG.isDebugEnabled()) { LOG.debug("Lease renewed for client " + previousName); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRenewer.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRenewer.java index f3817671b07..1bdb4979274 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRenewer.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRenewer.java @@ -17,11 +17,14 @@ */ package org.apache.hadoop.hdfs; +import static org.junit.Assert.*; + import java.io.IOException; import java.util.concurrent.atomic.AtomicInteger; import org.apache.hadoop.security.UserGroupInformation; +import org.apache.hadoop.test.GenericTestUtils; import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -29,6 +32,8 @@ import org.mockito.Mockito; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; +import com.google.common.base.Supplier; + public class TestLeaseRenewer { private String FAKE_AUTHORITY="hdfs://nn1/"; private UserGroupInformation FAKE_UGI_A = @@ -46,19 +51,24 @@ public class TestLeaseRenewer { @Before public void setupMocksAndRenewer() throws IOException { - MOCK_DFSCLIENT = Mockito.mock(DFSClient.class); - Mockito.doReturn(true) - .when(MOCK_DFSCLIENT).isClientRunning(); - Mockito.doReturn((int)FAST_GRACE_PERIOD) - .when(MOCK_DFSCLIENT).getHdfsTimeout(); - Mockito.doReturn("myclient") - .when(MOCK_DFSCLIENT).getClientName(); + MOCK_DFSCLIENT = createMockClient(); renewer = LeaseRenewer.getInstance( FAKE_AUTHORITY, FAKE_UGI_A, MOCK_DFSCLIENT); renewer.setGraceSleepPeriod(FAST_GRACE_PERIOD); } + private DFSClient createMockClient() { + DFSClient mock = Mockito.mock(DFSClient.class); + Mockito.doReturn(true) + .when(mock).isClientRunning(); + Mockito.doReturn((int)FAST_GRACE_PERIOD) + .when(mock).getHdfsTimeout(); + Mockito.doReturn("myclient") + .when(mock).getClientName(); + return mock; + } + @Test public void testInstanceSharing() throws IOException { // Two lease renewers with the same UGI should return @@ -93,11 +103,11 @@ public class TestLeaseRenewer { public void testRenewal() throws Exception { // Keep track of how many times the lease gets renewed final AtomicInteger leaseRenewalCount = new AtomicInteger(); - Mockito.doAnswer(new Answer() { + Mockito.doAnswer(new Answer() { @Override - public Void answer(InvocationOnMock invocation) throws Throwable { + public Boolean answer(InvocationOnMock invocation) throws Throwable { leaseRenewalCount.incrementAndGet(); - return null; + return true; } }).when(MOCK_DFSCLIENT).renewLease(); @@ -120,6 +130,57 @@ public class TestLeaseRenewer { renewer.closeFile(filePath, MOCK_DFSCLIENT); } + /** + * Regression test for HDFS-2810. In this bug, the LeaseRenewer has handles + * to several DFSClients with the same name, the first of which has no files + * open. Previously, this was causing the lease to not get renewed. + */ + @Test + public void testManyDfsClientsWhereSomeNotOpen() throws Exception { + // First DFSClient has no files open so doesn't renew leases. + final DFSClient mockClient1 = createMockClient(); + Mockito.doReturn(false).when(mockClient1).renewLease(); + assertSame(renewer, LeaseRenewer.getInstance( + FAKE_AUTHORITY, FAKE_UGI_A, mockClient1)); + + // Set up a file so that we start renewing our lease. + DFSOutputStream mockStream1 = Mockito.mock(DFSOutputStream.class); + String filePath = "/foo"; + renewer.put(filePath, mockStream1, mockClient1); + + // Second DFSClient does renew lease + final DFSClient mockClient2 = createMockClient(); + Mockito.doReturn(true).when(mockClient2).renewLease(); + assertSame(renewer, LeaseRenewer.getInstance( + FAKE_AUTHORITY, FAKE_UGI_A, mockClient2)); + + // Set up a file so that we start renewing our lease. + DFSOutputStream mockStream2 = Mockito.mock(DFSOutputStream.class); + renewer.put(filePath, mockStream2, mockClient2); + + + // Wait for lease to get renewed + GenericTestUtils.waitFor(new Supplier() { + @Override + public Boolean get() { + try { + Mockito.verify(mockClient1, Mockito.atLeastOnce()).renewLease(); + Mockito.verify(mockClient2, Mockito.atLeastOnce()).renewLease(); + return true; + } catch (AssertionError err) { + LeaseRenewer.LOG.warn("Not yet satisfied", err); + return false; + } catch (IOException e) { + // should not throw! + throw new RuntimeException(e); + } + } + }, 100, 10000); + + renewer.closeFile(filePath, mockClient1); + renewer.closeFile(filePath, mockClient2); + } + @Test public void testThreadName() throws Exception { DFSOutputStream mockStream = Mockito.mock(DFSOutputStream.class);