You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by GitBox <gi...@apache.org> on 2022/11/25 09:48:22 UTC
[GitHub] [rocketmq] drpmma opened a new pull request, #5596: [ISSUE #5595] Add validateSystemTopicWhenUpdateTopic
drpmma opened a new pull request, #5596:
URL: https://github.com/apache/rocketmq/pull/5596
## What is the purpose of the change
Add validateSystemTopicWhenUpdateTopic
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] aaron-ai commented on a diff in pull request #5596: [ISSUE #5595] Add validateSystemTopicWhenUpdateTopic
Posted by GitBox <gi...@apache.org>.
aaron-ai commented on code in PR #5596:
URL: https://github.com/apache/rocketmq/pull/5596#discussion_r1035708348
##########
broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java:
##########
@@ -456,10 +458,12 @@ private synchronized RemotingCommand updateAndCreateStaticTopic(ChannelHandlerCo
response.setRemark(result.getRemark());
return response;
}
- if (TopicValidator.isSystemTopic(topic)) {
- response.setCode(ResponseCode.SYSTEM_ERROR);
- response.setRemark("The topic[" + topic + "] is conflict with system topic.");
- return response;
+ if (brokerController.getBrokerConfig().isValidateSystemTopicWhenUpdateTopic()) {
+ if (TopicValidator.isSystemTopic(topic)) {
+ response.setCode(ResponseCode.SYSTEM_ERROR);
Review Comment:
Merge the judgement into one-line could help us to reduce the unnecessary code indent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] drpmma merged pull request #5596: [ISSUE #5595] Add validateSystemTopicWhenUpdateTopic
Posted by GitBox <gi...@apache.org>.
drpmma merged PR #5596:
URL: https://github.com/apache/rocketmq/pull/5596
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] ShadowySpirits commented on a diff in pull request #5596: [ISSUE #5595] Add validateSystemTopicWhenUpdateTopic
Posted by GitBox <gi...@apache.org>.
ShadowySpirits commented on code in PR #5596:
URL: https://github.com/apache/rocketmq/pull/5596#discussion_r1032419199
##########
broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java:
##########
@@ -412,10 +412,12 @@ private synchronized RemotingCommand updateAndCreateTopic(ChannelHandlerContext
response.setRemark(result.getRemark());
return response;
}
- if (TopicValidator.isSystemTopic(topic)) {
- response.setCode(ResponseCode.SYSTEM_ERROR);
- response.setRemark("The topic[" + topic + "] is conflict with system topic.");
- return response;
+ if (brokerController.getBrokerConfig().isValidateSystemTopicWhenUpdateTopic()) {
+ if (TopicValidator.isSystemTopic(topic)) {
Review Comment:
Some other places, such as updateAndCreateStaticTopic, also use TopicValidator#isSystemTopic. Should we deal with them in the same way?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] codecov-commenter commented on pull request #5596: [ISSUE #5595] Add validateSystemTopicWhenUpdateTopic
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #5596:
URL: https://github.com/apache/rocketmq/pull/5596#issuecomment-1328442942
# [Codecov](https://codecov.io/gh/apache/rocketmq/pull/5596?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#5596](https://codecov.io/gh/apache/rocketmq/pull/5596?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6a0fd22) into [develop](https://codecov.io/gh/apache/rocketmq/commit/08dbe7ede925e578c2c9e9bda4a556937cec7c89?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (08dbe7e) will **increase** coverage by `0.01%`.
> The diff coverage is `33.33%`.
```diff
@@ Coverage Diff @@
## develop #5596 +/- ##
=============================================
+ Coverage 42.37% 42.39% +0.01%
- Complexity 7929 7931 +2
=============================================
Files 1022 1022
Lines 71254 71267 +13
Branches 9399 9401 +2
=============================================
+ Hits 30196 30211 +15
+ Misses 37230 37229 -1
+ Partials 3828 3827 -1
```
| [Impacted Files](https://codecov.io/gh/apache/rocketmq/pull/5596?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../java/org/apache/rocketmq/common/BrokerConfig.java](https://codecov.io/gh/apache/rocketmq/pull/5596/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jb21tb24vQnJva2VyQ29uZmlnLmphdmE=) | `28.01% <0.00%> (-0.14%)` | :arrow_down: |
| [...ocketmq/broker/processor/AdminBrokerProcessor.java](https://codecov.io/gh/apache/rocketmq/pull/5596/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvcHJvY2Vzc29yL0FkbWluQnJva2VyUHJvY2Vzc29yLmphdmE=) | `25.28% <40.00%> (-0.04%)` | :arrow_down: |
| [...ketmq/client/impl/consumer/PullMessageService.java](https://codecov.io/gh/apache/rocketmq/pull/5596/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9jb25zdW1lci9QdWxsTWVzc2FnZVNlcnZpY2UuamF2YQ==) | `49.23% <0.00%> (-3.08%)` | :arrow_down: |
| [...apache/rocketmq/store/ha/GroupTransferService.java](https://codecov.io/gh/apache/rocketmq/pull/5596/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL2hhL0dyb3VwVHJhbnNmZXJTZXJ2aWNlLmphdmE=) | `92.30% <0.00%> (-1.29%)` | :arrow_down: |
| [...a/org/apache/rocketmq/store/StoreStatsService.java](https://codecov.io/gh/apache/rocketmq/pull/5596/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL1N0b3JlU3RhdHNTZXJ2aWNlLmphdmE=) | `33.97% <0.00%> (-1.11%)` | :arrow_down: |
| [...ent/impl/consumer/DefaultLitePullConsumerImpl.java](https://codecov.io/gh/apache/rocketmq/pull/5596/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9jb25zdW1lci9EZWZhdWx0TGl0ZVB1bGxDb25zdW1lckltcGwuamF2YQ==) | `68.53% <0.00%> (-1.10%)` | :arrow_down: |
| [...ava/org/apache/rocketmq/store/index/IndexFile.java](https://codecov.io/gh/apache/rocketmq/pull/5596/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL2luZGV4L0luZGV4RmlsZS5qYXZh) | `73.68% <0.00%> (-0.88%)` | :arrow_down: |
| [...rocketmq/client/impl/factory/MQClientInstance.java](https://codecov.io/gh/apache/rocketmq/pull/5596/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9mYWN0b3J5L01RQ2xpZW50SW5zdGFuY2UuamF2YQ==) | `45.79% <0.00%> (-0.60%)` | :arrow_down: |
| [.../org/apache/rocketmq/store/ha/DefaultHAClient.java](https://codecov.io/gh/apache/rocketmq/pull/5596/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL2hhL0RlZmF1bHRIQUNsaWVudC5qYXZh) | `61.53% <0.00%> (-0.52%)` | :arrow_down: |
| ... and [12 more](https://codecov.io/gh/apache/rocketmq/pull/5596/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] aaron-ai commented on a diff in pull request #5596: [ISSUE #5595] Add validateSystemTopicWhenUpdateTopic
Posted by GitBox <gi...@apache.org>.
aaron-ai commented on code in PR #5596:
URL: https://github.com/apache/rocketmq/pull/5596#discussion_r1035708348
##########
broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java:
##########
@@ -456,10 +458,12 @@ private synchronized RemotingCommand updateAndCreateStaticTopic(ChannelHandlerCo
response.setRemark(result.getRemark());
return response;
}
- if (TopicValidator.isSystemTopic(topic)) {
- response.setCode(ResponseCode.SYSTEM_ERROR);
- response.setRemark("The topic[" + topic + "] is conflict with system topic.");
- return response;
+ if (brokerController.getBrokerConfig().isValidateSystemTopicWhenUpdateTopic()) {
+ if (TopicValidator.isSystemTopic(topic)) {
+ response.setCode(ResponseCode.SYSTEM_ERROR);
Review Comment:
Merge the judgement into one-line could help us to the unnecessary code indent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] drpmma commented on a diff in pull request #5596: [ISSUE #5595] Add validateSystemTopicWhenUpdateTopic
Posted by GitBox <gi...@apache.org>.
drpmma commented on code in PR #5596:
URL: https://github.com/apache/rocketmq/pull/5596#discussion_r1033060795
##########
broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java:
##########
@@ -412,10 +412,12 @@ private synchronized RemotingCommand updateAndCreateTopic(ChannelHandlerContext
response.setRemark(result.getRemark());
return response;
}
- if (TopicValidator.isSystemTopic(topic)) {
- response.setCode(ResponseCode.SYSTEM_ERROR);
- response.setRemark("The topic[" + topic + "] is conflict with system topic.");
- return response;
+ if (brokerController.getBrokerConfig().isValidateSystemTopicWhenUpdateTopic()) {
+ if (TopicValidator.isSystemTopic(topic)) {
Review Comment:
It's reasonable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] drpmma commented on pull request #5596: [ISSUE #5595] Add validateSystemTopicWhenUpdateTopic
Posted by GitBox <gi...@apache.org>.
drpmma commented on PR #5596:
URL: https://github.com/apache/rocketmq/pull/5596#issuecomment-1328424901
>
This pr is about `validateSystemTopicWhenUpdateTopic ` instead of `transactionOpBatchInterval`. Maybe you mistake the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org