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

Re-instantiate replay progress reporting #1096

Merged
merged 2 commits into from
Apr 19, 2024
Merged

Conversation

kim
Copy link
Contributor

@kim kim commented Apr 16, 2024

Description of Changes

Output was wrongly logged at debug, and percentage reporting was missing a way to obtain the max offset to expect.

Note that this may need some reconsideration once we apply history suffixes. Do not rely on the output as is.

API and ABI breaking changes

Nada

Expected complexity level and risk

1

Testing

  • Manual testing: spin up an existing database and observe the log output

@kim kim added the release-any To be landed in any release window label Apr 16, 2024
@kim kim requested review from jdetter and bfops April 16, 2024 20:20
last_logged_percentage = percentage;
}
// Print _something_ even if we don't know what's still ahead.
} else if tx_offset % 10_000 == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: In what case would this happen? Would this flood the log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would happen for future implementations of the History trait which, for some reason, can’t determine how many txs are yet to come. For example, apply could be continuously fed from a replication stream.

In this case, we’d print a log every 10k txs, which seems reasonable.

let progress = |tx_offset: u64| {
if let Some(max_tx_offset) = max_tx_offset {
let percentage = f64::floor((tx_offset as f64 / max_tx_offset as f64) * 100.0) as i32;
if percentage > last_logged_percentage && percentage % 10 == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Our general UX issue with this sort of logging would be that if it takes a really long time to get to the first 10% we really don't know how long it will take to complete the reload until we see that first 10% log. I would suggest changing this so that it outputs a log every 15 seconds instead of doing a percentage based log. If its not an easy change then don't worry about it this is fine for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, ya, not sure if we'd want to spawn a thread just for this, or ask for the system time a couple of million times. I'll take a note that we might want to stick a tokio::runtime::Handle into RelationalDB for these kinds of things.

Copy link
Collaborator

@jdetter jdetter left a comment

Choose a reason for hiding this comment

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

This is tested + working. I had some small feedback but nothing that would prevent merging. Thanks Kim!

kim added 2 commits April 19, 2024 11:19
Useful for reporting replay progress.
Include note that it is somewhat similar to `std::iter::Iterator::size_hint`.
The percentage is calculated as starting from the zero offset, although
that may change in the future.
@kim kim force-pushed the kim/commitlog2/replay-progress branch from 548bc2e to c675c8e Compare April 19, 2024 09:21
@kim kim enabled auto-merge April 19, 2024 09:21
@kim kim added this pull request to the merge queue Apr 19, 2024
Merged via the queue into master with commit bee6ae1 Apr 19, 2024
6 checks passed
bfops pushed a commit that referenced this pull request Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants