Merge branch 'main' into ntran/write_parquet_3

pull/24376/head
kodiakhq[bot] 2021-04-05 20:42:08 +00:00 committed by GitHub
commit 872be9097c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 88 additions and 152 deletions

View File

@ -1,108 +0,0 @@
# CI Overview
# -----------
#
# All pushes and PRs (including from forks) run:
#
# - cargo build with the default cargo profile ("dev")
# - cargo test
# - cargo fmt
# - clippy (with warnings denied)
#
# All workflow actions make use of our build container
# (`quay.io/influxdb/rust:ci`) to satisfy system dependencies, and is updated
# nightly.
#
# Cargo's build artefacts are cached to reduce the build time, see
# https://github.com/actions/cache for more info.
on: [pull_request]
name: ci
env:
# Disable full debug symbol generation to speed up CI build
# "1" means line tables only, which is useful for panic tracebacks.
RUSTFLAGS: "-C debuginfo=1"
jobs:
build:
name: Build
runs-on: ubuntu-latest
container:
image: quay.io/influxdb/rust:ci
# Run as the "root" user in the build container to fix workspace & cache
# permission errors.
options: --user root
steps:
# Checkout the code
- uses: actions/checkout@v2
# Enable caching of build artefacts
- uses: actions/cache@v2
with:
path: target
key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
# Build!
- name: Run dev build
uses: actions-rs/cargo@v1
with:
command: build
args: --workspace
# Ensure benches still build
- name: Build Benches
uses: actions-rs/cargo@v1
with:
command: test
args: --workspace --benches --no-run
test:
name: Test
runs-on: ubuntu-latest
container:
image: quay.io/influxdb/rust:ci
options: --user root
steps:
- uses: actions/checkout@v2
- uses: actions/cache@v2
with:
path: target
key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
- name: Run tests
uses: actions-rs/cargo@v1
with:
command: test
args: --workspace
lints:
name: Rust Lints
runs-on: ubuntu-latest
container:
image: quay.io/influxdb/rust:ci
options: --user root
steps:
- uses: actions/checkout@v2
- uses: actions/cache@v2
with:
path: target
key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
- name: Run cargo fmt
uses: actions-rs/cargo@v1
with:
command: fmt
args: --all -- --check
- uses: actions-rs/clippy-check@v1
with:
token: ${{ secrets.GITHUB_TOKEN }}
args: --all-targets --workspace -- -D warnings
protobuf:
name: Protobuf Lints
runs-on: ubuntu-latest
container:
image: bufbuild/buf
steps:
- uses: actions/checkout@v2
- name: Lint IOx protobuf
run: buf lint

View File

@ -1,33 +1,34 @@
# 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.
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 IOx will
accept patches with `unsafe` code.
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 IOx will accept patches
with `unsafe` code.
We may consider taking unsafe code given:
1. performance benchmarks showing a *very* compelling improvement
2. a compelling explanation of why the same performance can not be achieved using `safe` code
2. tests showing how it works safely across threads
1. a compelling explanation of why the same performance can not be achieved using `safe` code
1. tests showing how it works safely across threads
### Avoid platform specific conditional compilation `cfg`
### Avoid platform-specific conditional compilation `cfg`
We hope that IOx 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.
We hope that IOx 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
@ -48,36 +49,34 @@ pub enum Error {
```
*Bad*:
```rust
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
```rust
ensure!(!self.schema_sample.is_empty(), NeedsAtLeastOneLine);
```
*Bad*
*Bad*:
```rust
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
@ -95,7 +94,8 @@ ensure!(foo.is_implemented(), NotImplemented {
}
```
*Bad*
*Bad*:
```rust
use crate::errors::NotImplemented;
// ...
@ -109,37 +109,38 @@ ensure!(foo.is_implemented(), NotImplemented {
*Good*:
* Reduces repetition
```
```rust
pub type Result<T, E = Error> = std::result::Result<T, E>;
...
fn foo() -> Result<bool> { true }
```
*Bad*
```
*Bad*:
```rust
...
fn foo() -> Result<bool, Error> { true }
```
### `Err` variants should be returned with `fail()`
*Good*
*Good*:
```rust
return NotImplemented {
operation_name: "Parquet format conversion",
}.fail();
```
*Bad*
*Bad*:
```rust
return Err(Error::NotImplemented {
operation_name: String::from("Parquet format conversion"),
});
```
### Use `context` to wrap underlying errors into module specific errors
*Good*:
@ -154,7 +155,7 @@ input_reader
})?;
```
*Bad*
*Bad*:
```rust
input_reader
@ -167,7 +168,8 @@ input_reader
*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` :
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`:
```rust
#[derive(Debug, Snafu)]
@ -187,12 +189,11 @@ database
.await
.map_err(|e| Box::new(e) as _)
.context(ListingPartitions)?;
```
Note the `as _` in the `map_err` call. Without it, you may get an error such as:
```
```console
error[E0271]: type mismatch resolving `<ListingPartitions as IntoError<influxrpc::Error>>::Source == Box<<D as Database>::Error>`
--> query/src/frontend/influxrpc.rs:63:14
|
@ -205,8 +206,7 @@ error[E0271]: type mismatch resolving `<ListingPartitions as IntoError<influxrpc
= note: for more information, visit https://doc.rust-lang.org/book/ch19-03-advanced-traits.html
```
### Each error cause in a module should have a distinct Error enum
### 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.
@ -233,14 +233,13 @@ write_lines.context(UnableToWriteGoodLines)?;
close_writer.context(UnableToCloseTableWriter))?;
```
*Bad*
*Bad*:
```rust
pub enum Error {
#[snafu(display("Error {}: {}", message, source))]
WritingError {
source: IngestError ,
source: IngestError,
message: String,
},
}
@ -252,3 +251,48 @@ close_writer.context(WritingError {
message: String::from("Error while closing the table writer"),
})?;
```
## 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*:
```rust
#[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*:
```rust
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(())
}
```

View File

@ -1,15 +1,15 @@
/// Client for the gRPC health checking API
/// Client for health checking API
pub mod health;
/// Client for the management API
/// Client for management API
pub mod management;
/// Client for the write API
/// Client for write API
pub mod write;
/// Client for the operations API
/// Client for long running operations API
pub mod operations;
#[cfg(feature = "flight")]
/// Client for the flight API
/// Client for query API (based on Arrow flight)
pub mod flight;