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

Add rocksdb #74

Closed
wants to merge 18 commits into from
Closed

Add rocksdb #74

wants to merge 18 commits into from

Conversation

igalshilman
Copy link
Contributor

@igalshilman igalshilman commented Feb 3, 2023

This PR adds an initial storage and wires it.
The api in the storage_api is a low level one, we need to iterate few more times once we will learn more about the domain objects that we need to persist.

Here is an example:

#[cfg(test)]
mod tests {
    use super::*;

    #[derive(Debug, Eq, PartialEq, Clone)]
    struct MyMessage(String);

    impl From<&str> for MyMessage {
        fn from(value: &str) -> Self {
            MyMessage(value.to_string())
        }
    }

    impl StorageDeserializer for MyMessage {
        fn from_bytes(bytes: impl AsRef<[u8]>) -> Self {
            let b = bytes.as_ref().to_vec();
            MyMessage(String::from_utf8(b).unwrap())
        }
    }

    impl AsRef<[u8]> for MyMessage {
        fn as_ref(&self) -> &[u8] {
            self.0.as_bytes()
        }
    }

    fn usage_example<S: Storage>(storage: S) {
        let mut txn = storage.transaction();

        txn.put(State, "abcc-a", "1");
        txn.put(State, "abcc-b", "2");

        txn.put(State, "abcd-a", "a");
        txn.put(State, "abcd-b", "b");
        txn.put(State, "abcd-c", "c");

        txn.put(State, "abce-d", "d");
        txn.put(State, "abce-a", "3");
        txn.put(State, "abce-b", "4");

        txn.commit();

        let mut vec: Vec<(MyMessage, MyMessage)> = vec![];
        storage.copy_prefix_into(State, "abcd-b", 4, &mut vec);

        assert_eq!(vec.len(), 2);
        assert_eq!(vec.get(0).cloned().unwrap(), ("abcd-b".into(), "b".into()));
        assert_eq!(vec.get(1).cloned().unwrap(), ("abcd-c".into(), "c".into()));
    }

    #[test]
    fn test_usage_example() {
        let opts = Options {
            path: "db/".to_string(),
        };
        let storage = RocksDBStorage::new(opts);
        usage_example(storage);
    }
}

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Great work @igalshilman. The changes look good to me. I think we need to add BSD-3-Clause and ISC to the list of allowed licenses in deny.toml. I think both licenses are ok to be used by Restate. I guess we need to check licenses for build-dependencies because they might output code that is included in the final product that is licensed with their respective license. +1 for merging afterwards.

@@ -46,8 +46,7 @@ impl Options {
/// }
///
/// #[tokio::main]
/// async fn main() {
/// use tracing_instrumentation::TokioRuntime;
/// async fn main() { /// use tracing_instrumentation::TokioRuntime;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// async fn main() { /// use tracing_instrumentation::TokioRuntime;
/// async fn main() {

@@ -36,7 +36,7 @@ impl Options {
/// panic if it is executed outside of a Tokio runtime.
///
/// Example:
/// ```
/// ```ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

What is ignore used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without ignore doc-test couldn't run since tracing_instrumentation does not depend on Tokio.
I couldn't run cargo test without it. Let me know if I've made here something silly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I've tried just test instead of cargo test and it worked without the ignore.
I'm not sure I understand what is going on there, but let me revert the ignore.

using `just test` i.e.
`cargo test --all --all-features --all-targets` is able to run
the tests successfully.
@igalshilman
Copy link
Contributor Author

Great work @igalshilman. The changes look good to me. I think we need to add BSD-3-Clause and ISC to the list of allowed licenses in deny.toml. I think both licenses are ok to be used by Restate. I guess we need to check licenses for build-dependencies because they might output code that is included in the final product that is licensed with their respective license. +1 for merging afterwards.

okay Till :-) thanks for the review! I'll squash and merge.

@igalshilman igalshilman closed this Feb 3, 2023
Copy link
Contributor

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

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

Just left some comments for future :)

Comment on lines +15 to +24
fn cf_name(kind: TableKind) -> &'static str {
match kind {
State => STATE_TABLE_NAME,
Inbox => INBOX_TABLE_NAME,
Outbox => OUTBOX_TABLE_NAME,
Deduplication => DEDUP_TABLE_NAME,
PartitionStateMachine => FSM_TABLE_NAME,
Timers => TIMERS_TABLE_NAME,
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this can be a macro? You could use stringify

@@ -18,7 +19,12 @@ impl StateMachine {
///
/// We pass in the effects message as a mutable borrow to be able to reuse it across
/// invocations of this methods which lies on the hot path.
pub(super) fn on_apply(&self, command: Command, _effects: &mut Effects) {
pub(super) fn on_apply<S: StorageReader>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps the bound should be at impl level here

Copy link
Contributor

Choose a reason for hiding this comment

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

Reason is that I see this impl block evolving like:

  • on_apply (parent apply function)
  • on_message_x
  • on_message_y

All of these will need the same storage bound, as the entrypoint is still on_apply

slinkydeveloper added a commit to slinkydeveloper/restate that referenced this pull request Mar 21, 2024
…8d559..61ae44b2

61ae44b2 Update documentation on error codes (restatedev#77)
576a1b26 Add HandlerType. Fix restatedev#1092 (restatedev#76)
0624d092 Payload schema for input and output. This provides basic format awareness. (restatedev#74)

git-subtree-dir: crates/service-protocol/service-protocol
git-subtree-split: 61ae44b2175c2a54c2d7a138a3368df17f389ebb
slinkydeveloper added a commit to slinkydeveloper/restate that referenced this pull request Mar 26, 2024
…8d559..29b28f98

29b28f98 Modify Input/Output schema in deployment_manifest_schema.json (restatedev#80)
26d91e69 Replace protocol Empty message with custom one, so we remove the dependency on protobuf built-in messages (restatedev#79)
61ae44b2 Update documentation on error codes (restatedev#77)
576a1b26 Add HandlerType. Fix restatedev#1092 (restatedev#76)
0624d092 Payload schema for input and output. This provides basic format awareness. (restatedev#74)

git-subtree-dir: crates/service-protocol/service-protocol
git-subtree-split: 29b28f9867734bc01dd47c4666d9d56c90b626f5
slinkydeveloper added a commit to slinkydeveloper/restate that referenced this pull request Mar 26, 2024
…8d559..29b28f98

29b28f98 Modify Input/Output schema in deployment_manifest_schema.json (restatedev#80)
26d91e69 Replace protocol Empty message with custom one, so we remove the dependency on protobuf built-in messages (restatedev#79)
61ae44b2 Update documentation on error codes (restatedev#77)
576a1b26 Add HandlerType. Fix restatedev#1092 (restatedev#76)
0624d092 Payload schema for input and output. This provides basic format awareness. (restatedev#74)

git-subtree-dir: crates/service-protocol/service-protocol
git-subtree-split: 29b28f9867734bc01dd47c4666d9d56c90b626f5
slinkydeveloper added a commit that referenced this pull request Mar 26, 2024
…8d559..29b28f98

29b28f98 Modify Input/Output schema in deployment_manifest_schema.json (#80)
26d91e69 Replace protocol Empty message with custom one, so we remove the dependency on protobuf built-in messages (#79)
61ae44b2 Update documentation on error codes (#77)
576a1b26 Add HandlerType. Fix #1092 (#76)
0624d092 Payload schema for input and output. This provides basic format awareness. (#74)

git-subtree-dir: crates/service-protocol/service-protocol
git-subtree-split: 29b28f9867734bc01dd47c4666d9d56c90b626f5
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