Added is_retriable() to FlussError#422
Conversation
|
I ran some tests and didnt see any propagation issues, let me know if I missed something |
leekeiabstraction
left a comment
leekeiabstraction
left a comment
There was a problem hiding this comment.
TY for the PR, left a question
|
|
||
| /// Returns true if retrying the request may succeed. Similar toJava's RetriableException hierarchy. | ||
| static constexpr bool IsRetriable(int32_t code) { | ||
| return code == NETWORK_EXCEPTION || code == NOT_LEADER_OR_FOLLOWER || |
There was a problem hiding this comment.
Can you point me to where error code for retriable errors are defined on java side?
There was a problem hiding this comment.
I do not see the equivalent Java exceptions for the following (not exhaustive)
- INVALID_COORDINATOR_EXCEPTION
- FENCED_LEADER_EPOCH_EXCEPTION
- RETRIABLE_AUTHENTICATE_EXCEPTION
There was a problem hiding this comment.
feedback much appreciated,I should checked the fluss source myself. I asked an LLM to enumerate all the exceptions the extend RetriableException, but it seems to have given for ApiException and others as well. My apologies.
Apart from this having looked at the source myself ,there were a few others i think I should add, such as CorruptMessageException and SchemaNotExistException. let me know since i think the rest are correct
There was a problem hiding this comment.
I do not see the equivalent Java exceptions for the following (not exhaustive)
* INVALID_COORDINATOR_EXCEPTION * FENCED_LEADER_EPOCH_EXCEPTION * RETRIABLE_AUTHENTICATE_EXCEPTION
apart form these three and FencedTieringEpochException, the others have been kept similar to Java
There was a problem hiding this comment.
STORAGE_EXCEPTION seems to be missing too, can you push a new commit?
There was a problem hiding this comment.
added :
CorruptMessageSchemaNotExistCorruptRecordExceptionStorageException
and removed the incorrect ones
toxicteddy00077
force-pushed
the
fluss-retri-error
branch
from
d7d3784 to
a3ac0ee
Compare
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@toxicteddy00077 TY for the PR, left comment, PTAL
LogStorageException (code 10)
KvStorageException (code 11)
The are subclasses of StorageException, so should be retriable
toxicteddy00077
force-pushed
the
fluss-retri-error
branch
from
a3ac0ee to
bed9036
Compare
|
|
||
| /// Returns true if retrying the request may succeed. Mirrors Java's RetriableException hierarchy. | ||
| static constexpr bool IsRetriable(int32_t code) { | ||
| return code == NETWORK_EXCEPTION || code == CORRUPT_MESSAGE || |
There was a problem hiding this comment.
shall we update docs, btw?
There was a problem hiding this comment.
have a look and let me know
There was a problem hiding this comment.
@toxicteddy00077 I only see cpp docs updated.
What about rust, python?
There was a problem hiding this comment.
yeah thats why I asked you to have a look. Just wanted to confirm if this was fine. Now I'll make the updates ASAP thanks
toxicteddy00077
force-pushed
the
fluss-retri-error
branch
from
bed9036 to
65def98
Compare
toxicteddy00077
force-pushed
the
fluss-retri-error
branch
from
65def98 to
e967036
Compare
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@toxicteddy00077 Ty, LGTM
4 participants
Purpose
Linked issue: close #320
Description
Added basic
is_retriable()implementation, similar to Java's RetriableException .