IGNITE-19120 Raft client should get leader metadata along while getting leader itself #2255
Conversation
| @@ -41,4 +41,14 @@ public LeaderWithTerm(@Nullable Peer leader, long term) { | |||
| public long term() { | |||
| return term; | |||
| } | |||
|
|
|||
| @Override | |||
| public String toString() { | |||
There was a problem hiding this comment.
Could we use S.toString(LeaderWithTerm.class, this); ?
| * | ||
| * @return Leader peer with corresponding term. | ||
| */ | ||
| IgniteBiTuple<PeerId, Long> getLeaderWithTer(); |
| } | ||
| finally { | ||
| this.readLock.unlock(); | ||
| this.metrics.recordLatency("handle-read-index", Utils.monotonicMs() - startMs); |
There was a problem hiding this comment.
What's your proposal?
| @@ -347,6 +356,83 @@ public void testNotificationToPlacementDriverAboutMajorityLoss() throws Exceptio | |||
| stopReplicationGroup(GROUP_ID, grpNodes); | |||
| } | |||
|
|
|||
| @Test | |||
| @Disabled("IGNITE-19120 Raft client should get leader metadata along while getting leader itself") | |||
There was a problem hiding this comment.
why is this test disabled with the current ticket?
| 1_000 | ||
| ); | ||
|
|
||
| Thread.sleep(2_000); |
There was a problem hiding this comment.
I don't think that this is a good solution
There was a problem hiding this comment.
It is a disable test. When the corresponding ticket be implemented, it will be able to rewrite.
|
|
||
| list.removeAll(exceptNodes); | ||
|
|
||
| Collections.shuffle(list); |
There was a problem hiding this comment.
why do we need to shuffle the whole list, if we can generate random index to get?
There was a problem hiding this comment.
Nothing change, do you insist that?
There was a problem hiding this comment.
Moreover, we are already using this way for the same reason in other place.
| private CompletableFuture<AtomicReference<ClusterNode>> leaderFuture = new CompletableFuture<>(); | ||
|
|
||
| private AtomicReference<ClusterNode> leaderRef = new AtomicReference<>(); | ||
| // TODO:IGNITE-19120 Raft client should get leader metadata along while getting leader itself |
There was a problem hiding this comment.
todo with the current ticket number
| if (leaseExpirationTime != null) { | ||
| assert msg.leaseExpirationTime().after(leaseExpirationTime) : "Invalid lease expiration time in message, msg=" + msg; | ||
| } | ||
| return readyMajority.thenCompose(unused -> raftClient.readLeaderMetadata()).thenCompose(leaderMetadata -> { |
There was a problem hiding this comment.
what if a majority was lost on the moment of the call?
| .thenCompose(v -> { | ||
| if (msg.leaseExpirationTime().getPhysical() < currentTimeMillis()) { |
There was a problem hiding this comment.
Is this the correct way to compare hybrid timestamp? I was expecting HybridTimestamp#compareTo with clock.now
| * @param ex An exception | ||
| * @return True if this exception has thrown due to timeout or connection problem, false otherwise. | ||
| */ | ||
| private static boolean isConnectivityRelatedException(Throwable ex) { |
There was a problem hiding this comment.
this method is not used anywhere
https://issues.apache.org/jira/browse/IGNITE-19120