Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add revert failed cb #550

Merged
merged 6 commits into from
Sep 18, 2024
Merged

add revert failed cb #550

merged 6 commits into from
Sep 18, 2024

Conversation

EvgeniiVR
Copy link
Contributor

@EvgeniiVR EvgeniiVR commented Aug 30, 2024

Hello @olofhagsand
Following this conversation #517

Could you pls check if it possible to merge this PR?

Current callback order:

Aug 30 05:33:04.906929: Plugin 01: BEGIN
Aug 30 05:33:04.906944: Plugin 02: BEGIN
Aug 30 05:33:04.906947: Plugin 03: BEGIN
Aug 30 05:33:04.906950: Plugin 04: BEGIN
Aug 30 05:33:04.906969: Plugin 01: VALIDATE
Aug 30 05:33:04.906972: Plugin 02: VALIDATE
Aug 30 05:33:04.906974: Plugin 03: VALIDATE
Aug 30 05:33:04.906977: Plugin 04: VALIDATE
Aug 30 05:33:04.906980: Plugin 01: COMPLETE
Aug 30 05:33:04.906982: Plugin 02: COMPLETE
Aug 30 05:33:04.906984: Plugin 03: COMPLETE
Aug 30 05:33:04.906986: Plugin 04: COMPLETE
Aug 30 05:33:04.906995: Plugin 01: COMMIT
Aug 30 05:33:04.906998: Plugin 02: COMMIT
Aug 30 05:33:04.907000: Plugin 03: COMMIT
Aug 30 05:33:04.907002: Plugin 04: COMMIT FAILED
Aug 30 05:33:04.907009: Plugin 04: REVERT FAILED
Aug 30 05:33:04.907012: Plugin 03: REVERT
Aug 30 05:33:04.907014: Plugin 02: REVERT
Aug 30 05:33:04.907016: Plugin 01: REVERT
Aug 30 05:33:04.907019: Plugin 01: ABORT
Aug 30 05:33:04.907022: Plugin 02: ABORT
Aug 30 05:33:04.907024: Plugin 03: ABORT
Aug 30 05:33:04.907026: Plugin 04: ABORT

@olofhagsand
Copy link
Member

The compile error for fcgi is fixed in commit 73183c4. Can you please rebase?

@EvgeniiVR
Copy link
Contributor Author

HI @olofhagsand!
I've updated forked repo up to date. Pls check.

@@ -848,6 +862,8 @@ plugin_transaction_commit_all(clixon_handle h,
while ((cp = clixon_plugin_each(h, cp)) != NULL) {
i++;
if (plugin_transaction_commit_one(cp, h, td) < 0){
/*First make an effort ro revert transaction for the failed plugin*/
plugin_transaction_revert_failed(cp, h, td);
Copy link
Member

Choose a reason for hiding this comment

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

Since this is called after a commit failed, maybe the name of the callback should be "commit-failed"?
Also, maybe one should handle the return value of the call, could it be that one wants to stop the following plugin_transaction_revert_all() call, or do you want it to continue uninterrupted?
(Minor: please have an initial space in the comment: /*First make an --> /* First make an

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@olofhagsand ,
Since this is called after a commit failed, maybe the name of the callback should be "commit-failed"?
Agree w/ you, will rename.
Also, maybe one should handle the return value of the call, could it be that one wants to stop the following plugin_transaction_revert_all() call, or do you want it to continue uninterrupted?
I believe it's better to follow the same logic as normal revert. The difference should be in the code placed inside this callback. Developer have to consider if the changes within transaction data has been succesfully applied to the underlying system during commit or not and incrementally apply reverse rules and operations. But all of this is up to developer and customization features.
(Minor: please have an initial space in the comment: /*First make an --> /* First make an
yep, will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @olofhagsand,
I've made changes discussed above, pls check.

@olofhagsand
Copy link
Member

Also, the transaction callback description needs to be amended https://clixon-docs.readthedocs.io/en/latest/backend.html#transactions
Can you please make a PR here: https://github.com/clicon/clixon-docs/blob/master/backend.rst

@EvgeniiVR
Copy link
Contributor Author

Also, the transaction callback description needs to be amended https://clixon-docs.readthedocs.io/en/latest/backend.html#transactions Can you please make a PR here: https://github.com/clicon/clixon-docs/blob/master/backend.rst

done with this
clicon/clixon-docs#9

@olofhagsand olofhagsand merged commit 7ea344c into clicon:master Sep 18, 2024
6 checks passed
krihal pushed a commit that referenced this pull request Sep 23, 2024
* add revert failed cb

* add revert failed cb

* add commit failed cb

* add commit failed cb
krihal pushed a commit that referenced this pull request Sep 23, 2024
* add revert failed cb

* add revert failed cb

* add commit failed cb

* add commit failed cb
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