You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by kh...@apache.org on 2018/12/06 17:55:39 UTC
[geode] branch develop updated: GEODE-5971: Refactor ShowDeadlockCommand to return ResultModel and ex… (#2943)
This is an automated email from the ASF dual-hosted git repository.
khowe pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new c3c7600 GEODE-5971: Refactor ShowDeadlockCommand to return ResultModel and ex… (#2943)
c3c7600 is described below
commit c3c7600e001aa02933e07d9174f2aa029e1c810e
Author: Kenneth Howe <kh...@pivotal.io>
AuthorDate: Thu Dec 6 09:55:29 2018 -0800
GEODE-5971: Refactor ShowDeadlockCommand to return ResultModel and ex… (#2943)
GEODE-5971: Refactor ShowDeadlockCommand to return ResultModel and extend GfshCommand
- Updated test that was run order dependant
- Added a wait to make the deadlock test pass in repeat mode
Co-authored-by: Jens Deppe <jd...@pivotal.io>
Co-authored-by: Kenneth Howe <kh...@pivotal.io>
Co-authored-by: Peter Tran <pt...@pivotal.io>
Co-authored-by:Aditya Anchuri <aa...@pivotal.io>
---
.../cli/commands/ShowDeadlockDUnitTest.java | 134 +--------------------
.../internal/cli/commands/ShowDeadlockCommand.java | 70 ++++++++---
.../internal/cli/result/model/FileResultModel.java | 3 +-
.../commands/ShowDeadlockDistributedTestBase.java | 52 +++++---
.../commands/ShowDeadlockOverHttpDUnitTest.java | 34 ++++++
5 files changed, 128 insertions(+), 165 deletions(-)
diff --git a/geode-core/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java b/geode-core/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java
index 1439fc9..865cd46 100755
--- a/geode-core/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java
+++ b/geode-core/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java
@@ -14,33 +14,10 @@
*/
package org.apache.geode.management.internal.cli.commands;
-import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
-import static org.assertj.core.api.Assertions.assertThat;
+import org.junit.experimental.categories.Category;
-import java.io.File;
-import java.util.Properties;
-import java.util.concurrent.TimeUnit;
-import java.util.concurrent.locks.Lock;
-import java.util.concurrent.locks.ReentrantLock;
-
-import org.junit.After;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-
-import org.apache.geode.cache.execute.Function;
-import org.apache.geode.cache.execute.FunctionContext;
-import org.apache.geode.cache.execute.FunctionService;
-import org.apache.geode.cache.execute.ResultCollector;
-import org.apache.geode.distributed.ConfigurationProperties;
import org.apache.geode.distributed.internal.deadlock.GemFireDeadlockDetectorDUnitTest;
-import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
-import org.apache.geode.management.internal.cli.i18n.CliStrings;
-import org.apache.geode.test.concurrent.FileBasedCountDownLatch;
-import org.apache.geode.test.dunit.rules.ClusterStartupRule;
-import org.apache.geode.test.dunit.rules.MemberVM;
-import org.apache.geode.test.junit.rules.GfshCommandRule;
+import org.apache.geode.test.junit.categories.GfshTest;
/**
* Distributed tests for show deadlock command in {@link ShowDeadlockCommand}.
@@ -48,109 +25,6 @@ import org.apache.geode.test.junit.rules.GfshCommandRule;
* @see GemFireDeadlockDetectorDUnitTest
*/
-public class ShowDeadlockDUnitTest {
- private static Thread stuckThread = null;
- private static final Lock LOCK = new ReentrantLock();
-
- private MemberVM server1;
- private MemberVM server2;
-
- private File outputFile;
- private String showDeadlockCommand;
-
- @Rule
- public ClusterStartupRule lsRule = new ClusterStartupRule();
-
- @Rule
- public TemporaryFolder temporaryFolder = new TemporaryFolder();
-
- @Rule
- public GfshCommandRule gfsh = new GfshCommandRule();
-
- @Before
- public void setup() throws Exception {
- outputFile = new File(temporaryFolder.getRoot(), "dependency.txt").getAbsoluteFile();
- showDeadlockCommand = "show dead-locks --file=" + outputFile.getAbsolutePath();
- outputFile.delete();
-
- MemberVM locator = lsRule.startLocatorVM(0);
-
- Properties props = new Properties();
- props.setProperty(ConfigurationProperties.SERIALIZABLE_OBJECT_FILTER,
- "org.apache.geode.management.internal.cli.commands.ShowDeadlockDUnitTest*");
- server1 = lsRule.startServerVM(1, props, locator.getPort());
- server2 = lsRule.startServerVM(2, props, locator.getPort());
-
- gfsh.connect(locator);
- }
-
- @After
- public final void after() throws Exception {
- server1.invoke(() -> stuckThread.interrupt());
- server2.invoke(() -> stuckThread.interrupt());
- }
-
- @Test
- public void testNoDeadlock() throws Exception {
- gfsh.executeAndAssertThat(showDeadlockCommand).statusIsSuccess();
- String commandOutput = gfsh.getGfshOutput();
-
- assertThat(commandOutput).startsWith(CliStrings.SHOW_DEADLOCK__NO__DEADLOCK);
- assertThat(outputFile).exists();
- }
-
- @Test
- public void testDistributedDeadlockWithFunction() throws Exception {
- FileBasedCountDownLatch countDownLatch = new FileBasedCountDownLatch(2);
-
- // This thread locks the lock in server1 first, then server2.
- lockTheLocks(server1, server2, countDownLatch);
- // This thread locks the lock server2 first, then server1.
- lockTheLocks(server2, server1, countDownLatch);
-
- await()
- .untilAsserted(() -> {
- gfsh.executeAndAssertThat(showDeadlockCommand).statusIsSuccess();
- String commandOutput = gfsh.getGfshOutput();
- assertThat(commandOutput).startsWith(CliStrings.SHOW_DEADLOCK__DEADLOCK__DETECTED);
- assertThat(outputFile).exists();
- });
- }
-
- private void lockTheLocks(MemberVM thisVM, final MemberVM thatVM,
- FileBasedCountDownLatch countDownLatch) {
- thisVM.invokeAsync(() -> {
- LOCK.lock();
- countDownLatch.countDown();
- countDownLatch.await();
- // At this point each VM will hold its own lock.
- lockRemoteVM(thatVM);
- LOCK.unlock();
- });
- }
-
- private static void lockRemoteVM(MemberVM vmToLock) {
- InternalDistributedMember thatInternalMember = getInternalDistributedMember(vmToLock);
-
- ResultCollector collector =
- FunctionService.onMember(thatInternalMember).execute(new LockFunction());
- collector.getResult();
- }
-
- private static InternalDistributedMember getInternalDistributedMember(MemberVM memberVM) {
- return memberVM.getVM().invoke(
- () -> ClusterStartupRule.getCache().getInternalDistributedSystem().getDistributedMember());
- }
-
- private static class LockFunction implements Function<Object> {
- @Override
- public void execute(FunctionContext<Object> context) {
- stuckThread = Thread.currentThread();
- try {
- LOCK.tryLock(5, TimeUnit.MINUTES);
- } catch (InterruptedException e) {
- context.getResultSender().lastResult(null);
- }
- }
- }
+@Category({GfshTest.class})
+public class ShowDeadlockDUnitTest extends ShowDeadlockDistributedTestBase {
}
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockCommand.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockCommand.java
index 74e130d..fb38063 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockCommand.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockCommand.java
@@ -15,8 +15,11 @@
package org.apache.geode.management.internal.cli.commands;
+import java.io.IOException;
+import java.nio.file.Path;
import java.text.MessageFormat;
import java.util.Collection;
+import java.util.Map;
import java.util.Set;
import org.springframework.shell.core.annotation.CliCommand;
@@ -28,27 +31,32 @@ import org.apache.geode.distributed.internal.deadlock.Dependency;
import org.apache.geode.distributed.internal.deadlock.DependencyGraph;
import org.apache.geode.distributed.internal.deadlock.GemFireDeadlockDetector;
import org.apache.geode.management.cli.CliMetaData;
+import org.apache.geode.management.cli.GfshCommand;
import org.apache.geode.management.cli.Result;
+import org.apache.geode.management.internal.cli.CliAroundInterceptor;
+import org.apache.geode.management.internal.cli.GfshParseResult;
import org.apache.geode.management.internal.cli.i18n.CliStrings;
-import org.apache.geode.management.internal.cli.result.InfoResultData;
-import org.apache.geode.management.internal.cli.result.ResultBuilder;
+import org.apache.geode.management.internal.cli.result.model.FileResultModel;
+import org.apache.geode.management.internal.cli.result.model.InfoResultModel;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
import org.apache.geode.management.internal.security.ResourceOperation;
import org.apache.geode.security.ResourcePermission;
-public class ShowDeadlockCommand extends InternalGfshCommand {
+public class ShowDeadlockCommand extends GfshCommand {
@CliCommand(value = CliStrings.SHOW_DEADLOCK, help = CliStrings.SHOW_DEADLOCK__HELP)
- @CliMetaData(relatedTopic = {CliStrings.TOPIC_GEODE_DEBUG_UTIL})
+ @CliMetaData(relatedTopic = {CliStrings.TOPIC_GEODE_DEBUG_UTIL},
+ interceptor = "org.apache.geode.management.internal.cli.commands.ShowDeadlockCommand$Interceptor")
@ResourceOperation(resource = ResourcePermission.Resource.CLUSTER,
operation = ResourcePermission.Operation.READ)
- public Result showDeadlock(@CliOption(key = CliStrings.SHOW_DEADLOCK__DEPENDENCIES__FILE,
+ public ResultModel showDeadlock(@CliOption(key = CliStrings.SHOW_DEADLOCK__DEPENDENCIES__FILE,
help = CliStrings.SHOW_DEADLOCK__DEPENDENCIES__FILE__HELP,
mandatory = true) String filename) {
- Result result;
+ ResultModel result = new ResultModel();
try {
if (!filename.endsWith(".txt")) {
- return ResultBuilder
- .createUserErrorResult(CliStrings.format(CliStrings.INVALID_FILE_EXTENSION, ".txt"));
+ return ResultModel.createError(
+ CliStrings.format(CliStrings.INVALID_FILE_EXTENSION, ".txt"));
}
Set<DistributedMember> allMembers = getAllMembers();
GemFireDeadlockDetector gfeDeadLockDetector = new GemFireDeadlockDetector(allMembers);
@@ -63,26 +71,50 @@ public class ShowDeadlockCommand extends InternalGfshCommand {
}
Set<Dependency> dependencies = (Set<Dependency>) dependencyGraph.getEdges();
- InfoResultData resultData = ResultBuilder.createInfoResultData();
+ InfoResultModel infoResult = result.addInfo();
if (deadlock != null) {
if (deepest != null) {
- resultData.addLine(CliStrings.SHOW_DEADLOCK__DEEPEST_FOUND);
+ infoResult.addLine(CliStrings.SHOW_DEADLOCK__DEEPEST_FOUND);
} else {
- resultData.addLine(CliStrings.SHOW_DEADLOCK__DEADLOCK__DETECTED);
+ infoResult.addLine(CliStrings.SHOW_DEADLOCK__DEADLOCK__DETECTED);
}
- resultData.addLine(DeadlockDetector.prettyFormat(deadlock));
+ infoResult.addLine(DeadlockDetector.prettyFormat(deadlock));
} else {
- resultData.addLine(CliStrings.SHOW_DEADLOCK__NO__DEADLOCK);
+ infoResult.addLine(CliStrings.SHOW_DEADLOCK__NO__DEADLOCK);
}
- resultData.addAsFile(filename, DeadlockDetector.prettyFormat(dependencies),
- MessageFormat.format(CliStrings.SHOW_DEADLOCK__DEPENDENCIES__REVIEW, filename), false);
- result = ResultBuilder.buildResult(resultData);
-
+ result.addFile(filename, DeadlockDetector.prettyFormat(dependencies));
} catch (Exception e) {
- result = ResultBuilder
- .createGemFireErrorResult(CliStrings.SHOW_DEADLOCK__ERROR + " : " + e.getMessage());
+ result = ResultModel.createError(CliStrings.SHOW_DEADLOCK__ERROR + " : " + e.getMessage());
}
+
return result;
}
+
+ public static class Interceptor implements CliAroundInterceptor {
+ @Override
+ public ResultModel postExecution(GfshParseResult parseResult, ResultModel resultModel,
+ Path tempFile) {
+
+ if (resultModel.getFiles().size() != 1) {
+ resultModel.addInfo("No filename found to save");
+ resultModel.setStatus(Result.Status.ERROR);
+ return resultModel;
+ }
+
+ try {
+ for (Map.Entry<String, FileResultModel> entry : resultModel.getFiles().entrySet()) {
+ entry.getValue().saveFile();
+ resultModel.addInfo().addLine(
+ MessageFormat.format(CliStrings.SHOW_DEADLOCK__DEPENDENCIES__REVIEW, entry.getKey()));
+ }
+ } catch (IOException e) {
+ resultModel.addInfo().addLine("Unable to save file: " + e.getMessage());
+ resultModel.setStatus(Result.Status.ERROR);
+ }
+
+ return resultModel;
+ }
+ }
+
}
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/model/FileResultModel.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/model/FileResultModel.java
index 89721e1..2d769ff 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/model/FileResultModel.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/result/model/FileResultModel.java
@@ -113,8 +113,7 @@ public class FileResultModel {
Gfsh gfsh = Gfsh.getCurrentInstance();
if (file.exists()) {
- String fileExistsMessage = String.format("File with name \"%s\" already exists in \"%s\".",
- filename, directory.getAbsolutePath());
+ String fileExistsMessage = String.format("File \"%s\" already exists", filename);
if (gfsh != null && !gfsh.isQuietMode()) {
fileExistsMessage += " Overwrite? " + options + " : ";
String interaction = gfsh.interact(fileExistsMessage);
diff --git a/geode-core/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java b/geode-dunit/src/main/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDistributedTestBase.java
similarity index 81%
copy from geode-core/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java
copy to geode-dunit/src/main/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDistributedTestBase.java
index 1439fc9..f51c496 100755
--- a/geode-core/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDUnitTest.java
+++ b/geode-dunit/src/main/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockDistributedTestBase.java
@@ -20,7 +20,6 @@ import static org.assertj.core.api.Assertions.assertThat;
import java.io.File;
import java.util.Properties;
import java.util.concurrent.TimeUnit;
-import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import org.junit.After;
@@ -34,7 +33,6 @@ import org.apache.geode.cache.execute.FunctionContext;
import org.apache.geode.cache.execute.FunctionService;
import org.apache.geode.cache.execute.ResultCollector;
import org.apache.geode.distributed.ConfigurationProperties;
-import org.apache.geode.distributed.internal.deadlock.GemFireDeadlockDetectorDUnitTest;
import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
import org.apache.geode.management.internal.cli.i18n.CliStrings;
import org.apache.geode.test.concurrent.FileBasedCountDownLatch;
@@ -43,15 +41,14 @@ import org.apache.geode.test.dunit.rules.MemberVM;
import org.apache.geode.test.junit.rules.GfshCommandRule;
/**
- * Distributed tests for show deadlock command in {@link ShowDeadlockCommand}.
- *
- * @see GemFireDeadlockDetectorDUnitTest
+ * Distributed test base for show deadlock command in {@link ShowDeadlockCommand}.
*/
-public class ShowDeadlockDUnitTest {
- private static Thread stuckThread = null;
- private static final Lock LOCK = new ReentrantLock();
+public class ShowDeadlockDistributedTestBase {
+ private static Thread stuckThread;
+ private static final ReentrantLock LOCK = new ReentrantLock();
+ protected MemberVM locator;
private MemberVM server1;
private MemberVM server2;
@@ -73,21 +70,43 @@ public class ShowDeadlockDUnitTest {
showDeadlockCommand = "show dead-locks --file=" + outputFile.getAbsolutePath();
outputFile.delete();
- MemberVM locator = lsRule.startLocatorVM(0);
+ locator = lsRule.startLocatorVM(0, l -> l.withHttpService());
Properties props = new Properties();
props.setProperty(ConfigurationProperties.SERIALIZABLE_OBJECT_FILTER,
- "org.apache.geode.management.internal.cli.commands.ShowDeadlockDUnitTest*");
+ "org.apache.geode.management.internal.cli.commands.ShowDeadlock*");
server1 = lsRule.startServerVM(1, props, locator.getPort());
server2 = lsRule.startServerVM(2, props, locator.getPort());
- gfsh.connect(locator);
+ connect();
}
@After
- public final void after() throws Exception {
- server1.invoke(() -> stuckThread.interrupt());
- server2.invoke(() -> stuckThread.interrupt());
+ public final void interruptStuckThreads() throws Exception {
+ server1.invoke(() -> {
+ if (stuckThread != null) {
+ stuckThread.interrupt();
+ }
+ stuckThread = null;
+ });
+ server2.invoke(() -> {
+ if (stuckThread != null) {
+ stuckThread.interrupt();
+ }
+ stuckThread = null;
+ });
+
+ // Wait to allow the locks to be released to avoid environment pollution for tests that follow
+ server1.invoke(() -> {
+ await().until(() -> !LOCK.isLocked());
+ });
+ server2.invoke(() -> {
+ await().until(() -> !LOCK.isLocked());
+ });
+ }
+
+ public void connect() throws Exception {
+ gfsh.connectAndVerify(locator);
}
@Test
@@ -148,7 +167,12 @@ public class ShowDeadlockDUnitTest {
stuckThread = Thread.currentThread();
try {
LOCK.tryLock(5, TimeUnit.MINUTES);
+ LOCK.unlock();
} catch (InterruptedException e) {
+ if (LOCK.isHeldByCurrentThread()) {
+ LOCK.unlock();
+ }
+ } finally {
context.getResultSender().lastResult(null);
}
}
diff --git a/geode-web/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockOverHttpDUnitTest.java b/geode-web/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockOverHttpDUnitTest.java
new file mode 100644
index 0000000..45fc551
--- /dev/null
+++ b/geode-web/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/ShowDeadlockOverHttpDUnitTest.java
@@ -0,0 +1,34 @@
+/*
+ * 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.geode.management.internal.cli.commands;
+
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.test.junit.categories.GfshTest;
+import org.apache.geode.test.junit.categories.LoggingTest;
+import org.apache.geode.test.junit.rules.GfshCommandRule;
+
+@Category({GfshTest.class, LoggingTest.class})
+public class ShowDeadlockOverHttpDUnitTest extends ShowDeadlockDistributedTestBase {
+
+ @Override
+ public void connect() throws Exception {
+ if (!gfsh.isConnected()) {
+ gfsh.connectAndVerify(locator.getHttpPort(), GfshCommandRule.PortType.http);
+ }
+ }
+
+}