Rationale and Goals

As every Rust programmer knows, the language has many powerful features, and there are often several patterns which can express the same idea. Also, as every professional programmer comes to discover, code is almost always read far more than it is written.

Thus, we choose to use a consistent set of idioms throughout our code so that it is easier to read and understand for both existing and new contributors.

Unsafe and Platform-Dependent conditional compilation

Avoid unsafe Rust

One of the main reasons to use Rust as an implementation language is its strong memory safety guarantees; Almost all of these guarantees are voided by the use of unsafe. Thus, unless there is an excellent reason and the use is discussed beforehand, it is unlikely HoraeDB will accept patches with unsafe code.

We may consider taking unsafe code given:

• performance benchmarks showing a very compelling improvement
• a compelling explanation of why the same performance can not be achieved using safe code
• tests showing how it works safely across threads

Avoid platform-specific conditional compilation cfg

We hope that HoraeDB is usable across many different platforms and Operating systems, which means we put a high value on standard Rust.

While some performance critical code may require architecture specific instructions, (e.g. AVX512) most of the code should not.

Errors

All errors should follow the SNAFU crate philosophy and use SNAFU functionality

Good:

• Derives Snafu and Debug functionality
• Has a useful, end-user-friendly display message

#[derive(Snafu, Debug)]
pub enum Error {
    #[snafu(display(r#"Conversion needs at least one line of data"#))]
    NeedsAtLeastOneLine,
    // ...
}

Bad:

pub enum Error {
    NeedsAtLeastOneLine,
    // ...

Use the ensure! macro to check a condition and return an error

Good:

• Reads more like an assert!
• Is more concise

ensure!(!self.schema_sample.is_empty(), NeedsAtLeastOneLine);

Bad:

if self.schema_sample.is_empty() {
    return Err(Error::NeedsAtLeastOneLine {});
}

Errors should be defined in the module they are instantiated

Good:

• Groups related error conditions together most closely with the code that produces them
• Reduces the need to match on unrelated errors that would never happen

#[derive(Debug, Snafu)]
pub enum Error {
    #[snafu(display("Not implemented: {}", operation_name))]
    NotImplemented { operation_name: String }
}
// ...
ensure!(foo.is_implemented(), NotImplemented {
    operation_name: "foo",
}

Bad:

use crate::errors::NotImplemented;
// ...
ensure!(foo.is_implemented(), NotImplemented {
    operation_name: "foo",
}

The Result type alias should be defined in each module

Good:

• Reduces repetition

pub type Result<T, E = Error> = std::result::Result<T, E>;
...
fn foo() -> Result<bool> { true }

Bad:

...
fn foo() -> Result<bool, Error> { true }

Err variants should be returned with fail()

Good:

return NotImplemented {
    operation_name: "Parquet format conversion",
}.fail();

Bad:

return Err(Error::NotImplemented {
    operation_name: String::from("Parquet format conversion"),
});

Use context to wrap underlying errors into module specific errors

Good:

• Reduces boilerplate

input_reader
    .read_to_string(&mut buf)
    .context(UnableToReadInput {
        input_filename,
    })?;

Bad:

input_reader
    .read_to_string(&mut buf)
    .map_err(|e| Error::UnableToReadInput {
        name: String::from(input_filename),
        source: e,
    })?;

Hint for Box<dyn::std::error::Error> in Snafu:

If your error contains a trait object (e.g. Box<dyn std::error::Error + Send + Sync>), in order to use context() you need to wrap the error in a Box, we provide a box_err function to help do this conversion:

#[derive(Debug, Snafu)]
pub enum Error {

    #[snafu(display("gRPC planner got error listing partition keys: {}", source))]
    ListingPartitions {
        source: Box<dyn std::error::Error + Send + Sync>,
    },
}

...

use use common_util::error::BoxError;

  // Wrap error in a box prior to calling context()
database
  .partition_keys()
  .await
  .box_err()
  .context(ListingPartitions)?;

Each error cause in a module should have a distinct Error enum variant

Specific error types are preferred over a generic error with a message or kind field.

Good:

• Makes it easier to track down the offending code based on a specific failure
• Reduces the size of the error enum (String is 3x 64-bit vs no space)
• Makes it easier to remove vestigial errors
• Is more concise

#[derive(Debug, Snafu)]
pub enum Error {
    #[snafu(display("Error writing remaining lines {}", source))]
    UnableToWriteGoodLines { source: IngestError },

    #[snafu(display("Error while closing the table writer {}", source))]
    UnableToCloseTableWriter { source: IngestError },
}

// ...

write_lines.context(UnableToWriteGoodLines)?;
close_writer.context(UnableToCloseTableWriter))?;

Bad:

pub enum Error {
    #[snafu(display("Error {}: {}", message, source))]
    WritingError {
        source: IngestError,
        message: String,
    },
}

write_lines.context(WritingError {
    message: String::from("Error while writing remaining lines"),
})?;
close_writer.context(WritingError {
    message: String::from("Error while closing the table writer"),
})?;

Leaf error should contains backtrace

In order to make debugging easier, leaf errors in error chain should contains a backtrace.

// Error in module A
pub enum Error {
    #[snafu(display("This is a leaf error, source:{}.\nBacktrace:\n{}", source, backtrace))]
    LeafError {
        source: ErrorFromDependency,
        backtrace: Backtrace
    },
}

// Error in module B
pub enum Error {
    #[snafu(display("Another error, source:{}.\nBacktrace:\n{}", source, backtrace))]
    AnotherError {
        /// This error wraps another error that already has a
        /// backtrace. Instead of capturing our own, we forward the
        /// request for the backtrace to the inner error. This gives a
        /// more accurate backtrace.
        #[snafu(backtrace)]
        source: crate::A::Error,
    },
}

Tests

Don't return Result from test functions

At the time of this writing, if you return Result from test functions to use ? in the test function body and an Err value is returned, the test failure message is not particularly helpful. Therefore, prefer not having a return type for test functions and instead using expect or unwrap in test function bodies.

Good:

#[test]
fn google_cloud() {
    let config = Config::new();
    let integration = ObjectStore::new_google_cloud_storage(GoogleCloudStorage::new(
        config.service_account,
        config.bucket,
    ));

    put_get_delete_list(&integration).unwrap();
    list_with_delimiter(&integration).unwrap();
}

Bad:

type TestError = Box<dyn std::error::Error + Send + Sync + 'static>;
type Result<T, E = TestError> = std::result::Result<T, E>;

#[test]
fn google_cloud() -> Result<()> {
    let config = Config::new();
    let integration = ObjectStore::new_google_cloud_storage(GoogleCloudStorage::new(
        config.service_account,
        config.bucket,
    ));

    put_get_delete_list(&integration)?;
    list_with_delimiter(&integration)?;
    Ok(())
}

Thanks

Initial version of this doc is forked from influxdb_iox, thanks for their hard work.