feat: Implement ReadTransaction class and integrate with Client and Replica - #83
Conversation
| scheduleTransaction(transaction); | ||
| } | ||
|
|
||
| public void onReadMsg(ReadMsg msg) { |
There was a problem hiding this comment.
onReadMsg() directly accesses positions[msg.index], bypassing the bounds validation already provided by getPosition(). An invalid index can therefore throw an exception inside the actor.
Possible mitigations
Use getPosition(msg.index) or validate the index explicitly. The protocol should also define how an invalid read is reported, instead of allowing the actor to fail.
There was a problem hiding this comment.
I'll fix it, but now may that an unaware Client may throws an exception on a Replica without noticing it.
Should we implement a fallback Transaction.ErrorMsg to tell the client that the index it asked for is unavailable? (same could be for the WriteTransaction)
If yes, please add a follow-up Issue connected to this comment please!
| public static class ReadResultMsg extends Msg { | ||
| public final int replicaId; | ||
| public final int value; | ||
|
|
||
| public ReadResultMsg(TransactionId transactionId, EpochPair epochPair, ActorRef sender, int value, int replicaId) { | ||
| super(transactionId, epochPair, sender); | ||
| this.replicaId = replicaId; | ||
| this.value = value; |
There was a problem hiding this comment.
ReadResultMsg.sender already identifies the responding replica as an ActorRef, while replicaId carries a second identity for the same replica. These two values could theoretically disagree.
Possible mitigations
Remove replicaId from the internal message and use sender. If it remains, replica IDs should be validated as unique during system initialization, and maybe they can just be derived on the Client side from their ActorRef, with some method like getReplicaIdFromActorRef() or something similar.
There was a problem hiding this comment.
and maybe they can just be derived on the Client side from their ActorRef, with some method like
getReplicaIdFromActorRef()or something similar.
No, is not possible. The replicaId is an integer that could not be retriven from the ActorRef point of view, and yes, it could disagree with the ActorRef, but there is no other way to get the Replica.id value needed for populate the AbstractClient.ReadResult.fromReplica
And this is the cause that drive me in not sending a "negative result" (ReadResult.success = false), basically you don't have the replica number yet if no message response is received and there is no other way to get it.
Tell me if I'm wrong...
|
Also added #93 as a new issue. |
alanmasu
left a comment
alanmasu
left a comment
There was a problem hiding this comment.
Fixed the issue you rise.
| public static class ReadResultMsg extends Msg { | ||
| public final int replicaId; | ||
| public final int value; | ||
|
|
||
| public ReadResultMsg(TransactionId transactionId, EpochPair epochPair, ActorRef sender, int value, int replicaId) { | ||
| super(transactionId, epochPair, sender); | ||
| this.replicaId = replicaId; | ||
| this.value = value; |
There was a problem hiding this comment.
and maybe they can just be derived on the Client side from their ActorRef, with some method like
getReplicaIdFromActorRef()or something similar.
No, is not possible. The replicaId is an integer that could not be retriven from the ActorRef point of view, and yes, it could disagree with the ActorRef, but there is no other way to get the Replica.id value needed for populate the AbstractClient.ReadResult.fromReplica
And this is the cause that drive me in not sending a "negative result" (ReadResult.success = false), basically you don't have the replica number yet if no message response is received and there is no other way to get it.
Tell me if I'm wrong...
| scheduleTransaction(transaction); | ||
| } | ||
|
|
||
| public void onReadMsg(ReadMsg msg) { |
| scheduleTransaction(transaction); | ||
| } | ||
|
|
||
| public void onReadMsg(ReadMsg msg) { |
There was a problem hiding this comment.
I'll fix it, but now may that an unaware Client may throws an exception on a Replica without noticing it.
Should we implement a fallback Transaction.ErrorMsg to tell the client that the index it asked for is unavailable? (same could be for the WriteTransaction)
If yes, please add a follow-up Issue connected to this comment please!
…ener integration
github-project-automation
Bot
moved this from In review
to Done
in Distributed Systems Project
Integrate PR #83 read-transaction changes and resolve the Replica overlap by retaining both ReadMsg and UpdateMsg receive registrations and handlers. Focused read and update tests pass together after the resolution.
2 participants
This PR:
Fixes Implement the FSM [ReadTransaction] #51
Related to Document [ReadTransaction] #53
Is related to Test cases [ReadTransaction] #52