You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by shroman <gi...@git.apache.org> on 2017/01/25 03:58:23 UTC
[GitHub] incubator-rocketmq pull request #51: [ROCKETMQ-75] RemotingCommand header de...
GitHub user shroman opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/51
[ROCKETMQ-75] RemotingCommand header decoding swallows exceptions
JIRA issue: https://issues.apache.org/jira/browse/ROCKETMQ-75
Added logging on errors.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shroman/incubator-rocketmq ROCKETMQ-75
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/51.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 #51
----
commit e0144072d4ee6dbfae8beb45aee1f0abbc6e147e
Author: shtykh_roman <rs...@yahoo.com>
Date: 2017-01-25T03:57:51Z
[ROCKETMQ-75] RemotingCommand header decoding swallows exceptions
JIRA issue: https://issues.apache.org/jira/browse/ROCKETMQ-75
Added logging on errors.
----
---
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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
@shroman since 4.1.0, we will start a new branching model, more formalized, more flexible. All PRs except those hotfixs, we recommended to merge back to develop branch. This guide will publish in website sooner. So, we hope to postpone this PR merge action util 4.0.0 release :-)
---
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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
@shroman client/common/remoting
Frankly speaking, it's kind of dilemma. pom.xml has already defined Java language version to 1.7 and JREs under 1.7 are no longer maintained by Oracle and other vendors; But the reality is many application developers are stilling working on legacy systems which requires Java 1.5, 1.6.
---
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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
[](https://coveralls.io/builds/10539954)
Coverage decreased (-0.02%) to 31.014% when pulling **fa0f6d020161ed00c27433046ca137a5311ce4b0 on shroman:ROCKETMQ-75** into **c5d9fcb548ac80ab32cbb8dccd121de874c2e6d7 on apache:develop**.
---
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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
@lizhanhui @vongosling I see. I changed the code, please have a look.
Also, I will try to keep the modules in mind, but we need something not to forget they have to be treated differently from others. Can't propose any clean solution now.
---
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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
[](https://coveralls.io/builds/9830681)
Coverage increased (+0.01%) to 30.092% when pulling **f01c1a65ed7a349363f593232c4fcbbbb25563de on shroman:ROCKETMQ-75** into **30cabc77bd99abcf934d822d69030ddcfe38f94a 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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
@vongosling Is it for `rocketmq-remoting` only or for the whole project?
It doesn't sound exciting to go back to old JDKs, but ok :)
---
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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
Hi @shroman ,
IMO, it could be better that one PR/issue one thing, can we move `makeCustomHeaderToNet` to another issue, and make `makeCustomHeaderToNet ()` private will bring some reflection code, I think it's not very reasonable.
---
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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
Looks good.
---
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 #51: [ROCKETMQ-75] RemotingCommand header de...
Posted by shroman <gi...@git.apache.org>.
Github user shroman closed the pull request at:
https://github.com/apache/incubator-rocketmq/pull/51
---
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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
Agree, for the client related module, I think it's best to have maximum compatibility as many application developers are still working on legacy systems.
---
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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
@shroman Recently, some our customer resort to us to degrade JDK version to 1.6. IMO, we can keep the pace with netty for our SDK. So, if we polish code associating with SDK, we'd better use JDK 1.6 semantic :-)
---
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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
@zhouxinyu I agree on moving it to another issue. I will create another PR for that.
As to making the method private, using reflection for testing is safer than making a method public solely for testing, IMO. It is even called by another private method ;)
---
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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
[](https://coveralls.io/builds/9831058)
Coverage increased (+0.08%) to 30.16% when pulling **f01c1a65ed7a349363f593232c4fcbbbb25563de on shroman:ROCKETMQ-75** into **30cabc77bd99abcf934d822d69030ddcfe38f94a 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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
@vongosling @lizhanhui are you ok with this fix? If yes, I will merge.
---
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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
Looks like there's no reason (and even may be dangerous) to keep `makeCustomHeaderToNet()` _public_. Made it _private_.
Ready for the 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-rocketmq issue #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
@zhouxinyu I removed the 2nd commit from this PR. Please review it.
So, what shall we do with `makeCustomHeaderToNet()`?
---
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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
LGTM :-) We will degrade the client SDK version to 1.6 in the developer branch
---
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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
cool~
---
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 #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
LGTM, thanks @shroman , please @vongosling @lizhanhui help 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-rocketmq issue #51: [ROCKETMQ-75] RemotingCommand header decoding ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/51
@lizhanhui which modules in RocketMQ are 'client-related'? to be aware when changing the code.
Ideally, it would be nice to split the client-related code with what we are doing at Apache. Just my thoughts.
---
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.
---