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

refactor(storage): create new read snapshot to read for StateStore trait #20172

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

wenym1
Copy link
Contributor

@wenym1 wenym1 commented Jan 15, 2025

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Previously we have two traits to read in storage, LocalStateStore and StateStoreRead. One difference in the two traits is that, for StateStoreRead, we have to specify the epoch to read on, while for LocalStateStore, since its read methods always read from the latest snapshot, we don't need to specify the epoch.

In #20153, we introduce a new associated type FlushedSnapshotReader, which also reads data from the latest snapshot. However, since it implements the StateStoreRead trait, when we call its read methods, we still need to specify the epoch, which is actually unnecessary. Besides, it will be cleaner if we are able to make the read methods more unified in the trait definition.

Therefore, in this PR, we will remove the epoch parameter from the read methods of StateStoreRead trait. Instead, we add a new method new_read_snapshot to StateStore, whose return type is a new associated type ReadSnapshot that implements StateStoreRead. In the new_read_snapshot method, we specify the epoch and table_id to read. In the implementation of HummockStorage on new_read_snapshot, we also do try_wait_epoch on the epoch, so that the returned ReadSnapshot is ready to read.

Changes in the trait definition is as followed:

pub trait StateStoreRead: StaticSendSync {
    fn get(
        &self,
        key: TableKey<Bytes>,
        //  epoch: u64, // epoch is removed
        ...

    fn iter(
        &self,
        key_range: TableKeyRange,
        //  epoch: u64, // epoch is removed
        ...

    fn rev_iter(
        &self,
        key_range: TableKeyRange,
        //  epoch: u64, // epoch is removed
        ...
}

pub trait StateStore: StateStoreReadLog + StaticSendSync + Clone {
    type ReadSnapshot: StateStoreRead + Clone;

    fn new_read_snapshot(
        &self,
        epoch: HummockReadEpoch,
        options: NewReadSnapshotOptions,
    ) -> impl StorageFuture<'_, Self::ReadSnapshot>;
    ...
}

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

  • My PR needs documentation updates.
Release note

Base automatically changed from yiming/state-store-flushed-reader to main January 16, 2025 07:21
@wenym1 wenym1 force-pushed the yiming/state-store-trait-read-snapshot branch from 2772249 to 50f8a9d Compare January 16, 2025 07:51
@wenym1 wenym1 requested review from hzxa21 and Li0k January 16, 2025 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant