Skip to content

Conversation

@zuston
Copy link
Member

@zuston zuston commented Dec 19, 2025

Purpose

This change includes the hidden internal flush time during KV flushing.
RocksDBWriteBatchWrapper may trigger a flush implicitly when performing put or delete operations, which caused the previous latency measurement to be incomplete.

Brief change log

Move startTime to the first line of the method.

Tests

API and Format

Documentation

Copy link
Member

@wuchong wuchong left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I think you are right. The previous implementation failed to account for certain flush operations.

However, simply moving the start time before all put calls might bundle multiple flush operations into a single latency measurement, which could distort the metrics.

A better approach would be to move the flushLatencyHistogram into RocksDBWriteBatchWrapper and update it directly within RocksDBWriteBatchWrapper#flush. This ensures accurate, per-flush latency tracking.

@zuston
Copy link
Member Author

zuston commented Dec 19, 2025

A better approach would be to move the flushLatencyHistogram into RocksDBWriteBatchWrapper and update it directly within RocksDBWriteBatchWrapper#flush. This ensures accurate, per-flush latency tracking.

Make sense. Let me follow your idea. thanks @wuchong

@wuchong
Copy link
Member

wuchong commented Dec 23, 2025

Hi @zuston , Just checking in to see how things are going with this issue. No pressure at all — I'm just wondering if you're still able to work on it. If you're busy or need help, we'd be happy to assist or reassign it. Thanks again for taking it on!

@zuston
Copy link
Member Author

zuston commented Dec 23, 2025

Hi @zuston , Just checking in to see how things are going with this issue. No pressure at all — I'm just wondering if you're still able to work on it. If you're busy or need help, we'd be happy to assist or reassign it. Thanks again for taking it on!

I will continue to followup this and maybe later in this week

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