sanpwc commented on code in PR #7313:
URL: https://github.com/apache/ignite-3/pull/7313#discussion_r2647179197
##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/replication/PartitionReplicaListenerIndexLockingTest.java:
##########
@@ -422,6 +419,10 @@ void testReadWriteSingle(ReadWriteTestArg arg) {
);
}
+ private static @NotNull ReplicaPrimacy replicaPrimacy() {
+ return
ReplicaPrimacy.forPrimaryReplicaRequest(HybridTimestamp.MIN_VALUE.longValue());
Review Comment:
> Also, I don't see where that HybridTimestamp(0, 1) is used in the test
implementation.
```
public static final HybridTimestamp MIN_VALUE = new HybridTimestamp(0L,
1);
...
TestReplicaMetaImpl(@Nullable InternalClusterNode leaseholder) {
this(leaseholder, MIN_VALUE, MAX_VALUE);
}
```
> All in all, using just constant 1 seems to be an easier and, probably, not
more fragile way to get a 'primacy' instance.
What I mean here is that within test we should use consistent values. By a
lucky coincidence they are consistent if TestPlacementDriver is used, however:
- I'm not sure that it's used in all tests, where you've added
replicaPrimacy(). It's possible that some tests use fully fledged one. Didn't
check though.
- Someone may change leaseStartTime in TestPlacementDriver or in
ReplicaPrimacy.forPrimaryReplicaRequest and break the consistency.
> It looks fragile as at some point we could change the test placement
driver implementation to sometimes return null.
In that case we will obviously see a test failure and fix the problem.
--
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]
