You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by "BUAAserein (via GitHub)" <gi...@apache.org> on 2023/02/24 10:25:32 UTC
[GitHub] [iotdb] BUAAserein opened a new pull request, #9140: [IOTDB-5582] Add region type in ratis metric prefix
BUAAserein opened a new pull request, #9140:
URL: https://github.com/apache/iotdb/pull/9140
## Description
### The current ratis metric prefix cannot distinguish metric items from different region types.
Solution: Add a region type to the metric prefix.
Please see details in [IOTDB-5582](https://issues.apache.org/jira/browse/IOTDB-5582).
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] SpriCoder commented on a diff in pull request #9140: [IOTDB-5582] Add region type in ratis metric prefix
Posted by "SpriCoder (via GitHub)" <gi...@apache.org>.
SpriCoder commented on code in PR #9140:
URL: https://github.com/apache/iotdb/pull/9140#discussion_r1117046797
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/metrics/RatisMetricSet.java:
##########
@@ -18,21 +18,36 @@
*/
package org.apache.iotdb.consensus.ratis.metrics;
+import org.apache.iotdb.common.rpc.thrift.TConsensusGroupType;
import org.apache.iotdb.metrics.AbstractMetricService;
import org.apache.iotdb.metrics.metricsets.IMetricSet;
import org.apache.ratis.metrics.MetricRegistries;
public class RatisMetricSet implements IMetricSet {
private MetricRegistries manager;
+ private String consensusGroupType;
@Override
public void bindTo(AbstractMetricService metricService) {
manager = MetricRegistries.global();
+ if (manager instanceof MetricRegistryManager) {
+ ((MetricRegistryManager) manager).setConsensusGroupType(consensusGroupType);
+ }
}
@Override
public void unbindFrom(AbstractMetricService metricService) {
manager.clear();
}
+
+ public void setConsensusGroupType(TConsensusGroupType consensusGroupType) {
+ if (consensusGroupType == TConsensusGroupType.ConfigNodeRegion) {
+ this.consensusGroupType = "confignode";
+ } else if (consensusGroupType == TConsensusGroupType.DataRegion) {
+ this.consensusGroupType = "data_region";
+ } else {
+ this.consensusGroupType = "schema_region";
+ }
+ }
Review Comment:
May be use enum class can be a good idea
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] OneSizeFitsQuorum merged pull request #9140: [IOTDB-5582] Add region type in ratis metric prefix
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum merged PR #9140:
URL: https://github.com/apache/iotdb/pull/9140
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] SzyWilliam commented on a diff in pull request #9140: [IOTDB-5582] Add region type in ratis metric prefix
Posted by "SzyWilliam (via GitHub)" <gi...@apache.org>.
SzyWilliam commented on code in PR #9140:
URL: https://github.com/apache/iotdb/pull/9140#discussion_r1117064214
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/metrics/RatisMetricSet.java:
##########
@@ -18,21 +18,36 @@
*/
package org.apache.iotdb.consensus.ratis.metrics;
+import org.apache.iotdb.common.rpc.thrift.TConsensusGroupType;
import org.apache.iotdb.metrics.AbstractMetricService;
import org.apache.iotdb.metrics.metricsets.IMetricSet;
import org.apache.ratis.metrics.MetricRegistries;
public class RatisMetricSet implements IMetricSet {
private MetricRegistries manager;
+ private String consensusGroupType;
@Override
public void bindTo(AbstractMetricService metricService) {
manager = MetricRegistries.global();
+ if (manager instanceof MetricRegistryManager) {
+ ((MetricRegistryManager) manager).setConsensusGroupType(consensusGroupType);
+ }
}
@Override
public void unbindFrom(AbstractMetricService metricService) {
manager.clear();
}
+
+ public void setConsensusGroupType(TConsensusGroupType consensusGroupType) {
+ if (consensusGroupType == TConsensusGroupType.ConfigNodeRegion) {
+ this.consensusGroupType = "confignode";
+ } else if (consensusGroupType == TConsensusGroupType.DataRegion) {
+ this.consensusGroupType = "data_region";
+ } else {
+ this.consensusGroupType = "schema_region";
+ }
+ }
Review Comment:
We can use `consensusGroupType.toString()` here
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/metrics/RatisMetricSet.java:
##########
@@ -18,21 +18,36 @@
*/
package org.apache.iotdb.consensus.ratis.metrics;
+import org.apache.iotdb.common.rpc.thrift.TConsensusGroupType;
import org.apache.iotdb.metrics.AbstractMetricService;
import org.apache.iotdb.metrics.metricsets.IMetricSet;
import org.apache.ratis.metrics.MetricRegistries;
public class RatisMetricSet implements IMetricSet {
private MetricRegistries manager;
+ private String consensusGroupType;
Review Comment:
We can pass the consensusGroupType as a constructor param, instead of setting it manually.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] BUAAserein commented on a diff in pull request #9140: [IOTDB-5582] Add region type in ratis metric prefix
Posted by "BUAAserein (via GitHub)" <gi...@apache.org>.
BUAAserein commented on code in PR #9140:
URL: https://github.com/apache/iotdb/pull/9140#discussion_r1118046412
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/metrics/RatisMetricSet.java:
##########
@@ -18,21 +18,36 @@
*/
package org.apache.iotdb.consensus.ratis.metrics;
+import org.apache.iotdb.common.rpc.thrift.TConsensusGroupType;
import org.apache.iotdb.metrics.AbstractMetricService;
import org.apache.iotdb.metrics.metricsets.IMetricSet;
import org.apache.ratis.metrics.MetricRegistries;
public class RatisMetricSet implements IMetricSet {
private MetricRegistries manager;
+ private String consensusGroupType;
@Override
public void bindTo(AbstractMetricService metricService) {
manager = MetricRegistries.global();
+ if (manager instanceof MetricRegistryManager) {
+ ((MetricRegistryManager) manager).setConsensusGroupType(consensusGroupType);
+ }
}
@Override
public void unbindFrom(AbstractMetricService metricService) {
manager.clear();
}
+
+ public void setConsensusGroupType(TConsensusGroupType consensusGroupType) {
+ if (consensusGroupType == TConsensusGroupType.ConfigNodeRegion) {
+ this.consensusGroupType = "confignode";
+ } else if (consensusGroupType == TConsensusGroupType.DataRegion) {
+ this.consensusGroupType = "data_region";
+ } else {
+ this.consensusGroupType = "schema_region";
+ }
+ }
Review Comment:
Already changed.
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/metrics/RatisMetricSet.java:
##########
@@ -18,21 +18,36 @@
*/
package org.apache.iotdb.consensus.ratis.metrics;
+import org.apache.iotdb.common.rpc.thrift.TConsensusGroupType;
import org.apache.iotdb.metrics.AbstractMetricService;
import org.apache.iotdb.metrics.metricsets.IMetricSet;
import org.apache.ratis.metrics.MetricRegistries;
public class RatisMetricSet implements IMetricSet {
private MetricRegistries manager;
+ private String consensusGroupType;
Review Comment:
Already changed.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org