You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "TheR1sing3un (via GitHub)" <gi...@apache.org> on 2023/05/21 07:38:00 UTC
[GitHub] [rocketmq] TheR1sing3un opened a new pull request, #6778: [ISSUE #6777]Support metric in controller
TheR1sing3un opened a new pull request, #6778:
URL: https://github.com/apache/rocketmq/pull/6778
<!-- Please make sure the target branch is right. In most case, the target branch should be `develop`. -->
### Which Issue(s) This PR Fixes
<!-- Please ensure that the related issue has already been created, and [link this pull request to that issue using keywords](<https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword>) to ensure automatic closure. -->
Fixes #6777
### Brief Description
<!-- Write a brief description for your pull request to help the maintainer understand the reasons behind your changes. -->
### How Did You Test This Change?
<!-- In order to ensure the code quality of Apache RocketMQ, we expect every pull request to have undergone thorough testing. -->
--
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] TheR1sing3un commented on pull request #6778: [ISSUE #6777]Support metric in controller
Posted by "TheR1sing3un (via GitHub)" <gi...@apache.org>.
TheR1sing3un commented on PR #6778:
URL: https://github.com/apache/rocketmq/pull/6778#issuecomment-1556108323
@RongtongJin PTAL~
--
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] TheR1sing3un commented on a diff in pull request #6778: [ISSUE #6777]Support metric in controller
Posted by "TheR1sing3un (via GitHub)" <gi...@apache.org>.
TheR1sing3un commented on code in PR #6778:
URL: https://github.com/apache/rocketmq/pull/6778#discussion_r1202477872
##########
controller/pom.xml:
##########
@@ -58,5 +58,9 @@
<groupId>${project.groupId}</groupId>
<artifactId>rocketmq-srvutil</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>jul-to-slf4j</artifactId>
+ </dependency>
Review Comment:
> Any reasons to add this dependency?
We need to use log as mertric exporter.
<img width="1302" alt="image" src="https://github.com/apache/rocketmq/assets/87409330/9c927052-40cb-4eed-824e-5133279279e7">
--
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] RongtongJin commented on pull request #6778: [ISSUE #6777]Support metric in controller
Posted by "RongtongJin (via GitHub)" <gi...@apache.org>.
RongtongJin commented on PR #6778:
URL: https://github.com/apache/rocketmq/pull/6778#issuecomment-1566436936
LGTM～ @caigy plz help to review again.
--
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] caigy commented on a diff in pull request #6778: [ISSUE #6777]Support metric in controller
Posted by "caigy (via GitHub)" <gi...@apache.org>.
caigy commented on code in PR #6778:
URL: https://github.com/apache/rocketmq/pull/6778#discussion_r1201740693
##########
controller/src/main/java/org/apache/rocketmq/controller/BrokerHeartbeatManager.java:
##########
@@ -63,4 +64,10 @@ void onBrokerHeartbeat(final String clusterName, final String brokerName, final
* Check whether broker active
*/
boolean isBrokerActive(final String clusterName, final String brokerName, final Long brokerId);
+
+ /**
+ * Count the number of active brokers in each broker-set of each cluster
+ * @return active brokers count
+ */
+ Map<String/*cluster*/, Map<String/*broker-set*/, Long/*active broker num*/>> getActiveBrokersNum();
Review Comment:
It's unnecessary to use type `Long` for the number of brokers, `Integer` is enough.
##########
controller/src/main/java/org/apache/rocketmq/controller/impl/heartbeat/DefaultBrokerHeartbeatManager.java:
##########
@@ -173,4 +175,15 @@ public boolean isBrokerActive(String clusterName, String brokerName, Long broker
return false;
}
+ @Override
+ public Map<String, Map<String, Long>> getActiveBrokersNum() {
+ Map<String, Map<String, Long>> map = new HashMap<>();
+ this.brokerLiveTable.keySet().forEach(id -> {
+ map.computeIfAbsent(id.getClusterName(), k -> new HashMap<>());
+ map.get(id.getClusterName()).compute(id.getBrokerName(), (broker, num) ->
+ num == null ? 0L : num + 1L
+ );
+ });
+ return map;
+ }
Review Comment:
- Whether a broker is 'active' should be checked by `isBrokerActive()`, rather than just checking if it is in`brokerLiveTable`.
- IMO this method provides a dynamic view of `brokerLiveTable`. As the returned data structure is used only in metrics instead of heat beat of brokers, it seems inappropriate to add methods in the interface `BrokerHeartbeatManager`. Placing it in metric-related packages may be more appropriate.
--
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 #6778: [ISSUE #6777]Support metric in controller
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #6778:
URL: https://github.com/apache/rocketmq/pull/6778#issuecomment-1559657679
## [Codecov](https://app.codecov.io/gh/apache/rocketmq/pull/6778?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#6778](https://app.codecov.io/gh/apache/rocketmq/pull/6778?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (bc2a94c) into [develop](https://app.codecov.io/gh/apache/rocketmq/commit/9dfd4a95997bfe0aa0ab6d812f6b4dd01509d8d5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (9dfd4a9) will **increase** coverage by `0.01%`.
> The diff coverage is `34.00%`.
```diff
@@ Coverage Diff @@
## develop #6778 +/- ##
=============================================
+ Coverage 42.84% 42.85% +0.01%
- Complexity 8976 9000 +24
=============================================
Files 1108 1107 -1
Lines 78444 78738 +294
Branches 10212 10235 +23
=============================================
+ Hits 33607 33747 +140
- Misses 40617 40761 +144
- Partials 4220 4230 +10
```
| [Impacted Files](https://app.codecov.io/gh/apache/rocketmq/pull/6778?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../java/org/apache/rocketmq/common/BrokerConfig.java](https://app.codecov.io/gh/apache/rocketmq/pull/6778?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jb21tb24vQnJva2VyQ29uZmlnLmphdmE=) | `27.38% <ø> (-0.61%)` | :arrow_down: |
| [...a/org/apache/rocketmq/common/ControllerConfig.java](https://app.codecov.io/gh/apache/rocketmq/pull/6778?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jb21tb24vQ29udHJvbGxlckNvbmZpZy5qYXZh) | `0.00% <0.00%> (ø)` | |
| [.../impl/heartbeat/DefaultBrokerHeartbeatManager.java](https://app.codecov.io/gh/apache/rocketmq/pull/6778?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvY29udHJvbGxlci9pbXBsL2hlYXJ0YmVhdC9EZWZhdWx0QnJva2VySGVhcnRiZWF0TWFuYWdlci5qYXZh) | `73.86% <0.00%> (-12.22%)` | :arrow_down: |
| [...he/rocketmq/proxy/metrics/ProxyMetricsManager.java](https://app.codecov.io/gh/apache/rocketmq/pull/6778?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHJveHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3Byb3h5L21ldHJpY3MvUHJveHlNZXRyaWNzTWFuYWdlci5qYXZh) | `6.29% <0.00%> (ø)` | |
| [.../rocketmq/broker/metrics/BrokerMetricsManager.java](https://app.codecov.io/gh/apache/rocketmq/pull/6778?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvbWV0cmljcy9Ccm9rZXJNZXRyaWNzTWFuYWdlci5qYXZh) | `11.69% <11.11%> (ø)` | |
| [...q/controller/metrics/ControllerMetricsManager.java](https://app.codecov.io/gh/apache/rocketmq/pull/6778?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvY29udHJvbGxlci9tZXRyaWNzL0NvbnRyb2xsZXJNZXRyaWNzTWFuYWdlci5qYXZh) | `19.37% <19.37%> (ø)` | |
| [.../org/apache/rocketmq/proxy/config/ProxyConfig.java](https://app.codecov.io/gh/apache/rocketmq/pull/6778?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHJveHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3Byb3h5L2NvbmZpZy9Qcm94eUNvbmZpZy5qYXZh) | `54.20% <33.33%> (ø)` | |
| [...ntroller/processor/ControllerRequestProcessor.java](https://app.codecov.io/gh/apache/rocketmq/pull/6778?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvY29udHJvbGxlci9wcm9jZXNzb3IvQ29udHJvbGxlclJlcXVlc3RQcm9jZXNzb3IuamF2YQ==) | `44.05% <48.00%> (+0.83%)` | :arrow_up: |
| [...e/rocketmq/common/metrics/MetricsExporterType.java](https://app.codecov.io/gh/apache/rocketmq/pull/6778?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jb21tb24vbWV0cmljcy9NZXRyaWNzRXhwb3J0ZXJUeXBlLmphdmE=) | `53.33% <53.33%> (ø)` | |
| [.../main/java/org/apache/rocketmq/common/UtilAll.java](https://app.codecov.io/gh/apache/rocketmq/pull/6778?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jb21tb24vVXRpbEFsbC5qYXZh) | `36.84% <58.82%> (+0.93%)` | :arrow_up: |
| ... and [4 more](https://app.codecov.io/gh/apache/rocketmq/pull/6778?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
... and [19 files with indirect coverage changes](https://app.codecov.io/gh/apache/rocketmq/pull/6778/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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=apache)
--
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] RongtongJin merged pull request #6778: [ISSUE #6777]Support metric in controller
Posted by "RongtongJin (via GitHub)" <gi...@apache.org>.
RongtongJin merged PR #6778:
URL: https://github.com/apache/rocketmq/pull/6778
--
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] caigy commented on a diff in pull request #6778: [ISSUE #6777]Support metric in controller
Posted by "caigy (via GitHub)" <gi...@apache.org>.
caigy commented on code in PR #6778:
URL: https://github.com/apache/rocketmq/pull/6778#discussion_r1201632535
##########
common/src/main/java/org/apache/rocketmq/common/UtilAll.java:
##########
@@ -765,4 +765,31 @@ private static void createDirIfNotExist(String dirName) {
STORE_LOG.info(dirName + " mkdir " + (result ? "OK" : "Failed"));
}
}
+
+ public static long calculateFileSizeInPath(File path) {
+ long size = 0;
+ try {
+ if (!path.exists()) {
+ return 0;
+ }
+ if (path.isFile()) {
+ return path.length();
+ }
+ if (path.isDirectory()) {
+ File[] files = path.listFiles();
+ if (files != null) {
+ for (File file : files) {
+ long fileSize = 0;
+ fileSize = calculateFileSizeInPath(file);
Review Comment:
You'd better check whether the file is a symbolic link before recursion.
##########
controller/pom.xml:
##########
@@ -58,5 +58,9 @@
<groupId>${project.groupId}</groupId>
<artifactId>rocketmq-srvutil</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>jul-to-slf4j</artifactId>
+ </dependency>
Review Comment:
Any reasons to add this dependency?
##########
common/src/main/java/org/apache/rocketmq/common/UtilAll.java:
##########
@@ -765,4 +765,31 @@ private static void createDirIfNotExist(String dirName) {
STORE_LOG.info(dirName + " mkdir " + (result ? "OK" : "Failed"));
}
}
+
+ public static long calculateFileSizeInPath(File path) {
+ long size = 0;
+ try {
+ if (!path.exists()) {
+ return 0;
+ }
+ if (path.isFile()) {
+ return path.length();
+ }
+ if (path.isDirectory()) {
+ File[] files = path.listFiles();
+ if (files != null) {
+ for (File file : files) {
+ long fileSize = 0;
+ fileSize = calculateFileSizeInPath(file);
Review Comment:
It's unnecessary to initialize `fileSize` as 0 as it's assigned right after initialization.
--
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