Skip to content

Commit 43fe3ab

Browse files
doc: update contributing guide and readme
Move getting started and guidelines for Rust to the wiki and adopt standard ThinkParQ contributing guide.
1 parent ac0beb6 commit 43fe3ab

File tree

2 files changed

+69
-81
lines changed

2 files changed

+69
-81
lines changed

CONTRIBUTING.md

Lines changed: 68 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -1,80 +1,68 @@
1-
This document contains some high level guidelines for the BeeGFS Rust codebase. Low level style and good practice are enforced by [rustfmt](https://github.com/rust-lang/rustfmt) and [clippy](https://github.com/rust-lang/rust-clippy).
2-
3-
# Code style and best practice
4-
Coding style in general should follow the [Rust API Guidelines](https://rust-lang.github.io/api-guidelines/). These provide rules for and guidance on higher level design and implementation topics not covered by clippy. Following these helps to keep the codebase in a consistent and readable state.
5-
6-
While we don't provide an external library crate with a defined API, the codebase still contains lots of internal APIs: Every module, struct, function provides one which is used by programmers when interacting with them (e.g. calling a function). Following the guidelines helps to keep these internal interfaces clean and consistent, so they still make a lot of sense to follow.
7-
8-
Most of the items covered within the guidelines (see the [Checklist](https://rust-lang.github.io/api-guidelines/checklist.html)), we don't want to be overly strict for now. It's hard to follow them all at once and some also just might not apply or make sense for us. But they should still be taken into account when designing something.
9-
10-
For now, special attention should be payed to the first chapter: [Naming](https://rust-lang.github.io/api-guidelines/naming). Having consistent naming in the codebase increases the readability. The naming guidelines cover the general topics of casing, naming conversion functions, getters and more. In addition to that, the BeeGFS related naming is defined below.
11-
12-
## Specific naming conventions
13-
The following naming rules specify and complement the generic ones from [Naming](https://rust-lang.github.io/api-guidelines/naming):
14-
15-
### Word order
16-
If a name describes an action (meaning, it contains a verb), use the "natural" verb-object order. Example: `fn set_value()`, not `fn value_set()`. Exception: When items in the same namespace belong to categories, the category comes first. For example in the mgmtd command line options: `auth_enable` and `auth_file` belong together to the `auth` category, so `auth` comes first.
17-
18-
### Descriptive interface parameters
19-
Parameters being part of an interface (e.g. function arguments) should not just be named `id` but (a bit) more descriptive, e.g. `target_id`. This follows the convention that is used in the mgmtd database scheme and lets a reader immedately see which kind of ID that is.
20-
21-
### BeeGFS related naming
22-
BeeGFS related naming convention should be followed internally in the code as well as when communicating with others (log messages, documentation, ...). Referring to the same thing always with the same name makes everything easier to comprehend and looks professional to customers. Internally, omitting a word for a shorter variable name or function argument is allowed though if it is clear by the type what is expected.
23-
24-
* A buddy group is always called `buddy group`, not `buddy mirror group`, `mirror group` or `mirror buddy group` (as they are randomly in all of the old code)
25-
* A storage pools is called a `storage pool`. Since `storage_pool_id` is fairly long and used a lot, it can be (and usually is) abbreviated as `pool_id`.
26-
* A capacity pool is called `capacity pool` in free text, and, as part of names, `cap pool`.
27-
* `meta` is used for meta related stuff, not `metadata`, since it is shorter. Not sure what the "official" convention is (there probably isn't one), so it is up for discussion.
28-
29-
30-
31-
# Logging
32-
Logging should be used sparingly to avoid hard to read logs (at least at the higher levels, `INFO` and above). In general, one combined log message for a whole operation should be preferred over logging several times. For example, when multiple failures can occur in a loop without the function returning an error, do not log on each iteration but collect the failures and log once after the loop ends.
33-
34-
This also prevents mixing up related log messages with unrelated ones from other tasks/threads.
35-
36-
# Message handlers
37-
Incoming requests should be handled as single, atomic operations that can either complete successfully or fail as a whole. There are currently one or two exceptions where forwarding to others nodes is required (and for the moment we accept an potential inconsistent system), but for almost all cases, this is how it should be done. No partial success but all or nothing.
38-
39-
The following should be taken into account when writing message handlers:
40-
41-
## Only one `ERROR` log
42-
When the request fails, make exactly _one_ log entry containing the error chain leading to the failure. This should be done using the provided macro `log_error_chain!()`, which makes sure request errors look consistent.
43-
44-
## Only one `INFO` log on success if and only if system state changes
45-
If a request succeeds and the request changes the state of the system (e.g. writing to the database) - then, and only then, make an `INFO` level log entry telling the user what has been changed.
46-
47-
Info messages on readonly requests are superflous since the system state doesn't change and the requestor already knows that the request succeeded by retrieving the expected response. They would just clog the logfile.
48-
49-
# Database access
50-
The SQLite database is accessed using a single dedicated thread. This means, when accessing the database (e.g. by calling `db_op()`), evenrything in the provided function / closure is run on this thread only. To avoid clogging the application, no unnecessary expensive calculations or, worse, blocking operations should be done here.
51-
52-
## Transactions
53-
When accessing the database, the handle automatically starts a transaction which is commited after the provided closure / function has been processed. So, all executed operations in one call to `db_op` are automatically atomic. This ensures that read data using multiple statements is always consistent and also prevents partially successful operations.
54-
55-
Database interaction should therefore always be made within as few `db_op` calls as possible. This also reduces the overhead that comes from starting and commiting a transaction every time.
56-
57-
## Logging
58-
Since database transactions are "all or nothing", logging in the database thread should usually be avoided and instead happen outside, after the transaction succeeds or fails. If something goes wrong, an appropriate error should be returned instead, which can then be caught and logged by the requestor.
59-
60-
## User friendly errors
61-
While the database enforces its integrity itself using constraints and some triggers, errors that occur due to a constraint violation are technical and possibly hard to understand by a user. In particular, they do not tell the use in clear language what went wrong.
62-
63-
To improve that, queries / operations that rely on incoming data that needs to satisfy constraints should explicitly check this data for fullfilling these constraints and return a descriptive error in case it doesn't. For example, when a new unique alias is set but it actually exists already, we rather want to log an error like
64-
```
65-
Alias {} already exists
66-
```
67-
instead of
68-
```
69-
UNIQUE constraint failed: entities.alias: Error code 2067: A UNIQUE constraint failed
70-
```
71-
72-
# Error handling
73-
If an operation fails, the error should either be passed upwards by using `?` or handled by matching on the result. Panics are not caught, will abort the process and must therefore be avoided in normal operation. `panic!`, `.unwrap()`, `.expect()`, `assert!()` and other functions that fail by panicking must not be used.
74-
75-
There are some exceptions:
76-
* It can (and has to) be used in tests - if something unexpectedly returns error, the test has to fail anyway
77-
* If an error shouldn't happen during normal operation and can not easily be recovered from, panicking is allowed. This includes `assert!()` or `debug_assert!()` for checking invariants.
78-
* If a value demands for an `.unwrap()` of an `Option` and it is clear from the surrounding code that it cannot be `None`, unwrapping is also ok as a last resort. It is highly preferred though to restructure the code instead so that is not necessary anymore. In almost all cases it is possible (e.g. by using the inner value before putting it in the `Option` or use one of the countless helper functions`).
79-
80-
If `.unwrap()` should be used, consider using `.expect()` instead and provide additional information.
1+
# Contributing <!-- omit in toc -->
2+
Thank you for your interest in contributing to `beegfs-rs`! 🎉
3+
4+
We appreciate that you want to take the time to contribute. Please follow these steps before
5+
submitting your PR.
6+
7+
# Contents <!-- omit in toc -->
8+
9+
- [ThinkParQ Contributor License Agreement (CLA)](#thinkparq-contributor-license-agreement-cla)
10+
- [Creating a Pull Request](#creating-a-pull-request)
11+
- [Our Commitment](#our-commitment)
12+
13+
# ThinkParQ Contributor License Agreement (CLA)
14+
15+
Before contributions can be accepted we must have a signed CLA on file for all contributor(s):
16+
17+
* Download and fill out and sign the ThinkParQ CLA found at:
18+
https://www.beegfs.io/docs/ThinkParQ_CLA.pdf.
19+
* Email your signed copy to <[email protected]>.
20+
21+
22+
# Creating a Pull Request
23+
24+
1. Please search [existing issues](https://github.com/ThinkParQ/beegfs-rs/issues) to determine if an
25+
issue already exists for what you intend to contribute.
26+
2. If the issue does not exist, [create a new
27+
one](https://github.com/ThinkParQ/beegfs-rs/issues/new) that explains the bug or feature request.
28+
* Let us know in the issue that you plan on creating a pull request for it. This helps us to keep
29+
track of the pull request and avoid any duplicate efforts.
30+
3. Before creating a pull request, write up a brief proposal in the issue describing what your
31+
change would be and how it would work so that others can comment.
32+
* It's better to wait for feedback from the maintainers before writing code. We don't have an
33+
SLA for our feedback, but we will do our best to respond in a timely manner (at a minimum, to
34+
give you an idea if you're on the right track and that you should proceed, or not).
35+
4. While making your changes ensure they comply with the project's [coding
36+
standards](https://github.com/ThinkParQ/beegfs-rs/wiki/Getting-Started-with-Rust#coding-standards).
37+
5. When ready refer to the guidelines and process for submitting a [Pull
38+
Request](https://github.com/ThinkParQ/beegfs-rs/wiki/Pull-Requests).
39+
40+
41+
# Our Commitment
42+
While we truly appreciate your efforts on pull requests and will accept contributions as often as we
43+
can, we **cannot** commit to accepting all PRs. Here are a few reasons why a PR may be rejected:
44+
45+
* There are many factors involved in integrating new code into this project including:
46+
* Adding appropriate unit and end-to-end test coverage for new/changed functionality.
47+
* Ensuring adherence with ThinkParQ and industry standards around security and licensing.
48+
* Validating new functionality doesn't raise long-term maintainability and/or supportability
49+
concerns.
50+
* Verifying changes fit with the current and/or planned architecture.
51+
* etc.
52+
53+
In other words, while your bug fix or feature may be perfect as a standalone patch, we have to
54+
ensure the changes also work in all use cases, supported, configurations, and across our support
55+
matrix.
56+
57+
* The BeeGFS development team must plan resources to integrate your code into our code base and CI
58+
platform, and depending on the complexity of your PR, we may or may not have the resources
59+
available to make it happen in a timely fashion. We'll do our best, but typically the earliest
60+
changes can be merged into the master branch is with our next formal release, unless they resolve
61+
a critical bug or security vulnerability.
62+
63+
* Sometimes a PR doesn't fit into our future plans or conflicts with other items on the roadmap.
64+
It's possible that a PR you submit doesn't align with our upcoming plans, thus we won't be able to
65+
use it. It's not personal and why we highly recommend submitting an issue with your proposed
66+
changes so we can provide feedback before you expend significant effort on development.
67+
68+
Thank you for considering to contribute to `beegfs-rs`!

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# Build the management
22

3-
1. Read through [Getting started with Rust]( https://github.com/ThinkParQ/developer-handbook/tree/main/getting_started/rust) and setup your environment as described.
3+
1. Read through [Getting started with Rust](https://github.com/ThinkParQ/beegfs-rs/wiki/Getting-Started-with-Rust) and setup your environment as described.
44

55
2. Clone the management repository:
66

0 commit comments

Comments
 (0)