[jira] [Commented] (RATIS-17) Add basic retry cache implementation for Raft Server
[ https://issues.apache.org/jira/browse/RATIS-17?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15950439#comment-15950439 ] Jing Zhao commented on RATIS-17: Thanks for the review, Nicholas! Since RATIS-52 got reopened, I will commit the 006 patch shortly :) > Add basic retry cache implementation for Raft Server > > > Key: RATIS-17 > URL: https://issues.apache.org/jira/browse/RATIS-17 > Project: Ratis > Issue Type: Sub-task >Reporter: Jing Zhao >Assignee: Jing Zhao > Attachments: RATIS-17.000.patch, RATIS-17.001.patch, > RATIS-17.002.patch, RATIS-17.003.patch, RATIS-17.004.patch, > RATIS-17.005.patch, RATIS-17.006.patch > > > This jira will add the basic data structure definition for the retry cache. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (RATIS-17) Add basic retry cache implementation for Raft Server
[ https://issues.apache.org/jira/browse/RATIS-17?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15950372#comment-15950372 ] Tsz Wo Nicholas Sze commented on RATIS-17: -- +1 the 006 patch looks good. Please update it with trunk and commit it. Thanks. > Add basic retry cache implementation for Raft Server > > > Key: RATIS-17 > URL: https://issues.apache.org/jira/browse/RATIS-17 > Project: Ratis > Issue Type: Sub-task >Reporter: Jing Zhao >Assignee: Jing Zhao > Attachments: RATIS-17.000.patch, RATIS-17.001.patch, > RATIS-17.002.patch, RATIS-17.003.patch, RATIS-17.004.patch, > RATIS-17.005.patch, RATIS-17.006.patch > > > This jira will add the basic data structure definition for the retry cache. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (RATIS-17) Add basic retry cache implementation for Raft Server
[
https://issues.apache.org/jira/browse/RATIS-17?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15949686#comment-15949686
]
Jing Zhao commented on RATIS-17:
Thanks for the comments, Nicholas.
bq. For #2, should we just return the exception back to client, instead of a
real retry?
Yes, currently for #2 the patch always returns the exception back to client,
i.e., if the request gets committed in raft group, we always treat it as
success even if finally the state machine throws an exception while applying
it. Please see javadoc in {{CacheEntry}}.
bq. For #1, should we never cache the failure in Raft in the retry cache since
it won't be useful anyway?
If the failure is caught before the retry cache entry gets added, we will not
create the entry. This is the case of the first {{checkLeaderState}} call in
{{submitClientRequestAsync}}. However, if the failure is hit after adding the
entry, we only mark the entry as failed and depend on the retry cache's
eviction/expiration policy or the next retry attempt to evict the failed entry.
Because the failure can be hit in different places (replication failure, log
append failure, leader reelection etc.), in this way we can avoid manipulating
retry cache from a lot of other places.
bq. In RaftServerImpl.replyPendingRequest, cacheEntry.updateResult(r) expects
!replyFuture.isDone(). So, cacheEntry = retryCache.getOrCreateEntry(..)
actually must be creating a new entry.
For leader, the entry should already be there but its process is still pending
(until the txn gets applied to the state machine), thus the entry is not newly
created but the future is still pending. For followers we will create new
entries here.
I will update a new patch to address other comments.
> Add basic retry cache implementation for Raft Server
>
>
> Key: RATIS-17
> URL: https://issues.apache.org/jira/browse/RATIS-17
> Project: Ratis
> Issue Type: Sub-task
>Reporter: Jing Zhao
>Assignee: Jing Zhao
> Attachments: RATIS-17.000.patch, RATIS-17.001.patch,
> RATIS-17.002.patch, RATIS-17.003.patch, RATIS-17.004.patch, RATIS-17.005.patch
>
>
> This jira will add the basic data structure definition for the retry cache.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (RATIS-17) Add basic retry cache implementation for Raft Server
[
https://issues.apache.org/jira/browse/RATIS-17?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15944136#comment-15944136
]
Jing Zhao commented on RATIS-17:
Just realized that the {{queryCache}} still has a race condition. Will
re-upload a new patch soon.
> Add basic retry cache implementation for Raft Server
>
>
> Key: RATIS-17
> URL: https://issues.apache.org/jira/browse/RATIS-17
> Project: Ratis
> Issue Type: Sub-task
>Reporter: Jing Zhao
>Assignee: Jing Zhao
> Attachments: RATIS-17.000.patch, RATIS-17.001.patch,
> RATIS-17.002.patch, RATIS-17.003.patch
>
>
> This jira will add the basic data structure definition for the retry cache.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (RATIS-17) Add basic retry cache implementation for Raft Server
[ https://issues.apache.org/jira/browse/RATIS-17?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15941395#comment-15941395 ] Jing Zhao commented on RATIS-17: bq. The guava Cache is thread safe. Do we need to synchronize queryCache and getOrCreateEntry? The reason we currently synchronize the queryCache and getOrCreateEntry is that multiple retries of the same request may be handled concurrently (e.g., initially all the retries are sitting in the RPC queue and then get processed by different handlers concurrently). Synchronizing on the retry cache may affect the performance. But maybe we can improve it as a follow-on work. > Add basic retry cache implementation for Raft Server > > > Key: RATIS-17 > URL: https://issues.apache.org/jira/browse/RATIS-17 > Project: Ratis > Issue Type: Sub-task >Reporter: Jing Zhao >Assignee: Jing Zhao > Attachments: RATIS-17.000.patch, RATIS-17.001.patch, > RATIS-17.002.patch, RATIS-17.003.patch > > > This jira will add the basic data structure definition for the retry cache. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (RATIS-17) Add basic retry cache implementation for Raft Server
[ https://issues.apache.org/jira/browse/RATIS-17?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15937491#comment-15937491 ] Tsz Wo Nicholas Sze commented on RATIS-17: -- Thanks Jing, some quick comments: - CacheKey is created multiple times in RetryCache. We should only create once at queryCache or getOrCreateEntry, and then keep passing it. - The guava Cache is thread safe. Do we need to synchronize queryCache and getOrCreateEntry? - CacheEntry already has a replyFuture so that we should keep using it. e.g. checkLeaderState(..) should not create CompletableFuture anymore. - MAX_CAPACITY actually is MIN_CAPACITY. Otherwise, max = 16 is way to small. :) > Add basic retry cache implementation for Raft Server > > > Key: RATIS-17 > URL: https://issues.apache.org/jira/browse/RATIS-17 > Project: Ratis > Issue Type: Sub-task >Reporter: Jing Zhao >Assignee: Jing Zhao > Attachments: RATIS-17.000.patch, RATIS-17.001.patch, > RATIS-17.002.patch > > > This jira will add the basic data structure definition for the retry cache. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
