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/06/13 10:19:06 UTC
[GitHub] incubator-rocketmq pull request #119: [ROCKETMQ-223]-Rename DEFAULT_TOPIC
GitHub user Jaskey opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/119
[ROCKETMQ-223]-Rename DEFAULT_TOPIC
JIRA: https://issues.apache.org/jira/browse/ROCKETMQ-223
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Jaskey/incubator-rocketmq ROCKETMQ-223-rename-default-topic
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/119.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 #119
----
commit 6c94e638d460b1f4207dde7565a986e4c7106576
Author: Jaskey <li...@gmail.com>
Date: 2017-06-13T10:18:00Z
ROCKETMQ-223-Rename DEFAULT_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 #119: [ROCKETMQ-223]-Rename DEFAULT_TOPIC
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/119
[](https://coveralls.io/builds/11947883)
Coverage increased (+0.7%) to 39.103% when pulling **3cd9a0c2cfd9f30331233215ae707fee0d722973 on Jaskey:ROCKETMQ-223-rename-default-topic** into **c4a3e0c1325e67aec76aa94e9ade3a6ea1b682d2 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 #119: [ROCKETMQ-223]-Rename DEFAULT_TOPIC
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/119
[](https://coveralls.io/builds/11947883)
Coverage increased (+0.7%) to 39.103% when pulling **3cd9a0c2cfd9f30331233215ae707fee0d722973 on Jaskey:ROCKETMQ-223-rename-default-topic** into **c4a3e0c1325e67aec76aa94e9ade3a6ea1b682d2 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 #119: [ROCKETMQ-223]-Rename DEFAULT_TOPIC
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/119
@lizhanhui @dongeforever
any opinons? Can this pr be merged? please 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 #119: [ROCKETMQ-223]-Rename DEFAULT_TOPIC
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/119
Another thing is -- do we need the default topic at all?
---
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 #119: [ROCKETMQ-223]-Rename DEFAULT_TOPIC
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/119
@lizhanhui @zhouxinyu @shroman @dongeforever
What's your opinions on this?
---
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 #119: [ROCKETMQ-223]-Rename DEFAULT_TOPIC
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/119
@Jaskey Ok, it makes sense.
---
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 #119: [ROCKETMQ-223]-Rename DEFAULT_TOPIC
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/119
@lizhanhui
In my option for your consern # 2, I even consider that we separate the configuration logic from the default topic, since the clients may use different default key.
In my option, the below flow may be much more clearer:
1. Default topic is always created , just like the other preserved topic.
2. auto-creation configuration has no impact on this default topic, while it is a switch which enable the broker to support auto topic creation.
if (autoCreateTopicEnable) { //broker allow to auto create topic
if (topicKeyExist){ // exist the specified topic key
if (isInherite(topic)) { // topic has the right perm
log("topic key does not has the sufficient perm to auto create topic ")
}
}
else {
log("The topic key does not exist")
}
} else {
log("broker does not allow to auto create 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 #119: [ROCKETMQ-223]-Rename DEFAULT_TOPIC
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/119
Looks good to me. Actually some of the client code relies on presence of the topic which would log exceptions otherwise.
IMO, we'd better rename the variable name only to maintain best backward compatibility.
---
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 #119: [ROCKETMQ-223]-Rename DEFAULT_TOPIC
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/119
All 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-rocketmq issue #119: [ROCKETMQ-223]-Rename DEFAULT_TOPIC
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/119
[](https://coveralls.io/builds/11947355)
Coverage increased (+0.2%) to 38.662% when pulling **2f837bd68d46263ec68a5b64355ceed19c23aedb on Jaskey:ROCKETMQ-223-rename-default-topic** into **c4a3e0c1325e67aec76aa94e9ade3a6ea1b682d2 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 #119: [ROCKETMQ-223]-Rename DEFAULT_TOPIC
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/119
I am OK with this change.
---
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 #119: [ROCKETMQ-223]-Rename DEFAULT_TOPIC
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/119
I am +1 to rename the default topic name.
In addition, we'd better resolve all known issues relating the auto-create-topic mechanism.
1. Can we rename the name to something to void potential topic name collision?
1. Currently, once auto create was enabled, the auto_create topic is persisted, even if we later disable auto-creation, the topic is still there which does not make sense. IMO, we may exclude this topic from persistence.
---
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 #119: [ROCKETMQ-223]-Rename DEFAULT_TOPIC
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/119
@lizhanhui .
I think we'd better rename the topic name also since it has impact many users that they don't actually know what is this topic and hot to maintain it.
In the client code, the error of topic `defaultTopic` not exits will be ignored so it has no impact for the current users, unless that they will not be able to auto create topic if they only upgrade the broker but using the old client and they do not specify another topic key.
If needed , we can refactor the code flow on topic auto creation on another issue. For now, we just rename it to make it clear.
@shroman The reason I rename the variable is that the name `defaultTopic ` is confusing since it does not explain what it is used for and actually it is not a defaultTopic since it may not exist if auto creation is disabled.
---
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.
---