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 2021/12/29 08:30:34 UTC
[GitHub] [iotdb] HeimingZ opened a new pull request #4659: [To rel/0.12][IOTDB-1297] Refactor the memory control when enabling time partitions
HeimingZ opened a new pull request #4659:
URL: https://github.com/apache/iotdb/pull/4659
wait awhile for bytebuffers ready when creating wal node and meeting OOM
--
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] JackieTien97 merged pull request #4659: [To rel/0.12][IOTDB-1297] Refactor the memory control when enabling time partitions
Posted by GitBox <gi...@apache.org>.
JackieTien97 merged pull request #4659:
URL: https://github.com/apache/iotdb/pull/4659
--
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] HeimingZ commented on a change in pull request #4659: [To rel/0.12][IOTDB-1297] Refactor the memory control when enabling time partitions
Posted by GitBox <gi...@apache.org>.
HeimingZ commented on a change in pull request #4659:
URL: https://github.com/apache/iotdb/pull/4659#discussion_r777166012
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/StorageGroupProcessor.java
##########
@@ -252,7 +252,7 @@
private final Deque<ByteBuffer> walByteBufferPool = new LinkedList<>();
- private int currentWalPoolSize = 0;
+ private AtomicInteger currentWalPoolSize = new AtomicInteger(0);
Review comment:
I didn't see the synchronized key word. Already fixed.
--
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] HeimingZ commented on a change in pull request #4659: [To rel/0.12][IOTDB-1297] Refactor the memory control when enabling time partitions
Posted by GitBox <gi...@apache.org>.
HeimingZ commented on a change in pull request #4659:
URL: https://github.com/apache/iotdb/pull/4659#discussion_r777166114
##########
File path: server/src/main/java/org/apache/iotdb/db/writelog/manager/MultiFileLogNodeManager.java
##########
@@ -46,6 +46,12 @@
public class MultiFileLogNodeManager implements WriteLogNodeManager, IService {
private static final Logger logger = LoggerFactory.getLogger(MultiFileLogNodeManager.class);
+ // if OOM occurs when registering bytebuffer, getNode method will sleep awhile and then try again
+ private static final int REGISTER_BUFFER_SLEEP_INTERVAL = 20;
+ // if total sleep time exceeds this, getNode method will reject this write and throw original
+ // exception.
+ private static final int REGISTER_BUFFER_REJECT_THRESHOLD = 1_000;
Review comment:
I add ms unit for them. But, I think it's bad to make these threshold params configurable, because they may make users confused.
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/StorageGroupProcessor.java
##########
@@ -298,9 +298,21 @@
} else {
// if the queue is empty and current size is less than MAX_BYTEBUFFER_NUM
// we can construct another two more new byte buffer
- currentWalPoolSize += 2;
- res[0] = ByteBuffer.allocateDirect(WAL_BUFFER_SIZE);
- res[1] = ByteBuffer.allocateDirect(WAL_BUFFER_SIZE);
+ try {
+ currentWalPoolSize.getAndAdd(2);
+ res[0] = ByteBuffer.allocateDirect(WAL_BUFFER_SIZE);
+ res[1] = ByteBuffer.allocateDirect(WAL_BUFFER_SIZE);
+ } catch (OutOfMemoryError e) {
+ if (res[0] != null) {
+ MmapUtil.clean((MappedByteBuffer) res[0]);
+ currentWalPoolSize.getAndDecrement();
+ }
+ if (res[1] != null) {
+ MmapUtil.clean((MappedByteBuffer) res[1]);
+ currentWalPoolSize.getAndDecrement();
+ }
+ throw e;
Review comment:
fixed
--
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] JackieTien97 commented on a change in pull request #4659: [To rel/0.12][IOTDB-1297] Refactor the memory control when enabling time partitions
Posted by GitBox <gi...@apache.org>.
JackieTien97 commented on a change in pull request #4659:
URL: https://github.com/apache/iotdb/pull/4659#discussion_r777156332
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/StorageGroupProcessor.java
##########
@@ -252,7 +252,7 @@
private final Deque<ByteBuffer> walByteBufferPool = new LinkedList<>();
- private int currentWalPoolSize = 0;
+ private AtomicInteger currentWalPoolSize = new AtomicInteger(0);
Review comment:
Why change the type from `int` to `AtomicInteger`? `synchronized` key word has already ensure the concurrent safety
##########
File path: server/src/main/java/org/apache/iotdb/db/writelog/manager/MultiFileLogNodeManager.java
##########
@@ -46,6 +46,12 @@
public class MultiFileLogNodeManager implements WriteLogNodeManager, IService {
private static final Logger logger = LoggerFactory.getLogger(MultiFileLogNodeManager.class);
+ // if OOM occurs when registering bytebuffer, getNode method will sleep awhile and then try again
+ private static final int REGISTER_BUFFER_SLEEP_INTERVAL = 20;
+ // if total sleep time exceeds this, getNode method will reject this write and throw original
+ // exception.
+ private static final int REGISTER_BUFFER_REJECT_THRESHOLD = 1_000;
Review comment:
Make these two threshold configurable, and indicate theirs unit in the comment, like `ms` or `s`.
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/StorageGroupProcessor.java
##########
@@ -298,9 +298,21 @@
} else {
// if the queue is empty and current size is less than MAX_BYTEBUFFER_NUM
// we can construct another two more new byte buffer
- currentWalPoolSize += 2;
- res[0] = ByteBuffer.allocateDirect(WAL_BUFFER_SIZE);
- res[1] = ByteBuffer.allocateDirect(WAL_BUFFER_SIZE);
+ try {
+ currentWalPoolSize.getAndAdd(2);
+ res[0] = ByteBuffer.allocateDirect(WAL_BUFFER_SIZE);
+ res[1] = ByteBuffer.allocateDirect(WAL_BUFFER_SIZE);
+ } catch (OutOfMemoryError e) {
+ if (res[0] != null) {
+ MmapUtil.clean((MappedByteBuffer) res[0]);
+ currentWalPoolSize.getAndDecrement();
+ }
+ if (res[1] != null) {
+ MmapUtil.clean((MappedByteBuffer) res[1]);
+ currentWalPoolSize.getAndDecrement();
+ }
+ throw e;
Review comment:
return null instead of throw this exception again. Exception handling is much more expensive. Since you have already catch exception here, you can return null here.
##########
File path: server/src/main/java/org/apache/iotdb/db/writelog/manager/MultiFileLogNodeManager.java
##########
@@ -90,9 +96,39 @@ public WriteLogNode getNode(String identifier, Supplier<ByteBuffer[]> supplier)
node = new ExclusiveWriteLogNode(identifier);
WriteLogNode oldNode = nodeMap.putIfAbsent(identifier, node);
if (oldNode != null) {
- return oldNode;
+ node = oldNode;
} else {
- node.initBuffer(supplier.get());
+ ByteBuffer[] buffers = null;
+ int sleepTimeInMs = 0;
+ // try to get bytebuffer repeatedly
+ while (buffers == null) {
+ try {
+ buffers = supplier.get();
+ } catch (OutOfMemoryError e) {
+ // log error if this is the first time
+ if (sleepTimeInMs == 0) {
+ logger.error(
+ "OOM occurs when allocate bytebuffer for wal, please reduce wal_buffer_size or storage groups number",
+ e);
+ }
+ // sleep awhile and then try again
+ try {
+ Thread.sleep(REGISTER_BUFFER_SLEEP_INTERVAL);
+ sleepTimeInMs += REGISTER_BUFFER_SLEEP_INTERVAL;
+ } catch (InterruptedException interruptedException) {
+ e.addSuppressed(interruptedException);
+ nodeMap.remove(identifier);
+ throw e;
+ }
+ // sleep too long, throw exception
+ if (sleepTimeInMs >= REGISTER_BUFFER_REJECT_THRESHOLD) {
+ nodeMap.remove(identifier);
+ throw e;
+ }
Review comment:
just judge whether the buffers is null, and no need to catch OOM here.
--
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] HeimingZ commented on a change in pull request #4659: [To rel/0.12][IOTDB-1297] Refactor the memory control when enabling time partitions
Posted by GitBox <gi...@apache.org>.
HeimingZ commented on a change in pull request #4659:
URL: https://github.com/apache/iotdb/pull/4659#discussion_r777166118
##########
File path: server/src/main/java/org/apache/iotdb/db/writelog/manager/MultiFileLogNodeManager.java
##########
@@ -90,9 +96,39 @@ public WriteLogNode getNode(String identifier, Supplier<ByteBuffer[]> supplier)
node = new ExclusiveWriteLogNode(identifier);
WriteLogNode oldNode = nodeMap.putIfAbsent(identifier, node);
if (oldNode != null) {
- return oldNode;
+ node = oldNode;
} else {
- node.initBuffer(supplier.get());
+ ByteBuffer[] buffers = null;
+ int sleepTimeInMs = 0;
+ // try to get bytebuffer repeatedly
+ while (buffers == null) {
+ try {
+ buffers = supplier.get();
+ } catch (OutOfMemoryError e) {
+ // log error if this is the first time
+ if (sleepTimeInMs == 0) {
+ logger.error(
+ "OOM occurs when allocate bytebuffer for wal, please reduce wal_buffer_size or storage groups number",
+ e);
+ }
+ // sleep awhile and then try again
+ try {
+ Thread.sleep(REGISTER_BUFFER_SLEEP_INTERVAL);
+ sleepTimeInMs += REGISTER_BUFFER_SLEEP_INTERVAL;
+ } catch (InterruptedException interruptedException) {
+ e.addSuppressed(interruptedException);
+ nodeMap.remove(identifier);
+ throw e;
+ }
+ // sleep too long, throw exception
+ if (sleepTimeInMs >= REGISTER_BUFFER_REJECT_THRESHOLD) {
+ nodeMap.remove(identifier);
+ throw e;
+ }
Review comment:
fixed
--
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