From 5b685be174a56ae947ca5d5b36760d7360f76b2c Mon Sep 17 00:00:00 2001 From: Peter Somogyi Date: Tue, 29 Aug 2017 17:36:32 +0200 Subject: [PATCH] HBASE-18665 ReversedScannerCallable invokes getRegionLocations incorrectly The way how ReversedScannerCallable#prepare called getRegionLocations was faulty. Calling prepare with force reload used cache and vica versa. --- .../hbase/client/ReversedScannerCallable.java | 8 +- .../client/RpcRetryingCallerWithReadReplicas.java | 6 +- .../hbase/client/TestReversedScannerCallable.java | 94 ++++++++++++++++++++++ .../org/apache/hadoop/hbase/fs/HFileSystem.java | 2 +- 4 files changed, 102 insertions(+), 8 deletions(-) create mode 100644 hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestReversedScannerCallable.java diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ReversedScannerCallable.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ReversedScannerCallable.java index e169f7a018..8bd5865d3a 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ReversedScannerCallable.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ReversedScannerCallable.java @@ -102,8 +102,8 @@ public class ReversedScannerCallable extends ScannerCallable { if (!instantiated || reload) { if (locateStartRow == null) { // Just locate the region with the row - RegionLocations rl = RpcRetryingCallerWithReadReplicas.getRegionLocations(reload, id, - getConnection(), tableName, row); + RegionLocations rl = RpcRetryingCallerWithReadReplicas.getRegionLocations(!reload, id, + getConnection(), getTableName(), getRow()); this.location = id < rl.size() ? rl.getRegionLocation(id) : null; if (this.location == null) { throw new IOException("Failed to find location, tableName=" @@ -162,8 +162,8 @@ public class ReversedScannerCallable extends ScannerCallable { List regionList = new ArrayList(); byte[] currentKey = startKey; do { - RegionLocations rl = RpcRetryingCallerWithReadReplicas.getRegionLocations(reload, id, - getConnection(), tableName, currentKey); + RegionLocations rl = RpcRetryingCallerWithReadReplicas.getRegionLocations(!reload, id, + getConnection(), getTableName(), currentKey); HRegionLocation regionLocation = id < rl.size() ? rl.getRegionLocation(id) : null; if (regionLocation != null && regionLocation.getRegionInfo().containsRow(currentKey)) { regionList.add(regionLocation); diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RpcRetryingCallerWithReadReplicas.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RpcRetryingCallerWithReadReplicas.java index 80c187c37d..688a37c641 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RpcRetryingCallerWithReadReplicas.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RpcRetryingCallerWithReadReplicas.java @@ -299,10 +299,10 @@ public class RpcRetryingCallerWithReadReplicas { RegionLocations rl; try { - if (!useCache) { - rl = cConnection.relocateRegion(tableName, row, replicaId); + if (useCache) { + rl = cConnection.locateRegion(tableName, row, true, true, replicaId); } else { - rl = cConnection.locateRegion(tableName, row, useCache, true, replicaId); + rl = cConnection.relocateRegion(tableName, row, replicaId); } } catch (DoNotRetryIOException e) { throw e; diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestReversedScannerCallable.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestReversedScannerCallable.java new file mode 100644 index 0000000000..673cc5474b --- /dev/null +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestReversedScannerCallable.java @@ -0,0 +1,94 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.client; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.HRegionInfo; +import org.apache.hadoop.hbase.HRegionLocation; +import org.apache.hadoop.hbase.RegionLocations; +import org.apache.hadoop.hbase.ServerName; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.ipc.RpcControllerFactory; +import org.apache.hadoop.hbase.testclassification.ClientTests; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.junit.Before; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.runners.MockitoJUnitRunner; + +@RunWith(MockitoJUnitRunner.class) +@Category({ ClientTests.class, SmallTests.class }) +public class TestReversedScannerCallable { + + @Mock + private ClusterConnection connection; + @Mock + private Scan scan; + @Mock + private RegionLocations regionLocations; + + private final byte[] ROW = Bytes.toBytes("row1"); + + @Before + public void setUp() throws Exception { + Configuration conf = Mockito.mock(Configuration.class); + HRegionLocation regionLocation = Mockito.mock(HRegionLocation.class); + ServerName serverName = Mockito.mock(ServerName.class); + HRegionInfo regionInfo = Mockito.mock(HRegionInfo.class); + + Mockito.when(connection.getConfiguration()).thenReturn(conf); + Mockito.when(regionLocations.size()).thenReturn(1); + Mockito.when(regionLocations.getRegionLocation(0)).thenReturn(regionLocation); + Mockito.when(regionLocation.getHostname()).thenReturn("localhost"); + Mockito.when(regionLocation.getRegionInfo()).thenReturn(regionInfo); + Mockito.when(regionLocation.getServerName()).thenReturn(serverName); + Mockito.when(regionInfo.containsRow(ROW)).thenReturn(true); + Mockito.when(regionInfo.getEndKey()).thenReturn(HConstants.EMPTY_END_ROW); + Mockito.when(scan.getStartRow()).thenReturn(ROW); + } + + @Test + public void testPrepareDoesNotUseCache() throws Exception { + TableName tableName = TableName.valueOf("MyTable"); + Mockito.when(connection.relocateRegion(tableName, ROW, 0)).thenReturn(regionLocations); + + ReversedScannerCallable callable = + new ReversedScannerCallable(connection, tableName, scan, null, ROW, null, 0); + callable.prepare(true); + + Mockito.verify(connection).relocateRegion(tableName, ROW, 0); + } + + @Test + public void testPrepareUsesCache() throws Exception { + TableName tableName = TableName.valueOf("MyTable"); + Mockito.when(connection.locateRegion(tableName, ROW, true, true, 0)) + .thenReturn(regionLocations); + + ReversedScannerCallable callable = + new ReversedScannerCallable(connection, tableName, scan, null, ROW, null, 0); + callable.prepare(false); + + Mockito.verify(connection).locateRegion(tableName, ROW, true, true, 0); + } +} diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/HFileSystem.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/HFileSystem.java index e77409a76e..9933e19b46 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/HFileSystem.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/HFileSystem.java @@ -1,4 +1,4 @@ -/* +/** * Copyright The Apache Software Foundation * * Licensed to the Apache Software Foundation (ASF) under one -- 2.13.1