You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by wu-sheng <gi...@git.apache.org> on 2017/01/11 02:36:49 UTC
[GitHub] incubator-rocketmq pull request #34: fix/ROCKETMQ-39: avoid duplicated codes...
GitHub user wu-sheng opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/34
fix/ROCKETMQ-39: avoid duplicated codes.
ref issue: https://issues.apache.org/jira/browse/ROCKETMQ-39
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/wu-sheng/incubator-rocketmq fix/ROCKETMQ-39
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/34.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 #34
----
commit 02a2d0eab3c5ffccd74a2226ed0d5b59509c3d21
Author: wusheng <wu...@foxmail.com>
Date: 2017-01-11T02:34:18Z
fix/ROCKETMQ-39: avoid duplicated codes.
----
---
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-rocketmq issue #34: [ROCKETMQ-39] avoid duplicated codes.
Posted by dongeforever <gi...@git.apache.org>.
Github user dongeforever commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/34
@wu-sheng this PR has been merged. you may close the PR.
---
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-rocketmq pull request #34: [ROCKETMQ-39] avoid duplicated codes.
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-rocketmq/pull/34
---
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-rocketmq issue #34: fix/ROCKETMQ-39: avoid duplicated codes.
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/34
The extracted shutdown hook misses required license header and causes check-style failure. Please fix it and double check if there are more similar code style issues.
You may refer [here](http://rocketmq.incubator.apache.org/docs/code-guidelines/) in terms of code style.
---
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-rocketmq issue #34: [ROCKETMQ-39] avoid duplicated codes.
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/34
It could be better if we add some unit tests for this PR and please remove the author info.
---
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-rocketmq pull request #34: [ROCKETMQ-39] avoid duplicated codes.
Posted by WillemJiang <gi...@git.apache.org>.
Github user WillemJiang commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/34#discussion_r97189560
--- Diff: namesrv/src/main/java/org/apache/rocketmq/namesrv/NamesrvStartup.java ---
@@ -123,7 +123,7 @@ public static NamesrvController main0(String[] args) {
Runtime.getRuntime().addShutdownHook(new ShutdownHookThread(log, new Callable() {
@Override
- public Object call() throws Exception {
+ public Void call() throws Exception {
controller.shutdown();
return null;
--- End diff --
This line should be removed.
---
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-rocketmq pull request #34: [ROCKETMQ-39] avoid duplicated codes.
Posted by wu-sheng <gi...@git.apache.org>.
Github user wu-sheng commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/34#discussion_r97214517
--- Diff: filtersrv/src/main/java/org/apache/rocketmq/filtersrv/FiltersrvStartup.java ---
@@ -139,7 +139,7 @@ public static FiltersrvController createController(String[] args) {
Runtime.getRuntime().addShutdownHook(new ShutdownHookThread(log, new Callable() {
@Override
- public Object call() throws Exception {
+ public Void call() throws Exception {
controller.shutdown();
return null;
--- End diff --
+1 on `kill return`, sadly, it's not working.
---
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-rocketmq pull request #34: [ROCKETMQ-39] avoid duplicated codes.
Posted by wu-sheng <gi...@git.apache.org>.
Github user wu-sheng commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/34#discussion_r97197140
--- Diff: filtersrv/src/main/java/org/apache/rocketmq/filtersrv/FiltersrvStartup.java ---
@@ -139,7 +139,7 @@ public static FiltersrvController createController(String[] args) {
Runtime.getRuntime().addShutdownHook(new ShutdownHookThread(log, new Callable() {
@Override
- public Object call() throws Exception {
+ public Void call() throws Exception {
controller.shutdown();
return null;
--- End diff --
@WillemJiang, Are you sure about no need `return null`? I have run a `mvn package` on local, and you can see, `return statement` is required.
```
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.5.1:compile (default-compile) on project rocketmq-namesrv: Compilation failure
[ERROR] /Users/wusheng/Documents/code/github/incubator-rocketmq-fork/namesrv/src/main/java/org/apache/rocketmq/namesrv/NamesrvStartup.java:[128,17] \u7f3a\u5c11\u8fd4\u56de\u8bed\u53e5
[ERROR] -> [Help 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-rocketmq issue #34: [ROCKETMQ-39] avoid duplicated codes.
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/34
[](https://coveralls.io/builds/9780368)
Changes Unknown when pulling **c81c912526c4daf7ca01a187e8a181e605bc1824 on wu-sheng:fix/ROCKETMQ-39** into ** on apache:master**.
---
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-rocketmq issue #34: fix/ROCKETMQ-39: avoid duplicated codes.
Posted by wu-sheng <gi...@git.apache.org>.
Github user wu-sheng commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/34
@vongosling , fixed and done. Hoping `style-check` works well next time. \U0001f62d
---
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-rocketmq pull request #34: [ROCKETMQ-39] avoid duplicated codes.
Posted by WillemJiang <gi...@git.apache.org>.
Github user WillemJiang commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/34#discussion_r97214481
--- Diff: filtersrv/src/main/java/org/apache/rocketmq/filtersrv/FiltersrvStartup.java ---
@@ -139,7 +139,7 @@ public static FiltersrvController createController(String[] args) {
Runtime.getRuntime().addShutdownHook(new ShutdownHookThread(log, new Callable() {
@Override
- public Object call() throws Exception {
+ public Void call() throws Exception {
controller.shutdown();
return null;
--- End diff --
@wu-sheng , oh, it's my mistake, I just wrote a code and the return statement need to be kept.
As I'm not a big fan of null, I always want to kill it if possible.
---
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-rocketmq pull request #34: fix/ROCKETMQ-39: avoid duplicated codes...
Posted by WillemJiang <gi...@git.apache.org>.
Github user WillemJiang commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/34#discussion_r95544500
--- Diff: filtersrv/src/main/java/org/apache/rocketmq/filtersrv/FiltersrvStartup.java ---
@@ -135,27 +137,17 @@ public static FiltersrvController createController(String[] args) {
System.exit(-3);
}
- Runtime.getRuntime().addShutdownHook(new Thread(new Runnable() {
- private volatile boolean hasShutdown = false;
- private AtomicInteger shutdownTimes = new AtomicInteger(0);
-
+ Runtime.getRuntime().addShutdownHook(new ShutdownHookThread(log, new Callable() {
@Override
- public void run() {
- synchronized (this) {
- log.info("shutdown hook was invoked, " + this.shutdownTimes.incrementAndGet());
- if (!this.hasShutdown) {
- this.hasShutdown = true;
- long begineTime = System.currentTimeMillis();
- controller.shutdown();
- long consumingTimeTotal = System.currentTimeMillis() - begineTime;
- log.info("shutdown hook over, consuming time total(ms): " + consumingTimeTotal);
- }
- }
+ public Object call() throws Exception {
--- End diff --
If you don't want to return any thing here , you could define the method just like this.
public Void call() throws Exception {
}
---
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-rocketmq issue #34: [ROCKETMQ-39] avoid duplicated codes.
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/34
[](https://coveralls.io/builds/9761169)
Changes Unknown when pulling **68f64b643f795b34e8a2300475ca4f5fca52f53e on wu-sheng:fix/ROCKETMQ-39** into ** on apache:master**.
---
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-rocketmq issue #34: fix/ROCKETMQ-39: avoid duplicated codes.
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/34
@wu-sheng Could you modify your PR , following our codestyle and PR guide[1] :-)
[1]http://rocketmq.incubator.apache.org/docs/code-guidelines/
---
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-rocketmq pull request #34: fix/ROCKETMQ-39: avoid duplicated codes...
Posted by wu-sheng <gi...@git.apache.org>.
Github user wu-sheng commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/34#discussion_r95546560
--- Diff: filtersrv/src/main/java/org/apache/rocketmq/filtersrv/FiltersrvStartup.java ---
@@ -135,27 +137,17 @@ public static FiltersrvController createController(String[] args) {
System.exit(-3);
}
- Runtime.getRuntime().addShutdownHook(new Thread(new Runnable() {
- private volatile boolean hasShutdown = false;
- private AtomicInteger shutdownTimes = new AtomicInteger(0);
-
+ Runtime.getRuntime().addShutdownHook(new ShutdownHookThread(log, new Callable() {
@Override
- public void run() {
- synchronized (this) {
- log.info("shutdown hook was invoked, " + this.shutdownTimes.incrementAndGet());
- if (!this.hasShutdown) {
- this.hasShutdown = true;
- long begineTime = System.currentTimeMillis();
- controller.shutdown();
- long consumingTimeTotal = System.currentTimeMillis() - begineTime;
- log.info("shutdown hook over, consuming time total(ms): " + consumingTimeTotal);
- }
- }
+ public Object call() throws Exception {
--- End diff --
@WillemJiang done.
---
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-rocketmq pull request #34: [ROCKETMQ-39] avoid duplicated codes.
Posted by WillemJiang <gi...@git.apache.org>.
Github user WillemJiang commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/34#discussion_r97189548
--- Diff: filtersrv/src/main/java/org/apache/rocketmq/filtersrv/FiltersrvStartup.java ---
@@ -139,7 +139,7 @@ public static FiltersrvController createController(String[] args) {
Runtime.getRuntime().addShutdownHook(new ShutdownHookThread(log, new Callable() {
@Override
- public Object call() throws Exception {
+ public Void call() throws Exception {
controller.shutdown();
return null;
--- End diff --
When use the Void as the return signature, you don't need to use return in the function.
Please remove the line of 144.
---
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.
---