You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There is an example of the change being used in PR #6144.
Since isActionComplete currently only has one caller that depends on the replica action check we can move the replica action check to the caller method instead of having the check be inside isActionComplete.
The case of AsyncRequestFutureImpl without result tracking/null results:
The function does not support a AsyncRequestFutureImpl with null results (neither before nor after my proposed changes). I don't currently see a way to reliably check for action completion from within AsyncRequestFutureImpl for the null results case (I don't know how common this case is in practice, but it is accommodated for across AsyncRequestFutureImpl, and I test that path with Table.batch(List<Row>, null)). My understanding of that code is that in the null results case one cannot readily tell whether a given action has completed from within AsyncRequestFutureImpl because:
In the case of action success we just decrement action counter when calling setResult.
In the case of action failure the failure is tracked in BatchErrors, but BatchErrors does not track the action index for the failed action, only the Row, so even if we wanted to check whether for a given action an error was added to BatchErrors, we couldn't because of the way that Row equality is currently implemented - we can have two different actions in a batch with the same rows that are considered Row equal (e.g two gets for the same rowkey, but with different column family + column qualifiers specified, are considered equal).
If my understanding of AsyncRequestFutureImpl is flawed and/or there is a reliable way to do this check with null results, I would greatly appreciate the feedback.
The only current existing caller of this method will never hit the null results case because it only checks replica gets which will always have non-null results, and the proposed additional use of this method in PR #6144 currently will also only call the method in the non-null results case, so my proposal here is to just not support the null results case and throw IllegalStateException if one does try to call isActionComplete without checking that results exist.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There is an example of the change being used in PR #6144.
Since
isActionComplete
currently only has one caller that depends on the replica action check we can move the replica action check to the caller method instead of having the check be inside isActionComplete.The case of
AsyncRequestFutureImpl
without result tracking/null
results:The function does not support a
AsyncRequestFutureImpl
withnull
results (neither before nor after my proposed changes). I don't currently see a way to reliably check for action completion from withinAsyncRequestFutureImpl
for the null results case (I don't know how common this case is in practice, but it is accommodated for acrossAsyncRequestFutureImpl
, and I test that path with Table.batch(List<Row>, null)). My understanding of that code is that in the null results case one cannot readily tell whether a given action has completed from withinAsyncRequestFutureImpl
because:setResult
.BatchErrors
, butBatchErrors
does not track the action index for the failed action, only theRow
, so even if we wanted to check whether for a given action an error was added toBatchErrors
, we couldn't because of the way that Row equality is currently implemented - we can have two different actions in a batch with the same rows that are consideredRow
equal (e.g two gets for the same rowkey, but with different column family + column qualifiers specified, are considered equal).If my understanding of
AsyncRequestFutureImpl
is flawed and/or there is a reliable way to do this check with null results, I would greatly appreciate the feedback.The only current existing caller of this method will never hit the null results case because it only checks replica gets which will always have non-null results, and the proposed additional use of this method in PR #6144 currently will also only call the method in the non-null results case, so my proposal here is to just not support the null results case and throw
IllegalStateException
if one does try to callisActionComplete
without checking that results exist.