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/07/23 02:56:37 UTC
[GitHub] [incubator-iotdb] Ring-k opened a new pull request #1544: [IOTDB-810] Set Configuration of Max Number of Logs in Memory
Ring-k opened a new pull request #1544:
URL: https://github.com/apache/incubator-iotdb/pull/1544
----------------------------------------------------------------
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] [incubator-iotdb] LebronAl commented on pull request #1544: [IOTDB-810] Set Configuration of Max Number of Logs in Memory
Posted by GitBox <gi...@apache.org>.
LebronAl commented on pull request #1544:
URL: https://github.com/apache/incubator-iotdb/pull/1544#issuecomment-662844003
I think when the max_num_of_logs_in_mem threshold is reached we should delete the logs in memory to min_num_of_logs_in_mem rather than throwing an exception to the client.
These two configurations `max_num_of_logs_in_mem` and `log_deletion_check_interval_second ` are both used to ensure that the Raft logs did not take up whole memory rather than slow down the write rate for the client.
----------------------------------------------------------------
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] [incubator-iotdb] Ring-k commented on a change in pull request #1544: [IOTDB-810] Set Configuration of Max Number of Logs in Memory
Posted by GitBox <gi...@apache.org>.
Ring-k commented on a change in pull request #1544:
URL: https://github.com/apache/incubator-iotdb/pull/1544#discussion_r460035451
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/exception/LogNumberOutOfBoundException.java
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iotdb.cluster.exception;
+
+public class LogNumberOutOfBoundException extends Exception{
Review comment:
Thanks for your advice. I will remove 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
[GitHub] [incubator-iotdb] LebronAl edited a comment on pull request #1544: [IOTDB-810] Set Configuration of Max Number of Logs in Memory
Posted by GitBox <gi...@apache.org>.
LebronAl edited a comment on pull request #1544:
URL: https://github.com/apache/incubator-iotdb/pull/1544#issuecomment-662844003
I think when the max_num_of_logs_in_mem threshold is reached we should delete the logs to min_num_of_logs_in_mem rather than throwing an exception to the client.
These two configurations `max_num_of_logs_in_mem` and `log_deletion_check_interval_second ` are both used to ensure that the Raft logs did not take up whole memory rather than slow down the write rate for the client.
----------------------------------------------------------------
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] [incubator-iotdb] Ring-k commented on a change in pull request #1544: [IOTDB-810] Set Configuration of Max Number of Logs in Memory
Posted by GitBox <gi...@apache.org>.
Ring-k commented on a change in pull request #1544:
URL: https://github.com/apache/incubator-iotdb/pull/1544#discussion_r460055115
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/log/manage/RaftLogManager.java
##########
@@ -415,6 +423,9 @@ public void commitTo(long newCommitIndex, boolean ignoreExecutionExceptions)
.clear();
}
try {
+ if (committedEntryManager.getTotalSize() + entries.size() > maxNumOfLogsInMem) {
+ throw new LogNumberOutOfBoundException();
+ }
Review comment:
I see. Thanks for your suggestion.
----------------------------------------------------------------
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] [incubator-iotdb] jt2594838 merged pull request #1544: [IOTDB-810] Set Configuration of Max Number of Logs in Memory
Posted by GitBox <gi...@apache.org>.
jt2594838 merged pull request #1544:
URL: https://github.com/apache/incubator-iotdb/pull/1544
----------------------------------------------------------------
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] [incubator-iotdb] jt2594838 commented on a change in pull request #1544: [IOTDB-810] Set Configuration of Max Number of Logs in Memory
Posted by GitBox <gi...@apache.org>.
jt2594838 commented on a change in pull request #1544:
URL: https://github.com/apache/incubator-iotdb/pull/1544#discussion_r459850270
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/config/ClusterConfig.java
##########
@@ -63,7 +63,12 @@
/**
* max number of committed logs to be saved
*/
- private int maxNumberOfLogs = 100;
+ private int minNumOfLogsInMem = 100;
+
+ /**
+ * max number of committed logs in memory
+ */
+ private int maxNumOfLogsInMem = 100;
Review comment:
Maybe this should be 1000 or at least a different number from min.
##########
File path: cluster/src/assembly/resources/conf/iotdb-cluster.properties
##########
@@ -66,8 +66,11 @@ max_unsnapshoted_log_size=134217728
# whether to use batch append entries in log catch up
use_batch_in_catch_up=true
-# max number of committed logs to be saved
-max_number_of_logs=100
+# minimum number of committed logs in memory
+min_num_of_logs_in_mem=100
+
+# maximum number of committed logs in memory
+max_num_of_logs_in_mem=1000
Review comment:
Add more specific comments like
"# minimum number of committed logs in memory, after each log deletion, at most such number of logs will remain in memory. Increasing the number will reduce the chance to use snapshot in catch-ups, but will also increase memory footprint"
"# maximum number of committed logs in memory, when reached, a log deletion will be triggered. Increasing the number will reduce the chance to use snapshot in catch-ups, but will also increase memory footprint"
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/exception/LogNumberOutOfBoundException.java
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iotdb.cluster.exception;
+
+public class LogNumberOutOfBoundException extends Exception{
Review comment:
This exception is unneeded, and we should solve it silently by triggering a log deletion.
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/log/manage/RaftLogManager.java
##########
@@ -415,6 +423,9 @@ public void commitTo(long newCommitIndex, boolean ignoreExecutionExceptions)
.clear();
}
try {
+ if (committedEntryManager.getTotalSize() + entries.size() > maxNumOfLogsInMem) {
+ throw new LogNumberOutOfBoundException();
+ }
Review comment:
You should trigger a log deletion and proceed the committing instead of throwing an exception.
----------------------------------------------------------------
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] [incubator-iotdb] Ring-k commented on pull request #1544: [IOTDB-810] Set Configuration of Max Number of Logs in Memory
Posted by GitBox <gi...@apache.org>.
Ring-k commented on pull request #1544:
URL: https://github.com/apache/incubator-iotdb/pull/1544#issuecomment-663809312
> I think when the max_num_of_logs_in_mem threshold is reached we should delete the logs to min_num_of_logs_in_mem rather than throwing an exception to the client.
> These two configurations `max_num_of_logs_in_mem` and `log_deletion_check_interval_second ` are both used to ensure that the Raft logs did not take up whole memory rather than slow down the write rate for the client.
I got you. I will change these logics.
----------------------------------------------------------------
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] [incubator-iotdb] Ring-k commented on a change in pull request #1544: [IOTDB-810] Set Configuration of Max Number of Logs in Memory
Posted by GitBox <gi...@apache.org>.
Ring-k commented on a change in pull request #1544:
URL: https://github.com/apache/incubator-iotdb/pull/1544#discussion_r460036067
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/config/ClusterConfig.java
##########
@@ -63,7 +63,12 @@
/**
* max number of committed logs to be saved
*/
- private int maxNumberOfLogs = 100;
+ private int minNumOfLogsInMem = 100;
+
+ /**
+ * max number of committed logs in memory
+ */
+ private int maxNumOfLogsInMem = 100;
Review comment:
Sorry for the mistake. I will correct 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
[GitHub] [incubator-iotdb] Ring-k commented on a change in pull request #1544: [IOTDB-810] Set Configuration of Max Number of Logs in Memory
Posted by GitBox <gi...@apache.org>.
Ring-k commented on a change in pull request #1544:
URL: https://github.com/apache/incubator-iotdb/pull/1544#discussion_r460035651
##########
File path: cluster/src/assembly/resources/conf/iotdb-cluster.properties
##########
@@ -66,8 +66,11 @@ max_unsnapshoted_log_size=134217728
# whether to use batch append entries in log catch up
use_batch_in_catch_up=true
-# max number of committed logs to be saved
-max_number_of_logs=100
+# minimum number of committed logs in memory
+min_num_of_logs_in_mem=100
+
+# maximum number of committed logs in memory
+max_num_of_logs_in_mem=1000
Review comment:
Sure, that will be more clear.
----------------------------------------------------------------
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