szetszwo commented on code in PR #1045:
URL: https://github.com/apache/ratis/pull/1045#discussion_r1518074079
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/StateMachineUpdater.java:
##########
@@ -233,22 +233,27 @@ private
MemoizedSupplier<List<CompletableFuture<Message>>> applyLog() throws Raf
final long committed = raftLog.getLastCommittedIndex();
for(long applied; (applied = getLastAppliedIndex()) < committed && state
== State.RUNNING && !shouldStop(); ) {
final long nextIndex = applied + 1;
- final LogEntryProto next = raftLog.get(nextIndex);
+ final ReferenceCountedObject<LogEntryProto> next =
raftLog.retainLog(nextIndex);
if (next != null) {
Review Comment:
Let's change it to `if (next == null)`. We can have one less indentation.
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/memory/MemoryRaftLog.java:
##########
@@ -45,16 +45,17 @@ public class MemoryRaftLog extends RaftLogBase {
static class EntryList {
private final List<ReferenceCountedObject<LogEntryProto>> entries = new
ArrayList<>();
- LogEntryProto get(int i) {
- return i >= 0 && i < entries.size() ? entries.get(i).get() : null;
+ ReferenceCountedObject<LogEntryProto> get(int i) {
+ return i >= 0 && i < entries.size() ? entries.get(i) : null;
}
TermIndex getTermIndex(int i) {
- return TermIndex.valueOf(get(i));
+ ReferenceCountedObject<LogEntryProto> ref = get(i);
+ return TermIndex.valueOf(ref != null ? ref.get() : null);
}
private LogEntryHeader getLogEntryHeader(int i) {
- return LogEntryHeader.valueOf(get(i));
+ return LogEntryHeader.valueOf(get(i).get());
Review Comment:
Similar to `getTermIndex`, this needs to check null.
##########
ratis-server-api/src/main/java/org/apache/ratis/server/raftlog/RaftLog.java:
##########
@@ -58,8 +59,31 @@ default boolean contains(TermIndex ti) {
/**
* @return null if the log entry is not found in this log;
* otherwise, return the log entry corresponding to the given index.
+ * @deprecated use {@link RaftLog#retainLog(long)} instead.
*/
- LogEntryProto get(long index) throws RaftLogIOException;
+ @Deprecated
+ default LogEntryProto get(long index) throws RaftLogIOException {
+ ReferenceCountedObject<LogEntryProto> ref = retainLog(index);
+ try {
+ return ref != null ? ref.get() : null;
Review Comment:
We should return a copy instead; otherwise, it is already released.
Since `LogProtoUtils.copy(..)` is not in raft-server-api, let's keep the
method abstract and move the code to the implementations.
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java:
##########
@@ -284,43 +290,54 @@ record = segment.getLogRecord(index);
if (record == null) {
return null;
}
- final LogEntryProto entry =
segment.getEntryFromCache(record.getTermIndex());
+ final ReferenceCountedObject<LogEntryProto> entry =
segment.getEntryFromCache(record.getTermIndex());
if (entry != null) {
getRaftLogMetrics().onRaftLogCacheHit();
+ entry.retain();
return entry;
}
}
// the entry is not in the segment's cache. Load the cache without holding
the lock.
getRaftLogMetrics().onRaftLogCacheMiss();
cacheEviction.signal();
- return segment.loadCache(record);
+ ReferenceCountedObject<LogEntryProto> loaded = segment.loadCache(record);
+ if (loaded != null) {
+ loaded.retain();
+ }
+ return loaded;
}
@Override
public EntryWithData getEntryWithData(long index) throws RaftLogIOException {
Review Comment:
Let's add a new `getEntryWithData(LogEntryProto entry)` method. It will be
more readable.
```java
public EntryWithData getEntryWithData(long index) throws
RaftLogIOException {
final ReferenceCountedObject<LogEntryProto> entryRef = retainLog(index);
if (entryRef == null) {
throw new RaftLogIOException("Log entry not found: index = " + index);
}
try {
// TODO. The reference counted object should be passed to LogAppender
RATIS-2026.
return getEntryWithData(entryRef.get());
} finally {
entryRef.release();
}
}
private EntryWithData getEntryWithData(LogEntryProto entry) throws
RaftLogIOException {
// same code as before
}
```
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/memory/MemoryRaftLog.java:
##########
@@ -45,16 +45,17 @@ public class MemoryRaftLog extends RaftLogBase {
static class EntryList {
private final List<ReferenceCountedObject<LogEntryProto>> entries = new
ArrayList<>();
- LogEntryProto get(int i) {
- return i >= 0 && i < entries.size() ? entries.get(i).get() : null;
+ ReferenceCountedObject<LogEntryProto> get(int i) {
+ return i >= 0 && i < entries.size() ? entries.get(i) : null;
Review Comment:
Add a `getRef` method and keep the `get` method returning `LogEntryProto`.
```java
ReferenceCountedObject<LogEntryProto> getRef(int i) {
return i >= 0 && i < entries.size() ? entries.get(i) : null;
}
LogEntryProto get(int i) {
final ReferenceCountedObject<LogEntryProto> ref = getRef(i);
return ref != null ? ref.get() : null;
}
```
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java:
##########
@@ -284,43 +290,54 @@ record = segment.getLogRecord(index);
if (record == null) {
return null;
}
- final LogEntryProto entry =
segment.getEntryFromCache(record.getTermIndex());
+ final ReferenceCountedObject<LogEntryProto> entry =
segment.getEntryFromCache(record.getTermIndex());
if (entry != null) {
getRaftLogMetrics().onRaftLogCacheHit();
+ entry.retain();
return entry;
}
}
// the entry is not in the segment's cache. Load the cache without holding
the lock.
getRaftLogMetrics().onRaftLogCacheMiss();
cacheEviction.signal();
- return segment.loadCache(record);
+ ReferenceCountedObject<LogEntryProto> loaded = segment.loadCache(record);
+ if (loaded != null) {
+ loaded.retain();
Review Comment:
We should call `retain()` inside `LogEntryLoader.load(..)`.
```java
@@ -243,6 +243,7 @@ public final class LogSegment {
final TermIndex ti = TermIndex.valueOf(entry);
putEntryCache(ti, entryRef, Op.LOAD_SEGMENT_FILE);
if (ti.equals(key.getTermIndex())) {
+ entryRef.retain();
toReturn.set(entryRef);
}
entryRef.release();
```
##########
ratis-server-api/src/main/java/org/apache/ratis/server/raftlog/RaftLog.java:
##########
@@ -58,8 +59,31 @@ default boolean contains(TermIndex ti) {
/**
* @return null if the log entry is not found in this log;
* otherwise, return the log entry corresponding to the given index.
Review Comment:
Update the javadoc to mention copying.
```java
/**
* @return null if the log entry is not found in this log;
* otherwise, return a copy of the log entry corresponding to the
given index.
* @deprecated use {@link RaftLog#retainLog(long)} instead in order to
avoid copying.
*/
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
