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

WT-13948 Deprecate rollback_reason API #11519

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

LX6T
Copy link
Contributor

@LX6T LX6T commented Jan 30, 2025

After milestone 2 and WT-13947 all rollback reason messages should have been covered by the get_last_error API. This ticket involves deprecating the rollback reason API. In this case, we should be changing all existing rollback_reason related tests to use the get_last_error API.

Copy link

github-actions bot commented Jan 30, 2025

Thanks for creating a pull request! Please answer the questions below by editing this comment.

Type of change made in this PR

  • Functional change
  • Test-only change
  • Refactor-only change
  • Other non-functional change

What makes this change safe?

Answering this question helps the reviewers understand where they should focus their attention. Please consider these prompts:

  • How risky is this change? Why?
    Medium risk, as the change touches many areas of the code I'm not familiar with and also have low code coverage, as well as altering various APIs relied upon by the server.

  • What tests are you adding, changing or relying on? Why?
    Deprecating rollback_reason involved removing the python test file (test_txn27.py) associated with it, and altering the format test snap.c to instead use the session's err_info struct.

  • What, if anything, are you concerned about that you'd like the reviewer to focus on?
    See if there are any mentions of rollback_reason that I've missed, and check all the replacements with get_last_error/err_info are appropriate.

References:

Checklist before requesting a review

  • I have performed a self-review of my code.
  • I have made corresponding changes to the documentation, or no documentation change is needed.
  • I have tests that show my change is correct, or have described above why functional test changes are not required.

Copy link

wiredtiger-prbot bot commented Jan 30, 2025

Test coverage is very low, please refer to the Code change/coverage report links below and try to improve it if feasible.

Metric (for added/changed code) Coverage
Line coverage 64% (7/11)
Branch coverage 50% (6/12)

⚠️ This PR touches methods that have an extremely high complexity score!

  • In src/evict/evict_lru.c the complexity of __wti_evict_app_assist_worker has increased by 2 to 34.

@LX6T LX6T marked this pull request as ready for review January 31, 2025 00:19
@LX6T
Copy link
Contributor Author

LX6T commented Jan 31, 2025

Running a full patch build to check that nothing else relies on rollback_reason

@jiechenbo
Copy link
Contributor

I wouldn't deem this as high risk, maybe medium risk. High risk would refer to no testing can be done to ensure this is correct.

Copy link
Contributor

@jiechenbo jiechenbo left a comment

Choose a reason for hiding this comment

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

Nice go at the deprecation, have a few comments

src/evict/evict_lru.c Outdated Show resolved Hide resolved
src/evict/evict_lru.c Outdated Show resolved Hide resolved
src/include/txn.h Show resolved Hide resolved
src/include/txn_inline.h Show resolved Hide resolved
src/txn/txn.c Outdated Show resolved Hide resolved
@LX6T LX6T requested a review from jiechenbo February 2, 2025 22:34
Copy link
Contributor

@DylanSHLiang DylanSHLiang left a comment

Choose a reason for hiding this comment

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

LGTM!

src/include/txn_inline.h Outdated Show resolved Hide resolved
src/txn/txn.c Outdated Show resolved Hide resolved
Co-authored-by: Etienne Petrel <[email protected]>
@LX6T LX6T requested review from jiechenbo and lukech February 3, 2025 03:42
@LX6T LX6T requested a review from etienneptl February 3, 2025 05:38
Copy link
Contributor

@etienneptl etienneptl 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
Contributor

@jiechenbo jiechenbo left a comment

Choose a reason for hiding this comment

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

Have two comments, can you also do a full wiredtiger patch build and once MongoDB has removed the usage of rollback_reason to also do a full MongoDB patch build?


# test_txn27.py
# Test that the API returning a rollback error sets the reason for the rollback.
class test_txn27(wttest.WiredTigerTestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

is this test still valuable to have? Maybe its worth placing it under a error_info.py test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there's anything in there that isn't covered by test_error_info02.py, I can double check with @DylanSHLiang.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just checked, he says nothing of value will be lost.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather keep it cause its a completed written test, there is no harm in having potential extra coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/session/session_api.c Show resolved Hide resolved
@LX6T LX6T requested a review from jiechenbo February 4, 2025 00:27
@LX6T
Copy link
Contributor Author

LX6T commented Feb 4, 2025

Hi @lukech, for context we thought it'd be worth adding you as a reviewer to this PR since it involves deprecating a feature that could potentially have wider consequences for Mongo or any other customers currently relying on it.
The rollback_reason API is currently also being removed from Mongo by the RSS team, so once that's done we want to make sure it's safe to be removed from WT. Do you know of any other places the API needs to be removed from, or any reasons that this change might be problematic?

@lukech
Copy link
Contributor

lukech commented Feb 4, 2025

@LX6T thanks for the useful context. I'm not aware of any application/customer other than Mongo server that makes use of this rollback reason API. Just to confirm you won't merge this PR until after Mongo Server removes the usage of this API, right? Normally, API interface changes need to update the dist/api_data.py file and corresponding documentation. I'm guessing this rollback reason removal change won't affect the public API interface but rather impact the internal structure. Please confirm. Testing wise, I'd suggest running a full WT patch build and a mongo server patch build to make sure no breakage before merge.

@LX6T
Copy link
Contributor Author

LX6T commented Feb 4, 2025

@LX6T thanks for the useful context. I'm not aware of any application/customer other than Mongo server that makes use of this rollback reason API. Just to confirm you won't merge this PR until after Mongo Server removes the usage of this API, right? Normally, API interface changes need to update the dist/api_data.py file and corresponding documentation. I'm guessing this rollback reason removal change won't affect the public API interface but rather impact the internal structure. Please confirm. Testing wise, I'd suggest running a full WT patch build and a mongo server patch build to make sure no breakage before merge.

Thanks for the response @lukech, I can confirm that we won't merge this until get_rollback_reason is fully removed from Mongo, and it isn't mentioned in api_data.py so there's nothing to update there. The only documentation change is in wiredtiger.in, I'm pretty sure that's the only place it's mentioned. And I'll definitely run some full patch builds once the Mongo side is all done.

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.

5 participants