You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by Jaskey <gi...@git.apache.org> on 2017/03/10 08:24:20 UTC
[GitHub] incubator-rocketmq pull request #75: Add AuthenticationException class to re...
GitHub user Jaskey opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/75
Add AuthenticationException class to remove hard coded Aliyun authentication code
JIRA:https://issues.apache.org/jira/browse/ROCKETMQ-138
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Jaskey/incubator-rocketmq ROCKETMQ-138-AuthenticationException
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/75.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 #75
----
commit 79bb95e5fa7c8ddbf224e22868b13451b7c21f8b
Author: Jaskey <li...@gmail.com>
Date: 2017-03-10T08:17:05Z
Add AuthenticationException class to remove hard coded Aliyun authentication class
----
---
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 #75: [ROCKETMQ-138]Remove hard coded Aliyun ...
Posted by Jaskey <gi...@git.apache.org>.
GitHub user Jaskey reopened a pull request:
https://github.com/apache/incubator-rocketmq/pull/75
[ROCKETMQ-138]Remove hard coded Aliyun authentication code
JIRA:https://issues.apache.org/jira/browse/ROCKETMQ-138
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Jaskey/incubator-rocketmq ROCKETMQ-138-AuthenticationException
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/75.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 #75
----
commit ce164ebb4dd624f7debcd3d83e9eaec246137b18
Author: Jaskey <li...@gmail.com>
Date: 2017-03-10T08:17:05Z
[ROCKETMQ-138]remove hard coded Aliyun authentication class
----
---
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 #75: [ROCKETMQ-138]Remove hard coded Aliyun authent...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
[](https://coveralls.io/builds/11020784)
Coverage increased (+0.3%) to 31.868% when pulling **ce164ebb4dd624f7debcd3d83e9eaec246137b18 on Jaskey:ROCKETMQ-138-AuthenticationException** into **203cb30a906a77f41b0e5ba09fc351434862d408 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 #75: Add AuthenticationException class to remove ha...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
[](https://coveralls.io/builds/10542075)
Coverage decreased (-0.006%) to 31.023% when pulling **79bb95e5fa7c8ddbf224e22868b13451b7c21f8b on Jaskey:ROCKETMQ-138-AuthenticationException** into **a146646b27af75540b7691e6dd9b1227d6aaf59b 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 #75: [ROCKETMQ-138]Add AuthenticationException clas...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
Do you guys really consider that is good enough?? @vongosling @lollipopjin
IMO, the origin design is OK, since the AuthenticationException is thrown from the hook.
The devs will log their own Authentication fail log already (may be print periodically or somehow ), but rocketmq remoting should only care about the real error.
If we remove it, all authentication fail will have one log, and this could be possibly very often and fill up with the remoting log, which I do not think it is good enough.
User should care about their custom authentication log, and remoting component should care about real error thrown from the processor.
---
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 #75: [ROCKETMQ-138]Add AuthenticationException clas...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
@vongosling OK, look forward to that!
So when will this pr be merged?
---
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 #75: [ROCKETMQ-138]Add AuthenticationException clas...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
@Jaskey Thanks for your elaborative consideration about exception. Let it go as you have changed. we will merge this 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 issue #75: [ROCKETMQ-138]Add AuthenticationException clas...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
[](https://coveralls.io/builds/10801304)
Coverage increased (+0.08%) to 31.701% when pulling **ce164ebb4dd624f7debcd3d83e9eaec246137b18 on Jaskey:ROCKETMQ-138-AuthenticationException** into **203cb30a906a77f41b0e5ba09fc351434862d408 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 #75: [ROCKETMQ-138]Remove hard coded Aliyun authent...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
It seems this pr is not merged yet, so I reopen 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-rocketmq issue #75: Add AuthenticationException class to remove ha...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
:+1: to this fix.
---
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 #75: [ROCKETMQ-138]Remove hard coded Aliyun ...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey closed the pull request at:
https://github.com/apache/incubator-rocketmq/pull/75
---
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 #75: [ROCKETMQ-138]Add AuthenticationException clas...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
So do you mean we just remove the hard code snippet in the pr and just make it log for that? Better solution will be introduced in 4.1.x in another patches?
---
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 #75: Add AuthenticationException class to re...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/75#discussion_r105351861
--- Diff: remoting/src/main/java/org/apache/rocketmq/remoting/exception/AuthenticationException.java ---
@@ -0,0 +1,26 @@
+/*
+ * 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.rocketmq.remoting.exception;
+
+/**
+ * Used when authorization failure , when Remoting component catches this exception, will ignore it.
--- End diff --
Let's not have "when Remoting component catches this exception, will ignore it." here, because it describes how it is used in NettyRemotingAbstract, not the class itself or its properties.
The description of what this class is and its role is sufficient.
---
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 #75: [ROCKETMQ-138]Add AuthenticationException clas...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
@vongosling @shroman
please review the updated 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 issue #75: [ROCKETMQ-138]Add AuthenticationException clas...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
If no special consideration for authentication errors is needed (which was obviously the intention in the original code), +1 for just removing the hard-coded snippet. No `AuthenticationException` is needed then.
I see no problem logging on every `Throwable`.
---
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 #75: Add AuthenticationException class to re...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/75#discussion_r105354854
--- Diff: remoting/src/main/java/org/apache/rocketmq/remoting/exception/AuthenticationException.java ---
@@ -0,0 +1,26 @@
+/*
+ * 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.rocketmq.remoting.exception;
+
+/**
+ * Used when authorization failure , when Remoting component catches this exception, will ignore it.
--- End diff --
OK, I will modify it later.
Besides, just a small questions about the commit process, should I squash the commits and recommit or just append a commit to let the admin to squash them?OK, I will modify it later.
Besides, just a small questions about the commit process, should I squash the commits and recommit or just append a commit to let the admin to squash them?
---
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 #75: [ROCKETMQ-138]Add AuthenticationException clas...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
[](https://coveralls.io/builds/10557748)
Coverage decreased (-0.04%) to 30.971% when pulling **e8104c0b89fb7b3527d24d99d2de358a82e91e36 on Jaskey:ROCKETMQ-138-AuthenticationException** into **d7decc84abc32dab63ee423d4d904f28d18cb1d7 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 pull request #75: [ROCKETMQ-138]Add AuthenticationExcepti...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey closed the pull request at:
https://github.com/apache/incubator-rocketmq/pull/75
---
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 #75: [ROCKETMQ-138]Add AuthenticationException clas...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
Yep agreed. We can remove the instanceof check and just log 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-rocketmq issue #75: [ROCKETMQ-138]Add AuthenticationException clas...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
LGTM
---
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 #75: [ROCKETMQ-138]Add AuthenticationException clas...
Posted by lollipopjin <gi...@git.apache.org>.
Github user lollipopjin commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
@Jaskey In my opinion, it' better to just remove the
> if (!"com.aliyun.openservices.ons.api.impl.authority.exception.AuthenticationException" .equals(e.getClass().getCanonicalName()))
and record the log in any case throwable.
---
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 #75: [ROCKETMQ-138]Remove hard coded Aliyun authent...
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
Done, please help close this PR. @Jaskey
---
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 #75: Add AuthenticationException class to re...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/75#discussion_r105356003
--- Diff: remoting/src/main/java/org/apache/rocketmq/remoting/exception/AuthenticationException.java ---
@@ -0,0 +1,26 @@
+/*
+ * 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.rocketmq.remoting.exception;
+
+/**
+ * Used when authorization failure , when Remoting component catches this exception, will ignore it.
--- End diff --
Normally, a committer can easily squash it (I don't exclude request to you to do it in some cases -- too many modifications, etc.) -- no problem, go ahead and just make your modifications.
---
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 #75: Add AuthenticationException class to remove ha...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
please notice your PR topic
---
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 #75: [ROCKETMQ-138]Add AuthenticationException clas...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
Could you remove the code snippet \u201c (!(e instanceof AuthenticationException)) {\u201d and class AuthenticationException
---
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 #75: [ROCKETMQ-138]Add AuthenticationException clas...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
@vongosling
Now nothing special has been done unless remove the hard corded aliyun code snippet.
I will close this pr after 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 #75: [ROCKETMQ-138]Add AuthenticationException clas...
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
@lollipopjin Please help review this 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 issue #75: [ROCKETMQ-138]Add AuthenticationException clas...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/75
If we remove it, all authentication fail will have one log, and this could be possibly very often and fill up with the remoting log, which I do not think it is good enough.
We can just keep it as the previous. We will introduce a new remoting module in the 4.1 with seriously consideration about exception mechanism~
---
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.
---