sanpwc commented on code in PR #4700:
URL: https://github.com/apache/ignite-3/pull/4700#discussion_r1850895741


##########
modules/replicator/src/main/java/org/apache/ignite/internal/replicator/ReplicaManager.java:
##########
@@ -1040,17 +1041,18 @@ private void sendAwaitReplicaResponse(String 
senderConsistentId, long correlatio
     /**
      * Prepares replica response.
      */
-    private NetworkMessage prepareReplicaResponse(boolean sendTimestamp, 
Object result) {
+    private NetworkMessage prepareReplicaResponse(boolean sendTimestamp, 
ReplicaResult result) {
         if (sendTimestamp) {
+            HybridTimestamp commitTs = 
result.applyResult().getCommitTimestamp();

Review Comment:
   We may easily introduce a bug with such logic, that will be extremely 
difficult to fix.
   In assumption that commitTs is !null in case of full transaction and taking 
into consideration the fact that 
   in case of full tx commitTs is propagated through safeTime - `new 
CommandApplicationResult(((UpdateCommand) res.getCommand()).safeTime()` `and 
cmd.full() ? cmd.safeTime() : null` it's required to generate safeTime for full 
tx as clock.now(). As as far as I remember, you had an idea to eliminate ticks 
within safeTime generation. It worth at least to write corresponding comment in 
code near safeTime generation with all aforementioned.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to