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.

Sign up for GitHub

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

Jump to bottom

WT-13952 Move session_set_last_error out of session API and move last error from internal session to external session #11373

Merged
merged 39 commits into from
Jan 7, 2025
Merged

WT-13952 Move session_set_last_error out of session API and move last error from internal session to external session #11373

merged 39 commits into from
Jan 7, 2025

Conversation

DylanSHLiang
Copy link
Contributor

Modified session_set_last_error function to outside of session API and moved last error from internal session to external session when releasing the internal session.

LX6T and others added 29 commits December 23, 2024 04:36
@LX6T
WT-13864 Capture message and error from WT_RET_MSG and WT_ERR_MSG
48b3f29
@LX6T
Moved new test to test_error_info.py
042e293
@LX6T
Removed unused import
bb55ac8
@LX6T
Added unit test for session_set_last_error
7ae2558
@LX6T
Return if null err_msg_content rather than abort
33bd751
@LX6T
No-op if message content is null or if session is mock
0a4b63b
@LX6T
Added unit tests for WT_RET_MSG and WT_ERR_MSG
c13b3b9
@LX6T
Pass WT_SESSION_IMPL into __wt_session_set_last_error instead of WT_S…
6fe3e3c 
…ESSION
@LX6T
Updated mock session and altered __wt_session_set_last_error
6c29c7c
@LX6T
Remove non-null error message assertion
e2aa18c
@LX6T
Alter memory freeing in mock session destructor
a872aff
@LX6T
Free message string from dummy sessions created during connection setup
92c4f22
@LX6T
Call set_last_error only once after var-arg formatting for json
c605e7c
@LX6T
Merge branch 'develop' into wt-13864-capture-message-and-error
5e99138
@LX6T
Initial error message values should be empty string
308cf3f
@LX6T
Added additional return value checks to the catch2/python tests
472fff9
@LX6T
Moved __wt_session_set_last_error calls inside conditionals to preven…
ac44b58 
…t repeated calls
@LX6T
Ensure that errors are recorded for external sessions only
6bd2119
@LX6T
Make early return conditions more explicit in __wt_session_set_last_e…
5999a54 
…rror
@LX6T
Free message from dummy session just in case
6f0384f
@LX6T
Revert explicit check
b7211d0
@LX6T
Revert
35c983d
@LX6T
Only set the error if it results in a change
5ca5fb3
@LX6T
Make change condition more explicit
9feef00
@LX6T
Only record the error for external sessions
4eb7655
@LX6T
Set dummy session as WT_SESSION_INTERNAL
3a213f9
@LX6T
No longer setting dummy sessions as WT_SESSION_INTERNAL
91709f7
@DylanSHLiang
Merge branch 'wt-13864-capture-message-and-error' of https://github.c…
88ede58 
…om/wiredtiger/wiredtiger into wt-13952-move-last-error-from-internal-session-to-external-session
@DylanSHLiang
reverted changes that belong to a separate branch
8235023
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 3, 2025 •
edited by DylanSHLiang
Loading

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?

This change is low risk as the changes are refactoring the code and extending on existing code. The refactoring code does not change behaviour so it should not affect existing code, and the extension on existing code also does not change behaviour. Therefore, the python and unit tests should be enough to ensure correctness.

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.

jiechenbo
jiechenbo approved these changes Jan 3, 2025
View reviewed changes
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.

LGTM

@jiechenbo jiechenbo self-requested a review January 3, 2025 02:51
@jiechenbo
Copy link
Contributor

Actually I realised that the copying of errors from internal sessions to external sessions have not be done yet. Could you potentially be missing those changes?

@DylanSHLiang
Copy link
Contributor Author

Actually I realised that the copying of errors from internal sessions to external sessions have not be done yet. Could you potentially be missing those changes?

Fixed in this commit

@evergreen-ci-prod evergreen-ci-prod bot mentioned this pull request Jan 6, 2025
Closed
@jiechenbo jiechenbo requested a review from jeremythorp January 6, 2025 21:50
jiechenbo
jiechenbo reviewed Jan 6, 2025
View reviewed changes
Copy link
Contributor

@jiechenbo jiechenbo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Close to LGTM, i would also update the comment within __wti_schema_internal_session function, to mention that we do not clear the new flag because it follows from an external session.

src/schema/schema_util.c Outdated Show resolved Hide resolved
src/session/session_helper.c Outdated Show resolved Hide resolved
src/session/session_helper.c Outdated Show resolved Hide resolved
test/catch2/misc_tests/test_session_set_last_error.cpp Outdated Show resolved Hide resolved
src/session/session_helper.c Show resolved Hide resolved
zunyiliu
zunyiliu approved these changes Jan 6, 2025
View reviewed changes
Copy link
Contributor

@zunyiliu zunyiliu left a comment

Choose a reason for hiding this comment

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

LGTM, please fix the redundant code for F_SET as in my last comment.

src/session/session_api.c Outdated Show resolved Hide resolved
jeremythorp
jeremythorp approved these changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@jeremythorp jeremythorp left a comment

Choose a reason for hiding this comment

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

LGTM!

jiechenbo
jiechenbo approved these changes Jan 7, 2025
View reviewed changes
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.

LGTM!

@LX6T LX6T added this pull request to the merge queue Jan 7, 2025
Merged via the queue into develop with commit c3fbce2 Jan 7, 2025
10 checks passed
@LX6T LX6T deleted the wt-13952-move-last-error-from-internal-session-to-external-session branch January 7, 2025 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LX6T LX6T LX6T left review comments

@zunyiliu zunyiliu zunyiliu approved these changes

@jiechenbo jiechenbo jiechenbo approved these changes

@jeremythorp jeremythorp jeremythorp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DylanSHLiang @jiechenbo @jeremythorp @zunyiliu @LX6T