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-13502 Add 'backup_checkpoint' to 'query_timestamp'. #11011

Merged
merged 11 commits into from
Oct 16, 2024

Conversation

sueloverso
Copy link
Member

Add a configuration setting to be able to query the timestamp that is the timestamp of the checkpoint pinned by an open backup cursor.

@luke-pearson and @keitharnoldsmith here is an implementation of what we discussed. It basically works but I didn't go into situations where timestamps aren't in use or checkpoints sometimes taken without the stable or weird cases like that. This is the basic functionality and plumbing. But I did make a test!

Copy link

github-actions bot commented Sep 5, 2024

Thanks for creating a pull request! The below questions and checklist are intended to help with verifying your change is well tested. Response is optional, but if you choose to respond please edit this comment.

What makes this change safe?

A good answer to this question helps the reviewers understand where they should focus their attention, so please consider these questions:

  • Is the change risky or not? Why?
  • What tests are you adding or changing? Why?
  • What existing tests are you relying on?
  • What, if anything, are you concerned about that you'd like the reviewer to focus on?
    References:
  • Risk level guide
  • Testing frameworks

Checklist before requesting a review

  • I have performed a self-review of my code.
  • I have made corresponding changes to the documentation (if applicable).
  • I have added/updated tests that demonstrate my fix is effective or that my feature works correctly.

@tod-johnson-mongodb
Copy link
Contributor

LGTM. I didn't hear the discussion so I didn't mark it approved.

@luke-pearson
Copy link
Contributor

Closing as I don't think this solution is relevant anymore.

@sueloverso sueloverso reopened this Oct 8, 2024
@sueloverso
Copy link
Member Author

Reopening this PR because it is now relevant.

@sueloverso
Copy link
Member Author

@keitharnoldsmith @luke-pearson and @tod-johnson-mongodb this PR has been revived and is now part of the solution for the server code. It is now pulled to the current state of develop and ready for review.

Copy link

wiredtiger-prbot bot commented Oct 8, 2024

Test coverage is too low, this change probably shouldn't be merged as-is.

Metric (for added/changed code) Coverage
Line coverage 33% (1/3)
Branch coverage 10% (1/10)

@sueloverso
Copy link
Member Author

Right now the code will return a WT_TS_NONE timestamp if a backup cursor isn't opened. It could return an error instead.

@sueloverso
Copy link
Member Author

@keitharnoldsmith and @tod-johnson-mongodb can you please review this? It is real now and this PR has been reopened.

Copy link
Contributor

@luke-pearson luke-pearson left a comment

Choose a reason for hiding this comment

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

LGTM, i think one more comment in txn_timestamp.c may be helpful. See https://github.com/wiredtiger/wiredtiger/pull/11011/files#r1801689373

@sueloverso sueloverso added this pull request to the merge queue Oct 15, 2024
github-merge-queue bot pushed a commit that referenced this pull request Oct 15, 2024
Add a configuration setting to be able to query the timestamp that is
the timestamp of the checkpoint pinned by an open backup cursor.

@luke-pearson and @keitharnoldsmith here is an implementation of what we
discussed. It basically works but I didn't go into situations where
timestamps aren't in use or checkpoints sometimes taken without the
stable or weird cases like that. This is the basic functionality and
plumbing. But I did make a test!
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 15, 2024
@sueloverso sueloverso added this pull request to the merge queue Oct 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 15, 2024
@sueloverso sueloverso added this pull request to the merge queue Oct 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 16, 2024
@sueloverso sueloverso added this pull request to the merge queue Oct 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 16, 2024
@sueloverso sueloverso added this pull request to the merge queue Oct 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 16, 2024
@sueloverso sueloverso added this pull request to the merge queue Oct 16, 2024
Merged via the queue into develop with commit ba820be Oct 16, 2024
10 checks passed
@sueloverso sueloverso deleted the wt-13502-backup-timestamp branch October 16, 2024 20:46
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.

3 participants