You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ratis.apache.org by GitBox <gi...@apache.org> on 2020/12/03 08:02:37 UTC
[GitHub] [incubator-ratis] runzhiwang opened a new pull request #317: RATIS-1198. Fix OOM when use FileStore to write 1GB file
runzhiwang opened a new pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317
## What changes were proposed in this pull request?
**What's the problem ?**
Use FileStore write 1GB file, OOM happens.
**How to reproduce ?**
1. change timeout 100 seconds to 600 seconds.
public int getGlobalTimeoutSeconds() {
return 100;
}
2. change 1M to 50M
testMultipleFiles("file", 20, SizeInBytes.valueOf("1M"), newClient);
**What's the reason ?**
1. In FileStore, `CACHING_ENABLED_DEFAULT`, i.e. `stateMachineCachingEnabled` is false, so cache will append the complete LogEntryProto into cache, without remove StateMachineData.
```
if (stateMachineCachingEnabled) {
// The stateMachineData will be cached inside the StateMachine itself.
cache.appendEntry(ServerProtoUtils.removeStateMachineData(entry));
} else {
cache.appendEntry(entry);
}
```
2. But when getEntrySize, it does not calculate the size of StateMachineData
```
static long getEntrySize(LogEntryProto entry) {
final int serialized = ServerProtoUtils.removeStateMachineData(entry).getSerializedSize();
return serialized + CodedOutputStream.computeUInt32SizeNoTag(serialized) + 4L;
}
```
3. So `isSegmentFull` is false, and can not evict cache, then LogSegment#entryCache keep too many LogEntryProto, which contains the StateMachineData, so OOM happens.
if (isSegmentFull(currentOpenSegment, entry)) {
cache.rollOpenSegment(true);
fileLogWorker.rollLogSegment(currentOpenSegment);
checkAndEvictCache();
}
**How to fix ?**
If `stateMachineCachingEnabled` is false, we calculate entry size without removeStateMachineData.
```
static long getEntrySize(LogEntryProto entry, boolean stateMachineCachingEnabled) {
final int serialized = stateMachineCachingEnabled ?
ServerProtoUtils.removeStateMachineData(entry).getSerializedSize() :
entry.getSerializedSize();
return serialized + CodedOutputStream.computeUInt32SizeNoTag(serialized) + 4L;
}
```
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1198
## How was this patch tested?
test it manually
1. change timeout 100 seconds to 600 seconds.
public int getGlobalTimeoutSeconds() {
return 100;
}
2. change 1M to 50M
testMultipleFiles("file", 20, SizeInBytes.valueOf("1M"), newClient);
----------------------------------------------------------------
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-ratis] szetszwo commented on a change in pull request #317: [WIP]RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#discussion_r535185973
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
##########
@@ -388,10 +388,12 @@ public TermIndex getLastEntryTermIndex() {
try(AutoCloseableLock writeLock = writeLock()) {
validateLogEntry(entry);
final LogSegment currentOpenSegment = cache.getOpenSegment();
+ // The stateMachineData will be cached inside the StateMachine itself when enable stateMachineCaching.
+ LogEntryProto toAppendEntry = stateMachineCachingEnabled? ServerProtoUtils.removeStateMachineData(entry) : entry;
if (currentOpenSegment == null) {
cache.addOpenSegment(entry.getIndex());
fileLogWorker.startLogSegment(entry.getIndex());
- } else if (isSegmentFull(currentOpenSegment, entry)) {
+ } else if (isSegmentFull(currentOpenSegment, toAppendEntry)) {
Review comment:
For this case, it should removeStateMachineData no matter stateMachineCachingEnabled is true or false. How about the following?
```
+++ b/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
@@ -385,13 +385,14 @@ public class SegmentedRaftLog extends RaftLog {
if (LOG.isTraceEnabled()) {
LOG.trace("{}: appendEntry {}", getName(), ServerProtoUtils.toLogEntryString(entry));
}
+ final LogEntryProto removedStateMachineData = ServerProtoUtils.removeStateMachineData(entry);
try(AutoCloseableLock writeLock = writeLock()) {
validateLogEntry(entry);
final LogSegment currentOpenSegment = cache.getOpenSegment();
if (currentOpenSegment == null) {
cache.addOpenSegment(entry.getIndex());
fileLogWorker.startLogSegment(entry.getIndex());
- } else if (isSegmentFull(currentOpenSegment, entry)) {
+ } else if (isSegmentFull(currentOpenSegment, removedStateMachineData)) {
cache.rollOpenSegment(true);
fileLogWorker.rollLogSegment(currentOpenSegment);
checkAndEvictCache();
@@ -414,7 +415,7 @@ public class SegmentedRaftLog extends RaftLog {
fileLogWorker.writeLogEntry(entry).getFuture();
if (stateMachineCachingEnabled) {
// The stateMachineData will be cached inside the StateMachine itself.
- cache.appendEntry(ServerProtoUtils.removeStateMachineData(entry));
+ cache.appendEntry(removedStateMachineData));
} else {
cache.appendEntry(entry);
}
```
----------------------------------------------------------------
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-ratis] runzhiwang closed pull request #317: RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317
----------------------------------------------------------------
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-ratis] runzhiwang commented on a change in pull request #317: [WIP]RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#discussion_r535217053
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
##########
@@ -388,10 +388,12 @@ public TermIndex getLastEntryTermIndex() {
try(AutoCloseableLock writeLock = writeLock()) {
validateLogEntry(entry);
final LogSegment currentOpenSegment = cache.getOpenSegment();
+ // The stateMachineData will be cached inside the StateMachine itself when enable stateMachineCaching.
+ LogEntryProto toAppendEntry = stateMachineCachingEnabled? ServerProtoUtils.removeStateMachineData(entry) : entry;
if (currentOpenSegment == null) {
cache.addOpenSegment(entry.getIndex());
fileLogWorker.startLogSegment(entry.getIndex());
- } else if (isSegmentFull(currentOpenSegment, entry)) {
+ } else if (isSegmentFull(currentOpenSegment, toAppendEntry)) {
Review comment:
@szetszwo #317 (comment) need a minor change, otherwise it maybe add more entry in the open segment if stateMachineCachingEnabled is false. In the `else` branch of `isSegmentFull` , we pass `LogSegment.Op.WRITE_SEGMENT_FILE`, which count the size of entry after remove StateMachineData, but the segment.getTotalSize() is the total size of entry without remove StateMachineData. So `segment.getTotalSize() + entrySize > segmentMaxSize` becomes (total size of entries without remove StateMachineData) + (size of entry after remove StateMachineData) > segmentMaxSize
```
@@ -431,7 +432,7 @@ public class SegmentedRaftLog extends RaftLog {
if (segment.getTotalSize() >= segmentMaxSize) {
return true;
} else {
- final long entrySize = LogSegment.getEntrySize(entry);
+ final long entrySize = LogSegment.getEntrySize(entry, LogSegment.Op.WRITE_SEGMENT_FILE);
// if entry size is greater than the max segment size, write it directly
// into the current segment
return entrySize <= segmentMaxSize &&
segment.getTotalSize() + entrySize > segmentMaxSize;
```
----------------------------------------------------------------
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-ratis] szetszwo commented on a change in pull request #317: RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#discussion_r535039898
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/LogSegment.java
##########
@@ -231,6 +235,7 @@ private File getSegmentFile() {
private volatile long endIndex;
private RaftStorage storage;
private RaftLogMetrics raftLogMetrics;
+ private final boolean stateMachineCachingEnabled;
Review comment:
Let's pass the information in the method calls instead of add stateMachineCachingEnabled to LogSegment.
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
##########
@@ -431,7 +431,7 @@ private boolean isSegmentFull(LogSegment segment, LogEntryProto entry) {
if (segment.getTotalSize() >= segmentMaxSize) {
return true;
} else {
- final long entrySize = LogSegment.getEntrySize(entry);
+ final long entrySize = LogSegment.getEntrySize(entry, stateMachineCachingEnabled);
Review comment:
Even if stateMachineCachingEnabled is false, the stateMachineData will be removed before writing the entry to the file. So, it should remove stateMachineData inside LogSegment.getEntrySize.
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/LogSegment.java
##########
@@ -60,8 +60,10 @@
static final Logger LOG = LoggerFactory.getLogger(LogSegment.class);
- static long getEntrySize(LogEntryProto entry) {
- final int serialized = ServerProtoUtils.removeStateMachineData(entry).getSerializedSize();
+ static long getEntrySize(LogEntryProto entry, boolean stateMachineCachingEnabled) {
Review comment:
There are different reasons to getEntrySize. stateMachineCachingEnabled alone cannot determine if it should removeStateMachineData.
----------------------------------------------------------------
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-ratis] runzhiwang commented on pull request #317: RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#issuecomment-737735251
@szetszwo Could you help review this ?
----------------------------------------------------------------
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-ratis] szetszwo commented on pull request #317: RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#issuecomment-737819776
Here is more details:
```
diff --git a/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/LogSegment.java b/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/LogSegment.java
index fb58a4a8..3bd3076d 100644
--- a/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/LogSegment.java
+++ b/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/LogSegment.java
@@ -60,8 +60,25 @@ public class LogSegment implements Comparable<Long> {
static final Logger LOG = LoggerFactory.getLogger(LogSegment.class);
- static long getEntrySize(LogEntryProto entry) {
- final int serialized = ServerProtoUtils.removeStateMachineData(entry).getSerializedSize();
+ enum Op {
+ LOAD_SEGMENT_FILE,
+ WRITE_SEGMENT_FILE,
+ WRITE_CACHE_WITH_STATE_MACHINE_CACHE,
+ WRITE_CACHE_WITHOUT_STATE_MACHINE_CACHE;
+ }
+
+ static long getEntrySize(LogEntryProto entry, Op op) {
+ LogEntryProto e = entry;
+ if (op == Op.WRITE_SEGMENT_FILE) {
+ e = ServerProtoUtils.removeStateMachineData(entry);
+ } else if (op == Op.LOAD_SEGMENT_FILE || op == Op.WRITE_CACHE_WITH_STATE_MACHINE_CACHE) {
+ Preconditions.assertTrue(entry == ServerProtoUtils.removeStateMachineData(entry),
+ () -> "Unexpected LogEntryProto with StateMachine data: op=" + op + ", entry=" + entry);
+ } else {
+ Preconditions.assertTrue(op == Op.WRITE_CACHE_WITHOUT_STATE_MACHINE_CACHE,
+ () -> "Unexpected op " + op + ", entry=" + entry);
+ }
+ final int serialized = e.getSerializedSize();
return serialized + CodedOutputStream.computeUInt32SizeNoTag(serialized) + 4L;
}
@@ -139,7 +156,7 @@ public class LogSegment implements Comparable<Long> {
final CorruptionPolicy corruptionPolicy = CorruptionPolicy.get(storage, RaftStorage::getLogCorruptionPolicy);
final int entryCount = readSegmentFile(file, start, end, isOpen, corruptionPolicy, raftLogMetrics, entry -> {
- segment.append(keepEntryInCache || isOpen, entry);
+ segment.append(keepEntryInCache || isOpen, entry, Op.LOAD_SEGMENT_FILE);
if (logConsumer != null) {
logConsumer.accept(entry);
}
@@ -273,12 +290,12 @@ public class LogSegment implements Comparable<Long> {
return CorruptionPolicy.get(storage, RaftStorage::getLogCorruptionPolicy);
}
- void appendToOpenSegment(LogEntryProto entry) {
+ void appendToOpenSegment(LogEntryProto entry, Op op) {
Preconditions.assertTrue(isOpen(), "The log segment %s is not open for append", this);
- append(true, entry);
+ append(true, entry, op);
}
- private void append(boolean keepEntryInCache, LogEntryProto entry) {
+ private void append(boolean keepEntryInCache, LogEntryProto entry, Op op) {
Objects.requireNonNull(entry, "entry == null");
if (records.isEmpty()) {
Preconditions.assertTrue(entry.getIndex() == startIndex,
@@ -300,7 +317,7 @@ public class LogSegment implements Comparable<Long> {
if (entry.hasConfigurationEntry()) {
configEntries.add(record.getTermIndex());
}
- totalSize += getEntrySize(entry);
+ totalSize += getEntrySize(entry, op);
endIndex = entry.getIndex();
}
diff --git a/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java b/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
index dbdbced9..ebbb00fa 100644
--- a/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
+++ b/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
@@ -414,9 +414,10 @@ public class SegmentedRaftLog extends RaftLog {
fileLogWorker.writeLogEntry(entry).getFuture();
if (stateMachineCachingEnabled) {
// The stateMachineData will be cached inside the StateMachine itself.
- cache.appendEntry(ServerProtoUtils.removeStateMachineData(entry));
+ cache.appendEntry(ServerProtoUtils.removeStateMachineData(entry),
+ LogSegment.Op.WRITE_CACHE_WITH_STATE_MACHINE_CACHE);
} else {
- cache.appendEntry(entry);
+ cache.appendEntry(entry, LogSegment.Op.WRITE_CACHE_WITHOUT_STATE_MACHINE_CACHE);
}
return writeFuture;
} catch (Exception e) {
@@ -431,7 +432,7 @@ public class SegmentedRaftLog extends RaftLog {
if (segment.getTotalSize() >= segmentMaxSize) {
return true;
} else {
- final long entrySize = LogSegment.getEntrySize(entry);
+ final long entrySize = LogSegment.getEntrySize(entry, LogSegment.Op.WRITE_SEGMENT_FILE);
// if entry size is greater than the max segment size, write it directly
// into the current segment
return entrySize <= segmentMaxSize &&
diff --git a/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogCache.java b/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogCache.java
index 260718f9..2177f890 100644
--- a/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogCache.java
+++ b/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogCache.java
@@ -521,11 +521,11 @@ public class SegmentedRaftLogCache {
closedSegments.get(closedSegments.size() - 1).getLastTermIndex());
}
- void appendEntry(LogEntryProto entry) {
+ void appendEntry(LogEntryProto entry, LogSegment.Op op) {
// SegmentedRaftLog does the segment creation/rolling work. Here we just
// simply append the entry into the open segment.
Preconditions.assertTrue(openSegment != null);
- openSegment.appendToOpenSegment(entry);
+ openSegment.appendToOpenSegment(entry, op);
}
/**
```
----------------------------------------------------------------
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-ratis] runzhiwang removed a comment on pull request #317: [WIP]RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
runzhiwang removed a comment on pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#issuecomment-737903046
Please wait for me.
----------------------------------------------------------------
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-ratis] szetszwo commented on a change in pull request #317: RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#discussion_r535879874
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/LogSegment.java
##########
@@ -390,8 +414,21 @@ int getLoadingTimes() {
return loadingTimes.get();
}
- synchronized void evictCache() {
+ void evictCache() {
entryCache.clear();
+ totalCacheSize.set(0);
+ }
+
+ void putEntryCache(TermIndex key, LogEntryProto value, Op op) {
+ entryCache.put(key, value);
+ totalCacheSize.getAndAdd(getEntrySize(value, op));
Review comment:
We should check previous.
```
final LogEntryProto previous = entryCache.put(key, value);
long previousSize = 0;
if (previous != null) {
previousSize = getEntrySize(value, Op.REMOVE_CACHE);
}
totalCacheSize.getAndAdd(getEntrySize(value, op) - previousSize);
```
Or
```
Preconditions.assertNull(previous, "previous");
```
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogCache.java
##########
@@ -167,8 +165,27 @@ int size() {
}
}
- long sizeInBytes() {
- return sizeInBytes;
+ long getTotalFileSize() {
+ try(AutoCloseableLock readLock = readLock()) {
+ long size = 0;
+ // TODO(runzhiwang): If there is performance problem, we do not need to re-calculate it every time
+ // because it is only for metric
+ for (LogSegment seg : segments) {
+ size += seg.getTotalFileSize();
+ }
+ return size;
+ }
+ }
Review comment:
We cannot control how often the metric call this method. We should cache it or use the original code. Let's do it separately.
----------------------------------------------------------------
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-ratis] szetszwo merged pull request #317: RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
szetszwo merged pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317
----------------------------------------------------------------
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-ratis] runzhiwang commented on pull request #317: [WIP]RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#issuecomment-737903046
Please wait for me.
----------------------------------------------------------------
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-ratis] szetszwo commented on a change in pull request #317: [WIP]RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#discussion_r535202962
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
##########
@@ -388,10 +388,12 @@ public TermIndex getLastEntryTermIndex() {
try(AutoCloseableLock writeLock = writeLock()) {
validateLogEntry(entry);
final LogSegment currentOpenSegment = cache.getOpenSegment();
+ // The stateMachineData will be cached inside the StateMachine itself when enable stateMachineCaching.
+ LogEntryProto toAppendEntry = stateMachineCachingEnabled? ServerProtoUtils.removeStateMachineData(entry) : entry;
if (currentOpenSegment == null) {
cache.addOpenSegment(entry.getIndex());
fileLogWorker.startLogSegment(entry.getIndex());
- } else if (isSegmentFull(currentOpenSegment, entry)) {
+ } else if (isSegmentFull(currentOpenSegment, toAppendEntry)) {
Review comment:
You are right. The simple fix won't. I guess we need this fix https://github.com/apache/incubator-ratis/pull/317#issuecomment-737819776 ?
----------------------------------------------------------------
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-ratis] runzhiwang commented on a change in pull request #317: [WIP]RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#discussion_r535217053
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
##########
@@ -388,10 +388,12 @@ public TermIndex getLastEntryTermIndex() {
try(AutoCloseableLock writeLock = writeLock()) {
validateLogEntry(entry);
final LogSegment currentOpenSegment = cache.getOpenSegment();
+ // The stateMachineData will be cached inside the StateMachine itself when enable stateMachineCaching.
+ LogEntryProto toAppendEntry = stateMachineCachingEnabled? ServerProtoUtils.removeStateMachineData(entry) : entry;
if (currentOpenSegment == null) {
cache.addOpenSegment(entry.getIndex());
fileLogWorker.startLogSegment(entry.getIndex());
- } else if (isSegmentFull(currentOpenSegment, entry)) {
+ } else if (isSegmentFull(currentOpenSegment, toAppendEntry)) {
Review comment:
@szetszwo #317 (comment) need a minor change, otherwise it maybe add more entry in the open segment if stateMachineCachingEnabled is false. In the `else` branch of `isSegmentFull` , we pass `LogSegment.Op.WRITE_SEGMENT_FILE`, which count the size of entry after remove StateMachineData, but the segment.getTotalSize() is the total size of entries without remove StateMachineData. So `segment.getTotalSize() + entrySize > segmentMaxSize` becomes `(total size of entries without remove StateMachineData) + (size of entry after remove StateMachineData) > segmentMaxSize`. So in `isSegmentFull`, we should distinguish stateMachineCachingEnabled is true or false
```
@@ -431,7 +432,7 @@ public class SegmentedRaftLog extends RaftLog {
if (segment.getTotalSize() >= segmentMaxSize) {
return true;
} else {
- final long entrySize = LogSegment.getEntrySize(entry);
+ final long entrySize = LogSegment.getEntrySize(entry, LogSegment.Op.WRITE_SEGMENT_FILE);
// if entry size is greater than the max segment size, write it directly
// into the current segment
return entrySize <= segmentMaxSize &&
segment.getTotalSize() + entrySize > segmentMaxSize;
```
----------------------------------------------------------------
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-ratis] runzhiwang commented on a change in pull request #317: [WIP]RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#discussion_r535198688
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
##########
@@ -388,10 +388,12 @@ public TermIndex getLastEntryTermIndex() {
try(AutoCloseableLock writeLock = writeLock()) {
validateLogEntry(entry);
final LogSegment currentOpenSegment = cache.getOpenSegment();
+ // The stateMachineData will be cached inside the StateMachine itself when enable stateMachineCaching.
+ LogEntryProto toAppendEntry = stateMachineCachingEnabled? ServerProtoUtils.removeStateMachineData(entry) : entry;
if (currentOpenSegment == null) {
cache.addOpenSegment(entry.getIndex());
fileLogWorker.startLogSegment(entry.getIndex());
- } else if (isSegmentFull(currentOpenSegment, entry)) {
+ } else if (isSegmentFull(currentOpenSegment, toAppendEntry)) {
Review comment:
@szetszwo The problem is `cache.appendEntry(entry)` will put entry with `StateMachineData` into `LogSegment#entryCache`. For example, each entry occupy 8KB after remove `StateMachineData`, each `StateMachineData` is 80M, the segment.size.max is 8MB, when `isSegmentFull` returns true, open segment has 1000 entry, and `LogSegment#entryCache` is 80M * 1000, i.e. 80GB.
```
if (stateMachineCachingEnabled) {
// The stateMachineData will be cached inside the StateMachine itself.
- cache.appendEntry(ServerProtoUtils.removeStateMachineData(entry));
+ cache.appendEntry(removedStateMachineData));
} else {
cache.appendEntry(entry);
}
```
----------------------------------------------------------------
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-ratis] runzhiwang commented on pull request #317: RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#issuecomment-737900305
@szetszwo Thanks the suggestions. I find a simpler fix.
----------------------------------------------------------------
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-ratis] runzhiwang removed a comment on pull request #317: RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
runzhiwang removed a comment on pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#issuecomment-737900305
@szetszwo Thanks the suggestions. I find a simpler fix.
----------------------------------------------------------------
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-ratis] runzhiwang commented on a change in pull request #317: [WIP]RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#discussion_r535217053
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
##########
@@ -388,10 +388,12 @@ public TermIndex getLastEntryTermIndex() {
try(AutoCloseableLock writeLock = writeLock()) {
validateLogEntry(entry);
final LogSegment currentOpenSegment = cache.getOpenSegment();
+ // The stateMachineData will be cached inside the StateMachine itself when enable stateMachineCaching.
+ LogEntryProto toAppendEntry = stateMachineCachingEnabled? ServerProtoUtils.removeStateMachineData(entry) : entry;
if (currentOpenSegment == null) {
cache.addOpenSegment(entry.getIndex());
fileLogWorker.startLogSegment(entry.getIndex());
- } else if (isSegmentFull(currentOpenSegment, entry)) {
+ } else if (isSegmentFull(currentOpenSegment, toAppendEntry)) {
Review comment:
@szetszwo #317 (comment) need a minor change, otherwise it maybe add more entry in the open segment if stateMachineCachingEnabled is false. In the `else` branch of `isSegmentFull` , we pass `LogSegment.Op.WRITE_SEGMENT_FILE`, which count the size of entry after remove StateMachineData, but the segment.getTotalSize() is the total size of entries without remove StateMachineData. So `segment.getTotalSize() + entrySize > segmentMaxSize` becomes `(total size of entries without remove StateMachineData) + (size of entry after remove StateMachineData) > segmentMaxSize`
```
@@ -431,7 +432,7 @@ public class SegmentedRaftLog extends RaftLog {
if (segment.getTotalSize() >= segmentMaxSize) {
return true;
} else {
- final long entrySize = LogSegment.getEntrySize(entry);
+ final long entrySize = LogSegment.getEntrySize(entry, LogSegment.Op.WRITE_SEGMENT_FILE);
// if entry size is greater than the max segment size, write it directly
// into the current segment
return entrySize <= segmentMaxSize &&
segment.getTotalSize() + entrySize > segmentMaxSize;
```
----------------------------------------------------------------
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-ratis] szetszwo commented on a change in pull request #317: [WIP]RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#discussion_r535224658
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
##########
@@ -388,10 +388,12 @@ public TermIndex getLastEntryTermIndex() {
try(AutoCloseableLock writeLock = writeLock()) {
validateLogEntry(entry);
final LogSegment currentOpenSegment = cache.getOpenSegment();
+ // The stateMachineData will be cached inside the StateMachine itself when enable stateMachineCaching.
+ LogEntryProto toAppendEntry = stateMachineCachingEnabled? ServerProtoUtils.removeStateMachineData(entry) : entry;
if (currentOpenSegment == null) {
cache.addOpenSegment(entry.getIndex());
fileLogWorker.startLogSegment(entry.getIndex());
- } else if (isSegmentFull(currentOpenSegment, entry)) {
+ } else if (isSegmentFull(currentOpenSegment, toAppendEntry)) {
Review comment:
I see. It is tricky. We actually have two sizes, file size and cache size, where
- file size always excludes state machine data, and
- cache size may or may not include state machine data depending on stateMachineCachingEnabled.
isSegmentFull(..) also has a bug when stateMachineCachingEnabled is false.
----------------------------------------------------------------
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-ratis] szetszwo commented on a change in pull request #317: RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#discussion_r535791223
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
##########
@@ -391,7 +391,10 @@ public TermIndex getLastEntryTermIndex() {
if (currentOpenSegment == null) {
cache.addOpenSegment(entry.getIndex());
fileLogWorker.startLogSegment(entry.getIndex());
- } else if (isSegmentFull(currentOpenSegment, entry)) {
+ } else if (isSegmentFull(currentOpenSegment, entry,
+ stateMachineCachingEnabled ?
+ LogSegment.Op.CHECK_SEGMENT_FILE_FULL_WITH_STATE_MACHINE_CACHE:
+ LogSegment.Op.CHECK_SEGMENT_FILE_FULL_WITHOUT_STATE_MACHINE_CACHE)) {
Review comment:
@runzhiwang , we have two segment sizes, segment file size and segment cache size. They can be different since
- for files, we always removeStateMachineData;
- for cache, it depends on stateMachineCachingEnabled.
These two sizes are different when stateMachineCachingEnabled == false.
The method isSegmentFull is actually asking isSegmentFileFull. Therefore, it should not care about stateMachineCachingEnabled. As you mentioned, it is a bug to use segment.getTotalSize() inside isSegmentFull. In order to fix the bug, we probably have to spit it into two methods
- segment.getTotalFileSize()
- segment.getTotalCacheSize()
----------------------------------------------------------------
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-ratis] runzhiwang commented on a change in pull request #317: [WIP]RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#discussion_r535217053
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
##########
@@ -388,10 +388,12 @@ public TermIndex getLastEntryTermIndex() {
try(AutoCloseableLock writeLock = writeLock()) {
validateLogEntry(entry);
final LogSegment currentOpenSegment = cache.getOpenSegment();
+ // The stateMachineData will be cached inside the StateMachine itself when enable stateMachineCaching.
+ LogEntryProto toAppendEntry = stateMachineCachingEnabled? ServerProtoUtils.removeStateMachineData(entry) : entry;
if (currentOpenSegment == null) {
cache.addOpenSegment(entry.getIndex());
fileLogWorker.startLogSegment(entry.getIndex());
- } else if (isSegmentFull(currentOpenSegment, entry)) {
+ } else if (isSegmentFull(currentOpenSegment, toAppendEntry)) {
Review comment:
@szetszwo #317 (comment) need a minor change, otherwise it maybe add more entry in the open segment if stateMachineCachingEnabled is false. In the `else` branch of `isSegmentFull` , we pass `LogSegment.Op.WRITE_SEGMENT_FILE`, which count the size of entry after remove StateMachineData, but the segment.getTotalSize() is the total size of entry without remove StateMachineData. So `segment.getTotalSize() + entrySize > segmentMaxSize` becomes `(total size of entries without remove StateMachineData) + (size of entry after remove StateMachineData) > segmentMaxSize`
```
@@ -431,7 +432,7 @@ public class SegmentedRaftLog extends RaftLog {
if (segment.getTotalSize() >= segmentMaxSize) {
return true;
} else {
- final long entrySize = LogSegment.getEntrySize(entry);
+ final long entrySize = LogSegment.getEntrySize(entry, LogSegment.Op.WRITE_SEGMENT_FILE);
// if entry size is greater than the max segment size, write it directly
// into the current segment
return entrySize <= segmentMaxSize &&
segment.getTotalSize() + entrySize > segmentMaxSize;
```
----------------------------------------------------------------
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-ratis] runzhiwang closed pull request #317: RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317
----------------------------------------------------------------
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-ratis] runzhiwang commented on a change in pull request #317: [WIP]RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#discussion_r535198688
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
##########
@@ -388,10 +388,12 @@ public TermIndex getLastEntryTermIndex() {
try(AutoCloseableLock writeLock = writeLock()) {
validateLogEntry(entry);
final LogSegment currentOpenSegment = cache.getOpenSegment();
+ // The stateMachineData will be cached inside the StateMachine itself when enable stateMachineCaching.
+ LogEntryProto toAppendEntry = stateMachineCachingEnabled? ServerProtoUtils.removeStateMachineData(entry) : entry;
if (currentOpenSegment == null) {
cache.addOpenSegment(entry.getIndex());
fileLogWorker.startLogSegment(entry.getIndex());
- } else if (isSegmentFull(currentOpenSegment, entry)) {
+ } else if (isSegmentFull(currentOpenSegment, toAppendEntry)) {
Review comment:
@szetszwo The problem is `cache.appendEntry(entry)` will put entry with `StateMachineData` into `LogSegment#entryCache`. For example, each entry occupy 8KB after remove `StateMachineData`, each `StateMachineData` is 80MB, the segment.size.max is 8MB, when `isSegmentFull` returns true, open segment has 1000 entry, and `LogSegment#entryCache` is 80MB * 1000, i.e. 80GB. Because `checkAndEvictCache` depends on `isSegmentFull` returns true, so OOM also can happen.
```
if (stateMachineCachingEnabled) {
// The stateMachineData will be cached inside the StateMachine itself.
- cache.appendEntry(ServerProtoUtils.removeStateMachineData(entry));
+ cache.appendEntry(removedStateMachineData));
} else {
cache.appendEntry(entry);
}
```
```
if (isSegmentFull(currentOpenSegment, toAppendEntry)) {
cache.rollOpenSegment(true);
fileLogWorker.rollLogSegment(currentOpenSegment);
checkAndEvictCache();
}
```
----------------------------------------------------------------
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-ratis] runzhiwang commented on a change in pull request #317: [WIP]RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#discussion_r535198688
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
##########
@@ -388,10 +388,12 @@ public TermIndex getLastEntryTermIndex() {
try(AutoCloseableLock writeLock = writeLock()) {
validateLogEntry(entry);
final LogSegment currentOpenSegment = cache.getOpenSegment();
+ // The stateMachineData will be cached inside the StateMachine itself when enable stateMachineCaching.
+ LogEntryProto toAppendEntry = stateMachineCachingEnabled? ServerProtoUtils.removeStateMachineData(entry) : entry;
if (currentOpenSegment == null) {
cache.addOpenSegment(entry.getIndex());
fileLogWorker.startLogSegment(entry.getIndex());
- } else if (isSegmentFull(currentOpenSegment, entry)) {
+ } else if (isSegmentFull(currentOpenSegment, toAppendEntry)) {
Review comment:
@szetszwo The problem is cache.appendEntry(entry) will put entry with StateMachineData into LogSegment#entryCache. For example, each entry occupy 8KB after remove StateMachineData, each StateMachineData is 80M, the max segment.size.max is 8MB, when `isSegmentFull` returns true, open segment has 1000 entry, and LogSegment#entryCache is 80M * 1000, i.e. 80GB.
```
if (stateMachineCachingEnabled) {
// The stateMachineData will be cached inside the StateMachine itself.
- cache.appendEntry(ServerProtoUtils.removeStateMachineData(entry));
+ cache.appendEntry(removedStateMachineData));
} else {
cache.appendEntry(entry);
}
```
----------------------------------------------------------------
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-ratis] runzhiwang commented on a change in pull request #317: [WIP]RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#discussion_r535198688
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
##########
@@ -388,10 +388,12 @@ public TermIndex getLastEntryTermIndex() {
try(AutoCloseableLock writeLock = writeLock()) {
validateLogEntry(entry);
final LogSegment currentOpenSegment = cache.getOpenSegment();
+ // The stateMachineData will be cached inside the StateMachine itself when enable stateMachineCaching.
+ LogEntryProto toAppendEntry = stateMachineCachingEnabled? ServerProtoUtils.removeStateMachineData(entry) : entry;
if (currentOpenSegment == null) {
cache.addOpenSegment(entry.getIndex());
fileLogWorker.startLogSegment(entry.getIndex());
- } else if (isSegmentFull(currentOpenSegment, entry)) {
+ } else if (isSegmentFull(currentOpenSegment, toAppendEntry)) {
Review comment:
@szetszwo The problem is `cache.appendEntry(entry)` will put entry with `StateMachineData` into `LogSegment#entryCache`. For example, each entry occupy 8KB after remove `StateMachineData`, each `StateMachineData` is 80MB, the segment.size.max is 8MB, when `isSegmentFull` returns true, open segment has 1000 entry, and `LogSegment#entryCache` is 80MB * 1000, i.e. 80GB.
```
if (stateMachineCachingEnabled) {
// The stateMachineData will be cached inside the StateMachine itself.
- cache.appendEntry(ServerProtoUtils.removeStateMachineData(entry));
+ cache.appendEntry(removedStateMachineData));
} else {
cache.appendEntry(entry);
}
```
----------------------------------------------------------------
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-ratis] runzhiwang commented on a change in pull request #317: RATIS-1198. Fix OOM when use FileStore to write 1GB file
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #317:
URL: https://github.com/apache/incubator-ratis/pull/317#discussion_r535865106
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java
##########
@@ -391,7 +391,10 @@ public TermIndex getLastEntryTermIndex() {
if (currentOpenSegment == null) {
cache.addOpenSegment(entry.getIndex());
fileLogWorker.startLogSegment(entry.getIndex());
- } else if (isSegmentFull(currentOpenSegment, entry)) {
+ } else if (isSegmentFull(currentOpenSegment, entry,
+ stateMachineCachingEnabled ?
+ LogSegment.Op.CHECK_SEGMENT_FILE_FULL_WITH_STATE_MACHINE_CACHE:
+ LogSegment.Op.CHECK_SEGMENT_FILE_FULL_WITHOUT_STATE_MACHINE_CACHE)) {
Review comment:
@szetszwo I have updated the patch. Could you help review it again ?
----------------------------------------------------------------
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