Fix GetReadVersionReply.recentRequests to reflect a proxy's number of recent requests in the commit stats. #3377
Conversation
|
|
||
| for (auto v : versions) { | ||
| if(v.version > rep.version) { | ||
| rep = v; | ||
| } | ||
| } | ||
| rep.recentRequests = commitData->stats.getRecentRequests(); |
There was a problem hiding this comment.
It seems like the change is moving this line to here after the for loop. What’s the problem it fixes?
There was a problem hiding this comment.
It seems that I cannot configure the reviewer to review the PR. Can you help add Evan into this since I have synced with him about this offline.
There was a problem hiding this comment.
Have updated the PR description, thanks.
| @@ -1411,13 +1411,13 @@ ACTOR Future<GetReadVersionReply> getLiveCommittedVersion(ProxyCommitData* commi | |||
| rep.version = commitData->committedVersion.get(); | |||
There was a problem hiding this comment.
won't these assignments be override in the following for() loop for the same reason?
Why don't we move them below the for loop?
There was a problem hiding this comment.
I think these are fine because they are supposed to overridden when other proxy has a larger version and it's 'locked' and 'metadataVersion' is more up-to-date.
|
20200616-233150-apple::PR_3377-52c19f9284c793c3 correctness run passed with previous known errors. |
This is a bug I found when working on #3307 and decide to make the change a patch against 6.3.
[1] https://github.com/apple/foundationdb/blob/master/fdbserver/MasterProxyServer.actor.cpp#L2086