szetszwo commented on code in PR #1042:
URL: https://github.com/apache/ratis/pull/1042#discussion_r1483563687
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/TransactionContext.java:
##########
@@ -96,9 +97,28 @@ public interface TransactionContext {
/**
* Returns the committed log entry
* @return the committed log entry
+ *
+ * @deprecated use {@link TransactionContext#getLogEntryRef()} instead.
*/
+ @Deprecated
LogEntryProto getLogEntry();
+ /**
+ * Returns the {@link ReferenceCountedObject} that references to committed
log entry.
+ *
+ * Clients of this method may call {@link ReferenceCountedObject#get()} to
access the log entry immediately inside
+ * the scope of {@link StateMachine#applyTransaction}.
+ *
+ * If the log entry is needed after {@link StateMachine#applyTransaction}
returns, e.g. for asynchronous computation
+ * or caching, the client code must invoke {@link
ReferenceCountedObject#retain()} and
+ * {@link ReferenceCountedObject#release()}.
+ *
+ * @return Reference to the committed log entry.
+ */
+ default ReferenceCountedObject<LogEntryProto> getLogEntryRef() {
+ return Optional.ofNullable(getLogEntry()).map(this::wrap).orElse(null);
Review Comment:
Use `getLogEntryUnsafe()` instead.
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/TransactionContext.java:
##########
@@ -96,9 +97,28 @@ public interface TransactionContext {
/**
* Returns the committed log entry
* @return the committed log entry
+ *
+ * @deprecated use {@link TransactionContext#getLogEntryRef()} instead.
*/
+ @Deprecated
LogEntryProto getLogEntry();
+ /**
+ * Returns the {@link ReferenceCountedObject} that references to committed
log entry.
+ *
+ * Clients of this method may call {@link ReferenceCountedObject#get()} to
access the log entry immediately inside
+ * the scope of {@link StateMachine#applyTransaction}.
+ *
+ * If the log entry is needed after {@link StateMachine#applyTransaction}
returns, e.g. for asynchronous computation
+ * or caching, the client code must invoke {@link
ReferenceCountedObject#retain()} and
+ * {@link ReferenceCountedObject#release()}.
+ *
+ * @return Reference to the committed log entry.
+ */
Review Comment:
The javadoc is good. Some minor suggestion below:
```java
/**
* Get a {@link ReferenceCountedObject} to the committed log entry.
*
* It is safe to access the log entry by calling {@link
ReferenceCountedObject#get()}
* (without {@link ReferenceCountedObject#retain()})
* inside the scope of {@link StateMachine#applyTransaction}.
*
* If the log entry is needed after {@link StateMachine#applyTransaction}
returns,
* e.g. for asynchronous computation or caching,
* the caller must invoke {@link ReferenceCountedObject#retain()} and
{@link ReferenceCountedObject#release()}.
*
* @return a reference to the committed log entry if it exists; otherwise,
returns null.
*/
```
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/TransactionContextImpl.java:
##########
@@ -209,4 +214,8 @@ public TransactionContext cancelTransaction() throws
IOException {
// call this to let the SM know that Transaction cannot be synced
return stateMachine.cancelTransaction(this);
}
+
+ public static LogEntryProto getLogEntry(TransactionContext context) {
+ return ((TransactionContextImpl) context).logEntry;
+ }
Review Comment:
Let's move this to `TransactionContext`
```java
//TransactionContext.java
/**
* @return the committed log entry if it exists; otherwise, returns null.
* The returned value is safe to use only before {@link
StateMachine#applyTransaction} returns.
* Once {@link StateMachine#applyTransaction} has returned, it is
unsafe to use the log entry
* since the underlying buffers can possiby be released.
*/
default LogEntryProto getLogEntryUnsafe() {
return getLogEntryRef().get();
}
```
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/TransactionContext.java:
##########
@@ -96,9 +97,28 @@ public interface TransactionContext {
/**
* Returns the committed log entry
* @return the committed log entry
+ *
+ * @deprecated use {@link TransactionContext#getLogEntryRef()} instead.
Review Comment:
Let's update the javadoc to mention "copy".
```java
* @return a copy of the committed log entry if it exists; otherwise,
returns null.
*
* @deprecated Use {@link #getLogEntryRef()} or {@link
#getLogEntryUnsafe()} to avoid copying.
```
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/TransactionContextImpl.java:
##########
@@ -177,7 +177,12 @@ public CompletableFuture<Long> getLogIndexFuture() {
@Override
public LogEntryProto getLogEntry() {
- return logEntry;
+ return LogProtoUtils.copy(logEntry);
Review Comment:
We should store the copy to avoiding copying it for each call.
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/TransactionContextImpl.java:
##########
@@ -177,7 +177,12 @@ public CompletableFuture<Long> getLogIndexFuture() {
@Override
public LogEntryProto getLogEntry() {
- return logEntry;
+ return LogProtoUtils.copy(logEntry);
+ }
+
+ @Override
+ public ReferenceCountedObject<LogEntryProto> getLogEntryRef() {
+ return logEntry != null ? wrap(logEntry) : null;
}
Review Comment:
This is not needed after we add `getLogEntryUnsafe()`.
--
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]
