sanpwc commented on code in PR #4700:
URL: https://github.com/apache/ignite-3/pull/4700#discussion_r1850896108
##########
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();
return REPLICA_MESSAGES_FACTORY
.timestampAwareReplicaResponse()
- .result(result)
- .timestamp(clockService.now())
+ .result(result.result())
+ .timestamp(commitTs == null ? clockService.current() :
commitTs)
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
