Skip to content

RATIS-872. Invalidate replied calls in retry cache.#942

Merged
SzyWilliam merged 1 commit intoapache:masterfrom
szetszwo:RATIS-872
Oct 31, 2023
Merged

RATIS-872. Invalidate replied calls in retry cache.#942
SzyWilliam merged 1 commit intoapache:masterfrom
szetszwo:RATIS-872

Conversation

@szetszwo
Copy link
Contributor

See RATIS-872.

@szetszwo
Copy link
Contributor Author

The test failure is related: https://github.com/apache/ratis/actions/runs/6566630497/job/17837715966?pr=942#step:5:811

Since the calls are submitted asynchronously, we cannot assume the callIds are in order. Will remove the callId set from the server side and let the client send multiple replied callIds.

Copy link
Member

@SzyWilliam SzyWilliam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @szetszwo for the retry cache optimization! The change looks good to me.
Just a small question, since we do not carry repliedId for read-only requests in unorderedAsync, shall we also exclude them in orderedAsync?

@szetszwo
Copy link
Contributor Author

..., since we do not carry repliedId for read-only requests in unorderedAsync, shall we also exclude them in orderedAsync?

This is a very good question! I handle blocking call and async calls separately. Actually, it is better to handle all the calls at the same time in RaftClientImpl, which will collect the replied call ids and use them to create requests. Let me update this after RATIS-1916.

@szetszwo
Copy link
Contributor Author

@SzyWilliam , moved all the replied callId handling to RaftClientImpl and any calls can include replied callIds.

Copy link
Member

@SzyWilliam SzyWilliam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed out the update. +1 the change looks good. The code looks really neat!

@SzyWilliam SzyWilliam merged commit 22cbefa into apache:master Oct 31, 2023
@szetszwo
Copy link
Contributor Author

@SzyWilliam , thanks a lot for reviewing and merging this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants