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

Extend some doccomments based on Github discussions #1237

Merged
merged 4 commits into from
Jan 6, 2021

Conversation

@vi vi force-pushed the doc_fixes branch 2 times, most recently from e90ae89 to 7931f21 Compare January 5, 2021 14:10
src/tree.rs Outdated
@@ -335,6 +337,9 @@ impl Tree {
/// assert_eq!(&processed.get(b"k3").unwrap().unwrap(), b"yappin' ligers");
/// # Ok(()) }
/// ```
///
/// Note simple string prepeding in the example above should be natively changed
/// to some complex operation, as transactions may be tried multiple times.
Copy link
Owner

Choose a reason for hiding this comment

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

incorrect. transactions are serializable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall I try to construct some concrete example (involving external I/O from within a transaction) where this break down?

src/lib.rs Outdated
//! Sled is not designed to be used by multiple processes simultaneously.
//! Although Sled tries to use advisory filesystem locking to
//! protect the database from accidental access from foreign
//! processes, this should not be reiled upon.
Copy link
Owner

Choose a reason for hiding this comment

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

no need for this comment here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where should it be instead?

I don't see other mentions of the shared multiprocess use cases in the documentation, yet questions do arise.

Copy link
Owner

Choose a reason for hiding this comment

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

I've reworded this and placed it high up on the main README 66cf957

src/db.rs Outdated
@@ -5,6 +5,9 @@ use crate::*;
/// The `sled` embedded database! Implements
/// `Deref<Target = sled::Tree>` to refer to
/// a default keyspace / namespace / bucket.
///
/// Note that dropping `Db` does not gurantee closing the database.
/// Sled may still access the underlying directory under the hood.
Copy link
Owner

Choose a reason for hiding this comment

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

no need for this comment here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where should it be (assuming the comment itself is not wrong, just misplaced)?

Filesystem usage aspect may be important for users, especially, for example, if the same process manages mounts/unmounts of the filesystem that backs the database.

src/tree.rs Outdated
@@ -231,6 +231,8 @@ impl Tree {

/// Perform a multi-key serializable transaction.
///
/// User-provided function `f` may be run multiple times in case of conflicts.
///
Copy link
Owner

Choose a reason for hiding this comment

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

please get rid of all of the other comments except this one, which is the only one we discussed earlier.

@spacejam
Copy link
Owner

spacejam commented Jan 6, 2021

Apologies for my harsh tone. I was feeling a bit frustrated at the feeling that I have been repeating the same thing, but better docs are a great way to address this frustration. I've reworded a few of your comments and placed them in places I hope will be visible to people who will encounter them. 66cf957

@spacejam spacejam merged commit 77f7c85 into spacejam:master Jan 6, 2021
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