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/09/13 07:58:05 UTC
[GitHub] [iotdb] BUAAserein opened a new pull request, #11134: [IOTDB-6152] Add IoTConsensus Sync Log Timer
BUAAserein opened a new pull request, #11134:
URL: https://github.com/apache/iotdb/pull/11134
## Description
When the IoT follower receives a syncLogEntries request, it will go through three stages: deserialize, sort and apply. This pr adds this three timers to record the IoT follower sync log time.
--
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 commented on a diff in pull request #11134: [IOTDB-6152] Add IoTConsensus Sync Log Timer
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #11134:
URL: https://github.com/apache/iotdb/pull/11134#discussion_r1324296995
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/iot/IoTConsensusServerMetrics.java:
##########
@@ -36,23 +37,44 @@ public class IoTConsensusServerMetrics implements IMetricSet {
private Histogram writeStateMachineHistogram = DoNothingMetricManager.DO_NOTHING_HISTOGRAM;
private Histogram offerRequestToQueueHistogram = DoNothingMetricManager.DO_NOTHING_HISTOGRAM;
private Histogram consensusWriteHistogram = DoNothingMetricManager.DO_NOTHING_HISTOGRAM;
+ private static final String IOT_SYNC_LOG = Metric.IOT_SYNC_LOG.toString();
+ private Timer deserializeTimer = DoNothingMetricManager.DO_NOTHING_TIMER;
+ private Timer sortTimer = DoNothingMetricManager.DO_NOTHING_TIMER;
+ private Timer applyTimer = DoNothingMetricManager.DO_NOTHING_TIMER;
+ private static final String DESERIALIZE = "deserialize";
+ private static final String SORT = "sort";
+ private static final String APPLY = "apply";
public IoTConsensusServerMetrics(IoTConsensusServerImpl impl) {
this.impl = impl;
}
private static final String IMPL = "ioTConsensusServerImpl";
+ public void recordDeserializeCost(long costTimeInNanos) {
+ deserializeTimer.updateNanos(costTimeInNanos);
+ }
+
+ public void recordSortCost(long costTimeInNanos) {
+ sortTimer.updateNanos(costTimeInNanos);
+ }
+
+ public void recordApplyCost(long costTimeInNanos) {
+ applyTimer.updateNanos(costTimeInNanos);
+ }
+
@Override
public void bindTo(AbstractMetricService metricService) {
bindAutoGauge(metricService);
bindStageHistogram(metricService);
+ bindSyncLogTimer(metricService);
Review Comment:
I was wondering if we could unify this into Histogram or timer, otherwise it seems like there would be some difference in the way they are currently computed? May cause the time does not correspond exactly, right? @SpriCoder What's your opinion?
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/iot/service/IoTConsensusRPCServiceProcessor.java:
##########
@@ -115,8 +115,11 @@ public void syncLogEntries(
: ByteBufferConsensusRequest::new)
.collect(Collectors.toList())));
}
+ long buildRequestTime = System.nanoTime();
IConsensusRequest deserializedRequest =
impl.getStateMachine().deserializeRequest(logEntriesInThisBatch);
+ impl.getIoTConsensusServerMetrics()
Review Comment:
Do we need to divide by the number of logs?
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/iot/IoTConsensusServerImpl.java:
##########
@@ -909,6 +916,7 @@ private TSStatus cacheAndInsertLatestNode(DeserializedBatchIndexedConsensusReque
for (IConsensusRequest insertNode : request.getInsertNodes()) {
subStatus.add(stateMachine.write(insertNode));
}
+ ioTConsensusServerMetrics.recordApplyCost(System.nanoTime() - sortTime);
Review Comment:
Should we count the time for each IConsensusRequest, otherwise it's not the time of applying the state machine for a single request
--
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 #11134: [IOTDB-6152] Add IoTConsensus Sync Log Timer
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum merged PR #11134:
URL: https://github.com/apache/iotdb/pull/11134
--
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 #11134: [IOTDB-6152] Add IoTConsensus Sync Log Timer
Posted by "BUAAserein (via GitHub)" <gi...@apache.org>.
BUAAserein commented on code in PR #11134:
URL: https://github.com/apache/iotdb/pull/11134#discussion_r1324314031
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/iot/service/IoTConsensusRPCServiceProcessor.java:
##########
@@ -115,8 +115,11 @@ public void syncLogEntries(
: ByteBufferConsensusRequest::new)
.collect(Collectors.toList())));
}
+ long buildRequestTime = System.nanoTime();
IConsensusRequest deserializedRequest =
impl.getStateMachine().deserializeRequest(logEntriesInThisBatch);
+ impl.getIoTConsensusServerMetrics()
Review Comment:
Yes.
--
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 #11134: [IOTDB-6152] Add IoTConsensus Sync Log Timer
Posted by "BUAAserein (via GitHub)" <gi...@apache.org>.
BUAAserein commented on code in PR #11134:
URL: https://github.com/apache/iotdb/pull/11134#discussion_r1324441896
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/service/metric/enums/Metric.java:
##########
@@ -45,6 +45,7 @@ public enum Metric {
// consensus related
STAGE("stage"),
IOT_CONSENSUS("iot_consensus"),
+ IOT_SYNC_LOG("iot_sync_log"),
Review Comment:
ok
--
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 commented on a diff in pull request #11134: [IOTDB-6152] Add IoTConsensus Sync Log Timer
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #11134:
URL: https://github.com/apache/iotdb/pull/11134#discussion_r1324426887
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/service/metric/enums/Metric.java:
##########
@@ -45,6 +45,7 @@ public enum Metric {
// consensus related
STAGE("stage"),
IOT_CONSENSUS("iot_consensus"),
+ IOT_SYNC_LOG("iot_sync_log"),
Review Comment:
iot_receive_log?
--
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 #11134: [IOTDB-6152] Add IoTConsensus Sync Log Timer
Posted by "BUAAserein (via GitHub)" <gi...@apache.org>.
BUAAserein commented on code in PR #11134:
URL: https://github.com/apache/iotdb/pull/11134#discussion_r1324313637
##########
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/iot/IoTConsensusServerImpl.java:
##########
@@ -909,6 +916,7 @@ private TSStatus cacheAndInsertLatestNode(DeserializedBatchIndexedConsensusReque
for (IConsensusRequest insertNode : request.getInsertNodes()) {
subStatus.add(stateMachine.write(insertNode));
}
+ ioTConsensusServerMetrics.recordApplyCost(System.nanoTime() - sortTime);
Review Comment:
Yes. Now it's the time of one Batch. We should divide it to calculate for every single request.
--
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