Skip to content

Conversation

@wey1and
Copy link
Contributor

@wey1and wey1and commented Jan 21, 2022

Added refactoring for RequestRetryPolicies and classes in exceptions.errors package

Added new Exception called TarantoolNoSuchProcedureException for this message, so that the user can specify it in the retry policy

InnerErrorMessage:
code: 33
message: Procedure '*' is not defined

Closes #159

@wey1and wey1and force-pushed the feature/waiting-for-crud branch from 9d5ed75 to 10880f7 Compare January 25, 2022 17:04
@wey1and wey1and changed the title Added test for reproduce "Procedure 'crud.*' is not defined" problem Added TarantoolNoSuchProcedureException and TarantoolErrors refactoring Jan 25, 2022
@wey1and wey1and requested review from ArtDu and vrogach2020 February 2, 2022 13:16
Copy link
Collaborator

@akudiyar akudiyar left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, but IT tests need more consideration.

vrogach2020
vrogach2020 previously approved these changes Feb 7, 2022
@vrogach2020
Copy link
Contributor

@wey1and please do not forget to include backwards compatibility changes into changelog

@ArtDu
Copy link
Contributor

ArtDu commented Feb 8, 2022

nit:
squash commits and make one commit message with list of what has changed. Either squash commits conveniently with a commit message. Because +459 −270 so hard to review without a general description or breakdown into commits.

One commit is expected before the merge with a full description for a simplified review and a clean commit history.

@wey1and wey1and changed the title Added TarantoolNoSuchProcedureException and TarantoolErrors refactoring Refactoring for RequestRetryPolicies and classes in exceptions.errors package Feb 8, 2022
@ArtDu
Copy link
Contributor

ArtDu commented Feb 8, 2022

nit:

The PR title does not reveal what is in the PR. It seems better not to use the word refactoring, because there was not only a refactoring, but also changes in API of the exceptions.errors package. And since the external behavior of the code will change, then you can not use this word, it seems better to use a CHANGE or something like that.

Just this will be a generalized version of what is happening in PR. And in the description, you can just write that a refactoring has taken place, the API has changed and a new exception has been added and why it was added. And I expect to see this also in the final squashed commit message.

ArtDu
ArtDu previously approved these changes Feb 8, 2022
Copy link
Contributor

@ArtDu ArtDu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@akudiyar akudiyar left a comment

Choose a reason for hiding this comment

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

LGTM.

BTW there is a truncate_space() test failing on 1.10 -- we need to fix it.

@vrogach2020
Copy link
Contributor

@akudiyar Created an issue on truncate
#164

@akudiyar akudiyar merged commit 5ed7e3e into master Feb 10, 2022
@vrogach2020 vrogach2020 deleted the feature/waiting-for-crud branch February 10, 2022 08:50
wey1and added a commit that referenced this pull request Feb 10, 2022
… package (#162)

Added TarantoolNoSuchProcedureException and refactored TarantoolErrors for parsing errors from MsgPack maps
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.

Choose required cartridge roles to wait at connect

5 participants