You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by n-tran <gi...@git.apache.org> on 2015/12/11 04:29:20 UTC
[GitHub] incubator-tinkerpop pull request: TINKERPOP-1019 Replaced all syst...
GitHub user n-tran opened a pull request:
https://github.com/apache/incubator-tinkerpop/pull/177
TINKERPOP-1019 Replaced all system.out.println and system.out.error with logger.
Removed the `muteTestLog` system property because logging can now be controlled by config file.
Ran `mvn clean install` and `mvn clean install -Dci`
Logging appear appropriate in both cases.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/n-tran/incubator-tinkerpop TINKERPOP-1019
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-tinkerpop/pull/177.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 #177
----
commit 0c783835c165b3c9097418947db252cc399a5c52
Author: Nghia Tran <ng...@gmail.com>
Date: 2015-12-11T03:24:02Z
Replaced all system.out.println and system.out.error with logger.
Removed the muteTestLog system property because logging can now be controlled by config file.
----
---
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-tinkerpop pull request: TINKERPOP-1019 Replaced all syst...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/177#issuecomment-164588155
i'll deal with the merge - probably not a big deal.
---
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-tinkerpop pull request: TINKERPOP-1019 Replaced all syst...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/177#issuecomment-164587847
Looks good now; I guess the merge conflict is just a minor issue.
VOTE: +1
---
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-tinkerpop pull request: TINKERPOP-1019 Replaced all syst...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/177#issuecomment-164762350
merged to master - thanks @n-tran
---
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-tinkerpop pull request: TINKERPOP-1019 Replaced all syst...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on a diff in the pull request:
https://github.com/apache/incubator-tinkerpop/pull/177#discussion_r47351037
--- Diff: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/GremlinProcessRunner.java ---
@@ -54,7 +56,7 @@ public void runChild(final FrameworkMethod method, final RunNotifier notifier) {
} catch (Throwable e) {
if (validateForGraphComputer(e)) {
eachNotifier.fireTestIgnored();
- System.err.println(e.getMessage());
+ logger.error(e.getMessage());
ignored = true;
--- End diff --
Yeah, I think WARN would be fine too. INFO is what's causing trouble.
---
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-tinkerpop pull request: TINKERPOP-1019 Replaced all syst...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on a diff in the pull request:
https://github.com/apache/incubator-tinkerpop/pull/177#discussion_r47519037
--- Diff: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverIntegrateTest.java ---
@@ -71,7 +73,7 @@
* @author Stephen Mallette (http://stephen.genoprime.com)
*/
public class GremlinDriverIntegrateTest extends AbstractGremlinServerIntegrationTest {
-
+ private static final Logger logger = LoggerFactory.getLogger(AbstractGremlinServerIntegrationTest.class);
--- End diff --
This should be `LoggerFactory.getLogger(GremlinDriverIntegrateTest .class)`, right?
---
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-tinkerpop pull request: TINKERPOP-1019 Replaced all syst...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/incubator-tinkerpop/pull/177#discussion_r47349514
--- Diff: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/GremlinProcessRunner.java ---
@@ -54,7 +56,7 @@ public void runChild(final FrameworkMethod method, final RunNotifier notifier) {
} catch (Throwable e) {
if (validateForGraphComputer(e)) {
eachNotifier.fireTestIgnored();
- System.err.println(e.getMessage());
+ logger.error(e.getMessage());
ignored = true;
--- End diff --
@dkuppitz perhaps on merge we could change this to WARN level? this situation doesn't really indicate a "failure" does it?
---
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-tinkerpop pull request: TINKERPOP-1019 Replaced all syst...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/177#issuecomment-163925807
lgtm - travis passes and logging line counts are down by about 2000 lines or so
vote: +1
---
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-tinkerpop pull request: TINKERPOP-1019 Replaced all syst...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/incubator-tinkerpop/pull/177#discussion_r47520037
--- Diff: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverIntegrateTest.java ---
@@ -71,7 +73,7 @@
* @author Stephen Mallette (http://stephen.genoprime.com)
*/
public class GremlinDriverIntegrateTest extends AbstractGremlinServerIntegrationTest {
-
+ private static final Logger logger = LoggerFactory.getLogger(AbstractGremlinServerIntegrationTest.class);
--- End diff --
well noted - i missed that one in my review.
---
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-tinkerpop pull request: TINKERPOP-1019 Replaced all syst...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-tinkerpop/pull/177
---
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-tinkerpop pull request: TINKERPOP-1019 Replaced all syst...
Posted by twilmes <gi...@git.apache.org>.
Github user twilmes commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/177#issuecomment-164592366
VOTE: +1
---
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.
---