You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2020/10/15 07:14:45 UTC
[GitHub] [iotdb] neuyilan opened a new pull request #1827: [Distributed]fix code smell
neuyilan opened a new pull request #1827:
URL: https://github.com/apache/iotdb/pull/1827
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] neuyilan commented on a change in pull request #1827: [Distributed]fix code smell
Posted by GitBox <gi...@apache.org>.
neuyilan commented on a change in pull request #1827:
URL: https://github.com/apache/iotdb/pull/1827#discussion_r505994733
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/log/LogDispatcher.java
##########
@@ -244,25 +244,20 @@ private void appendEntriesAsync(List<ByteBuffer> logList, AppendEntriesRequest r
private void appendEntriesSync(List<ByteBuffer> logList, AppendEntriesRequest request,
List<SendLogRequest> currBatch) {
- long start;
- if (Timer.ENABLE_INSTRUMENTING) {
- start = System.nanoTime();
- }
+ Timer.Statistic.RAFT_SENDER_WAIT_FOR_PREV_LOG.setStartTime();
if (!member.waitForPrevLog(peer, currBatch.get(0).getLog())) {
logger.warn("{}: node {} timed out when appending {}", member.getName(), receiver,
currBatch.get(0).getLog());
return;
}
- Timer.Statistic.RAFT_SENDER_WAIT_FOR_PREV_LOG.addNanoFromStart(start);
+ Timer.Statistic.RAFT_SENDER_WAIT_FOR_PREV_LOG.calCostTime();
Review comment:
you're right, its indeed cause concurrent problem, I'll fix it. thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] jt2594838 commented on a change in pull request #1827: [Distributed]fix code smell
Posted by GitBox <gi...@apache.org>.
jt2594838 commented on a change in pull request #1827:
URL: https://github.com/apache/iotdb/pull/1827#discussion_r505531952
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/log/LogDispatcher.java
##########
@@ -244,25 +244,20 @@ private void appendEntriesAsync(List<ByteBuffer> logList, AppendEntriesRequest r
private void appendEntriesSync(List<ByteBuffer> logList, AppendEntriesRequest request,
List<SendLogRequest> currBatch) {
- long start;
- if (Timer.ENABLE_INSTRUMENTING) {
- start = System.nanoTime();
- }
+ Timer.Statistic.RAFT_SENDER_WAIT_FOR_PREV_LOG.setStartTime();
if (!member.waitForPrevLog(peer, currBatch.get(0).getLog())) {
logger.warn("{}: node {} timed out when appending {}", member.getName(), receiver,
currBatch.get(0).getLog());
return;
}
- Timer.Statistic.RAFT_SENDER_WAIT_FOR_PREV_LOG.addNanoFromStart(start);
+ Timer.Statistic.RAFT_SENDER_WAIT_FOR_PREV_LOG.calCostTime();
Review comment:
I don't think the change is thread-safe, please confirm it.
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/Timer.java
##########
@@ -183,14 +184,33 @@ public void add(long val) {
}
/**
- * This method equals `add(System.nanoTime() - start)`. We wrap `System.nanoTime()` in this
- * method to avoid unnecessary calls when instrumenting is disabled.
- *
- * @param start
+ * set now time as the operation's start time
*/
- public void addNanoFromStart(long start) {
+ public void setStartTime() {
+ setStartTime(System.nanoTime());
+ }
+
+ public void setStartTime(long startTime) {
+ if (ENABLE_INSTRUMENTING) {
+ this.startTime = startTime;
+ }
+ }
Review comment:
The condition should be put into the first method, as `System.nanoTime()` is a heavy call and should be avoided when not using instrumenting.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] jt2594838 merged pull request #1827: [Distributed]fix code smell
Posted by GitBox <gi...@apache.org>.
jt2594838 merged pull request #1827:
URL: https://github.com/apache/iotdb/pull/1827
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] neuyilan commented on a change in pull request #1827: [Distributed]fix code smell
Posted by GitBox <gi...@apache.org>.
neuyilan commented on a change in pull request #1827:
URL: https://github.com/apache/iotdb/pull/1827#discussion_r505994859
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/Timer.java
##########
@@ -183,14 +184,33 @@ public void add(long val) {
}
/**
- * This method equals `add(System.nanoTime() - start)`. We wrap `System.nanoTime()` in this
- * method to avoid unnecessary calls when instrumenting is disabled.
- *
- * @param start
+ * set now time as the operation's start time
*/
- public void addNanoFromStart(long start) {
+ public void setStartTime() {
+ setStartTime(System.nanoTime());
+ }
+
+ public void setStartTime(long startTime) {
+ if (ENABLE_INSTRUMENTING) {
+ this.startTime = startTime;
+ }
+ }
Review comment:
Got it
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org