You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by deepakddixit <gi...@git.apache.org> on 2016/11/27 04:09:01 UTC
[GitHub] incubator-geode pull request #297: GEODE-734: gfsh export stack-traces shoul...
GitHub user deepakddixit opened a pull request:
https://github.com/apache/incubator-geode/pull/297
GEODE-734: gfsh export stack-traces should not require an output file with extension .txt
Removed mandatory txt file extension when exporting stack trace using gfsh command export stack-traces.
Added message warning user about possible overwrite of file if already present under locator directory.
Added test to check if non txt extension file is allowed.
All tests are passing
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/deepakddixit/incubator-geode feature/GEODE-734
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-geode/pull/297.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #297
----
commit c64c69c84623bc40365961665f7c9c66793ea0c5
Author: Deepak Dixit <de...@gmail.com>
Date: 2016-11-26T13:41:52Z
GEODE-734: Removing txt file restriction when exporting stack traces. Added message warning user about possible overwrite of file if already present.
Added test to check if non txt extension file is allowed.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request #297: GEODE-734: gfsh export stack-traces shoul...
Posted by kirklund <gi...@git.apache.org>.
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/297#discussion_r90506480
--- Diff: geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowStackTraceDUnitTest.java ---
@@ -90,6 +90,20 @@ public void run() {
public void testExportStacktrace() throws ClassNotFoundException, IOException {
setupSystem();
+ // Test non txt extension file is allowed
+ File stacktracesFile = new File("allStackTraces.log");
--- End diff --
Files should be created in a TemporaryFolder. Please see TemporaryFolder on https://github.com/junit-team/junit4/wiki/Rules.
We have a version of TemporaryFolder which works in DUnit (DistributedTests). See org.apache.geode.test.junit.rules.serializable.SerializableTemporaryFolder.
The TemporaryFolder Rule automatically takes care of deleting files and directories after each test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request #297: GEODE-734: gfsh export stack-traces shoul...
Posted by kirklund <gi...@git.apache.org>.
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/297#discussion_r90507113
--- Diff: geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowStackTraceDUnitTest.java ---
@@ -149,4 +163,92 @@ public void testExportStacktrace() throws ClassNotFoundException, IOException {
getLogWriter().info("Output : \n" + commandResultToString(commandResult));
assertFalse(commandResult.getStatus().equals(Status.OK));
}
+
+ /***
+ * Tests the behavior of the show stack-trace command when file is already present
+ *
+ * @throws ClassNotFoundException
+ * @throws IOException
+ */
+ @Test
--- End diff --
This looks like three different tests all inside one test. It's better to separate these so they're shorter, more readable and pass or fail independently of one another.
If you're worried about how long it takes to setup/teardown or run tests, then you really should consider testing this feature in a UnitTest or IntegrationTest.
UnitTest would use Mockito to help you isolate the one class for test (UnitTest shouldn't touch file system or network). IntegrationTest would allow you to touch file system or network, test group of classes, or create one Geode member (in one JVM). DistributedTest should only be used when you really need more than one member (multiple JVMs).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request #297: GEODE-734: gfsh export stack-traces shoul...
Posted by deepakddixit <gi...@git.apache.org>.
Github user deepakddixit commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/297#discussion_r90585692
--- Diff: geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ShowStackTraceDUnitTest.java ---
@@ -90,6 +90,20 @@ public void run() {
public void testExportStacktrace() throws ClassNotFoundException, IOException {
setupSystem();
+ // Test non txt extension file is allowed
+ File stacktracesFile = new File("allStackTraces.log");
--- End diff --
Thanks @kirklund. Will use TemporaryFolder to work with files.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode issue #297: GEODE-734: gfsh export stack-traces should not r...
Posted by metatype <gi...@git.apache.org>.
Github user metatype commented on the issue:
https://github.com/apache/incubator-geode/pull/297
Thanks for the contribution! However, I don't think we should require user input in order to gather stack traces. Quite often a user will automate collection (`gfsh export stack-traces ...`) and will not be able to provide input.
Here's a few ways we could extend the current behavior:
- Provide an option to auto-generate a unique name
- Provide an option that would cause the export to fail if the provided file name is already present.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode issue #297: GEODE-734: gfsh export stack-traces should not r...
Posted by deepakddixit <gi...@git.apache.org>.
Github user deepakddixit commented on the issue:
https://github.com/apache/incubator-geode/pull/297
@metatype and @kirklund I have addressed the review comments. Kindly have a look.
precheckin is successful. I am not sure why travis is failed with spotlessJavaCheck, at my box it is successful. I tried spotlessApply but it shows no changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---