From c94c74b5e0f5038d61e5fd45cd7004b7aa036c08 Mon Sep 17 00:00:00 2001 From: Briana Augenreich Date: Fri, 28 Oct 2022 06:55:05 -0400 Subject: [PATCH 1/6] HBASE-27390: prevent NPE when task status is null --- .../org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java index 4fee362a7355..142074d80389 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java @@ -104,6 +104,9 @@ public String getDescription() { @Override public String getStatus() { + if(status == null){ + return "No task status available."; + } return status; } From 4d5c985f74b85a906d6cfd7550c5f486c8ffa8e3 Mon Sep 17 00:00:00 2001 From: Briana Augenreich Date: Tue, 1 Nov 2022 10:29:25 -0400 Subject: [PATCH 2/6] spotless apply --- .../org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java index 142074d80389..622ff4165e0a 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java @@ -104,7 +104,7 @@ public String getDescription() { @Override public String getStatus() { - if(status == null){ + if (status == null) { return "No task status available."; } return status; From ce5c8bad0241ce5fdb57b12052df4fb61557e10b Mon Sep 17 00:00:00 2001 From: Briana Augenreich Date: Mon, 5 Dec 2022 15:25:37 -0500 Subject: [PATCH 3/6] PR feedback - simply prevent null values --- .../hbase/monitoring/MonitoredRPCHandlerImpl.java | 4 ++-- .../hadoop/hbase/monitoring/MonitoredTaskImpl.java | 10 ++++++---- .../apache/hadoop/hbase/monitoring/TaskMonitor.java | 6 ++---- .../org/apache/hadoop/hbase/ipc/TestCallRunner.java | 8 ++++---- .../apache/hadoop/hbase/ipc/TestFifoRpcScheduler.java | 2 +- .../hadoop/hbase/ipc/TestSimpleRpcScheduler.java | 8 ++++---- .../hadoop/hbase/monitoring/TestTaskMonitor.java | 2 +- 7 files changed, 20 insertions(+), 20 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java index 154f3b2e357e..81635fa9f1fc 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java @@ -44,8 +44,8 @@ public class MonitoredRPCHandlerImpl extends MonitoredTaskImpl implements Monito private boolean snapshot = false; private Map callInfoMap = new HashMap<>(); - public MonitoredRPCHandlerImpl() { - super(false); + public MonitoredRPCHandlerImpl(String description) { + super(false, description); // in this implementation, WAITING indicates that the handler is not // actively servicing an RPC call. setState(State.WAITING); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java index 622ff4165e0a..4d52908220bb 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java @@ -17,6 +17,7 @@ */ package org.apache.hadoop.hbase.monitoring; +import com.google.common.base.Preconditions; import java.io.IOException; import java.util.Collections; import java.util.HashMap; @@ -46,11 +47,13 @@ class MonitoredTaskImpl implements MonitoredTask { private static final Gson GSON = GsonUtil.createGson().create(); - public MonitoredTaskImpl(boolean enableJournal) { + public MonitoredTaskImpl(boolean enableJournal, String description) { startTime = EnvironmentEdgeManager.currentTime(); statusTime = startTime; stateTime = startTime; warnTime = startTime; + this.description = description; + this.status = "status unset"; if (enableJournal) { journal = new ConcurrentLinkedQueue<>(); } else { @@ -104,9 +107,6 @@ public String getDescription() { @Override public String getStatus() { - if (status == null) { - return "No task status available."; - } return status; } @@ -164,6 +164,7 @@ public void abort(String msg) { @Override public void setStatus(String status) { + Preconditions.checkNotNull(status, "Status is null"); this.status = status; statusTime = EnvironmentEdgeManager.currentTime(); if (journal != null) { @@ -178,6 +179,7 @@ protected void setState(State state) { @Override public void setDescription(String description) { + Preconditions.checkNotNull(description, "Description is null"); this.description = description; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/TaskMonitor.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/TaskMonitor.java index eff149239ed2..19f1354faa97 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/TaskMonitor.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/TaskMonitor.java @@ -94,8 +94,7 @@ public MonitoredTask createStatus(String description, boolean ignore) { public synchronized MonitoredTask createStatus(String description, boolean ignore, boolean enableJournal) { - MonitoredTask stat = new MonitoredTaskImpl(enableJournal); - stat.setDescription(description); + MonitoredTask stat = new MonitoredTaskImpl(enableJournal, description); MonitoredTask proxy = (MonitoredTask) Proxy.newProxyInstance(stat.getClass().getClassLoader(), new Class[] { MonitoredTask.class }, new PassthroughInvocationHandler<>(stat)); TaskAndWeakRefPair pair = new TaskAndWeakRefPair(stat, proxy); @@ -109,8 +108,7 @@ public synchronized MonitoredTask createStatus(String description, boolean ignor } public synchronized MonitoredRPCHandler createRPCStatus(String description) { - MonitoredRPCHandler stat = new MonitoredRPCHandlerImpl(); - stat.setDescription(description); + MonitoredRPCHandler stat = new MonitoredRPCHandlerImpl(description); MonitoredRPCHandler proxy = (MonitoredRPCHandler) Proxy.newProxyInstance(stat.getClass().getClassLoader(), new Class[] { MonitoredRPCHandler.class }, new PassthroughInvocationHandler<>(stat)); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestCallRunner.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestCallRunner.java index 828bed213a7b..4e8b7fdb6895 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestCallRunner.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestCallRunner.java @@ -81,7 +81,7 @@ public void testSimpleCall() { TraceUtil.trace(() -> { CallRunner cr = new CallRunner(mockRpcServer, mockCall); - cr.setStatus(new MonitoredRPCHandlerImpl()); + cr.setStatus(new MonitoredRPCHandlerImpl("test")); cr.run(); }, testName.getMethodName()); @@ -101,7 +101,7 @@ public void testCallCleanup() { TraceUtil.trace(() -> { CallRunner cr = new CallRunner(mockRpcServer, mockCall); - cr.setStatus(new MonitoredRPCHandlerImpl()); + cr.setStatus(new MonitoredRPCHandlerImpl("test")); cr.run(); }, testName.getMethodName()); Mockito.verify(mockCall, Mockito.times(1)).cleanup(); @@ -116,7 +116,7 @@ public void testCallRunnerDropDisconnected() { TraceUtil.trace(() -> { CallRunner cr = new CallRunner(mockRpcServer, mockCall); - cr.setStatus(new MonitoredRPCHandlerImpl()); + cr.setStatus(new MonitoredRPCHandlerImpl("test")); cr.drop(); }, testName.getMethodName()); Mockito.verify(mockCall, Mockito.times(1)).cleanup(); @@ -142,7 +142,7 @@ public void testCallRunnerDropConnected() { TraceUtil.trace(() -> { CallRunner cr = new CallRunner(mockRpcServer, mockCall); - cr.setStatus(new MonitoredRPCHandlerImpl()); + cr.setStatus(new MonitoredRPCHandlerImpl("test")); cr.drop(); }, testName.getMethodName()); Mockito.verify(mockCall, Mockito.times(1)).cleanup(); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestFifoRpcScheduler.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestFifoRpcScheduler.java index beaf6cd9e738..06b25f81ba92 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestFifoRpcScheduler.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestFifoRpcScheduler.java @@ -113,7 +113,7 @@ public void testCallQueueInfo() throws IOException, InterruptedException { for (int i = totalCallMethods; i > 0; i--) { CallRunner task = createMockTask(); - task.setStatus(new MonitoredRPCHandlerImpl()); + task.setStatus(new MonitoredRPCHandlerImpl("test")); if (!scheduler.dispatch(task)) { unableToDispatch++; diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestSimpleRpcScheduler.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestSimpleRpcScheduler.java index 0c629231728f..19aa46a0d626 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestSimpleRpcScheduler.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestSimpleRpcScheduler.java @@ -109,7 +109,7 @@ public void testBasic() throws IOException, InterruptedException { scheduler.init(CONTEXT); scheduler.start(); CallRunner task = createMockTask(); - task.setStatus(new MonitoredRPCHandlerImpl()); + task.setStatus(new MonitoredRPCHandlerImpl("test")); scheduler.dispatch(task); verify(task, timeout(10000)).run(); scheduler.stop(); @@ -164,7 +164,7 @@ public void testCallQueueInfo() throws IOException, InterruptedException { int totalCallMethods = 10; for (int i = totalCallMethods; i > 0; i--) { CallRunner task = createMockTask(); - task.setStatus(new MonitoredRPCHandlerImpl()); + task.setStatus(new MonitoredRPCHandlerImpl("test")); scheduler.dispatch(task); } @@ -205,7 +205,7 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable { } }; for (CallRunner task : tasks) { - task.setStatus(new MonitoredRPCHandlerImpl()); + task.setStatus(new MonitoredRPCHandlerImpl("test")); doAnswer(answerToRun).when(task).run(); } @@ -524,7 +524,7 @@ public void testScanQueues() throws Exception { private void doAnswerTaskExecution(final CallRunner callTask, final ArrayList results, final int value, final int sleepInterval) { - callTask.setStatus(new MonitoredRPCHandlerImpl()); + callTask.setStatus(new MonitoredRPCHandlerImpl("test")); doAnswer(new Answer() { @Override public Object answer(InvocationOnMock invocation) { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java index ee5ea621eaa2..2e84e91b1561 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java @@ -228,7 +228,7 @@ public void testStatusJournal() { @Test public void testClone() throws Exception { - MonitoredRPCHandlerImpl monitor = new MonitoredRPCHandlerImpl(); + MonitoredRPCHandlerImpl monitor = new MonitoredRPCHandlerImpl("test"); monitor.abort("abort RPC"); TestParam testParam = new TestParam("param1"); monitor.setRPC("method1", new Object[] { testParam }, 0); From 446c02ee2b1aa844dd591e672502234bd5da553d Mon Sep 17 00:00:00 2001 From: Briana Augenreich Date: Tue, 6 Dec 2022 16:14:39 -0500 Subject: [PATCH 4/6] PR feedback: fix precondition import --- .../org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java index 4d52908220bb..3dddbc00b5ce 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredTaskImpl.java @@ -17,7 +17,6 @@ */ package org.apache.hadoop.hbase.monitoring; -import com.google.common.base.Preconditions; import java.io.IOException; import java.util.Collections; import java.util.HashMap; @@ -29,6 +28,7 @@ import org.apache.hadoop.hbase.util.GsonUtil; import org.apache.yetus.audience.InterfaceAudience; +import org.apache.hbase.thirdparty.com.google.common.base.Preconditions; import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableList; import org.apache.hbase.thirdparty.com.google.gson.Gson; From 308a20722d7c6ab910c54fe3563d0a641ad7fc93 Mon Sep 17 00:00:00 2001 From: Briana Augenreich Date: Wed, 7 Dec 2022 09:29:49 -0500 Subject: [PATCH 5/6] Add tests --- .../org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java index 2e84e91b1561..c3bb1ec17663 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java @@ -54,6 +54,8 @@ public void testTaskMonitorBasics() { TaskMonitor tm = new TaskMonitor(new Configuration()); assertTrue("Task monitor should start empty", tm.getTasks().isEmpty()); + + // Make a task and fetch it back out MonitoredTask task = tm.createStatus("Test task"); MonitoredTask taskFromTm = tm.getTasks().get(0); @@ -62,6 +64,8 @@ public void testTaskMonitorBasics() { assertEquals(task.getDescription(), taskFromTm.getDescription()); assertEquals(-1, taskFromTm.getCompletionTimestamp()); assertEquals(MonitoredTask.State.RUNNING, taskFromTm.getState()); + assertEquals(task.getStatus(), taskFromTm.getStatus()); + assertEquals("status unset", taskFromTm.getStatus()); // Mark it as finished task.markComplete("Finished!"); From 72ef33fca29a1542e33a35b6dce519535b82d5da Mon Sep 17 00:00:00 2001 From: Briana Augenreich Date: Wed, 7 Dec 2022 09:51:02 -0500 Subject: [PATCH 6/6] Spotless --- .../org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java index c3bb1ec17663..65002c51c272 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/monitoring/TestTaskMonitor.java @@ -54,8 +54,6 @@ public void testTaskMonitorBasics() { TaskMonitor tm = new TaskMonitor(new Configuration()); assertTrue("Task monitor should start empty", tm.getTasks().isEmpty()); - - // Make a task and fetch it back out MonitoredTask task = tm.createStatus("Test task"); MonitoredTask taskFromTm = tm.getTasks().get(0);