You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by an...@apache.org on 2018/01/17 00:51:05 UTC
[1/2] mesos git commit: Updated C++ Style Guide.
Repository: mesos
Updated Branches:
refs/heads/master e34ed9602 -> 9b1fda56a
Updated C++ Style Guide.
Added three missing style notes:
* Use of `::` for global namespace
* Put `template <typename T>` on own line
* Prefer `->foo()` to `.get().foo()`
Review: https://reviews.apache.org/r/65114
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/9b1fda56
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/9b1fda56
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/9b1fda56
Branch: refs/heads/master
Commit: 9b1fda56a04da9dafadd117a21cdf92d24e50483
Parents: 55b66c6
Author: Andrew Schwartzmeyer <an...@schwartzmeyer.com>
Authored: Thu Jan 11 18:02:04 2018 -0800
Committer: Andrew Schwartzmeyer <an...@schwartzmeyer.com>
Committed: Tue Jan 16 16:50:11 2018 -0800
----------------------------------------------------------------------
docs/c++-style-guide.md | 13 +++++++++++++
1 file changed, 13 insertions(+)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/9b1fda56/docs/c++-style-guide.md
----------------------------------------------------------------------
diff --git a/docs/c++-style-guide.md b/docs/c++-style-guide.md
index ef80c86..80a1af3 100644
--- a/docs/c++-style-guide.md
+++ b/docs/c++-style-guide.md
@@ -12,6 +12,7 @@ The Mesos codebase follows the [Google C++ Style Guide](https://google.github.io
### Namespaces
* We avoid `using namespace foo` statements as it is not explicit about which symbols are pulled in, and it can often pull in a lot of symbols, which sometimes lead to conflicts.
* It is OK to use namespace aliases to help pull in sub-namespaces, such as `namespace http = process::http;`. These should only be present at the top of the .cpp file.
+* We prefer to prefix the global namespace with `::`, such as `::syscall`. This makes it clear that we're calling a C API, not a function in the local namespace.
## Naming
@@ -83,9 +84,21 @@ driver.acceptOffers({offer.id()},
### Templates
* Leave one space after the `template` keyword, e.g. `template <typename T>` rather than `template<typename T>`.
+* Put `template <typename T>` on its own line, with the templated code (such as a function) starting on the next line.
### Function Definition/Invocation
* Newline when calling or defining a function: indent with four spaces.
+
+* When using types like `Try` or `Owned`, use `operator->` instead of chained calls to `.get()` when possible:
+
+~~~{.cpp}
+// Preferred.
+Owned<MasterDetector> detector = master.get()->createDetector();
+
+// Don't use.
+Owned<MasterDetector> detector = master.get().get().createDetector();
+~~~
+
* We do not follow Google's style of wrapping on the open parenthesis, the general goal is to reduce visual "jaggedness" in the code. Prefer (1), (4), (5), sometimes (3), never (2):
~~~{.cpp}
[2/2] mesos git commit: Added Developer Guide to documentation.
Posted by an...@apache.org.
Added Developer Guide to documentation.
This guide was started with the intent to capture "tribal knowledge"
about Mesos development. I've added several general practices and
patterns I've had explained to me by various Mesos developers, and also
added a Windows section with design patterns I had documented
externally.
Review: https://reviews.apache.org/r/65113
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/55b66c62
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/55b66c62
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/55b66c62
Branch: refs/heads/master
Commit: 55b66c624f5a2e44fcf87bdcab8932a2c7a2c284
Parents: e34ed96
Author: Andrew Schwartzmeyer <an...@schwartzmeyer.com>
Authored: Thu Jan 11 17:42:45 2018 -0800
Committer: Andrew Schwartzmeyer <an...@schwartzmeyer.com>
Committed: Tue Jan 16 16:50:11 2018 -0800
----------------------------------------------------------------------
docs/advanced-contribution.md | 2 +
docs/developer-guide.md | 200 +++++++++++++++++++++++++++++++++++++
docs/home.md | 1 +
3 files changed, 203 insertions(+)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/55b66c62/docs/advanced-contribution.md
----------------------------------------------------------------------
diff --git a/docs/advanced-contribution.md b/docs/advanced-contribution.md
index 6af24cc..f951491 100644
--- a/docs/advanced-contribution.md
+++ b/docs/advanced-contribution.md
@@ -115,6 +115,8 @@ As you gain experience contributing to Mesos you may want to tackle more advance
For patches to the core, we ask that you follow the [Mesos C++ Style Guide](c++-style-guide.md).
+The [Mesos Developer Guide](developer-guide.md) contains some best practices and design patterns that are useful for new developers to learn.
+
## Additional Guidance
The following links provide additional guidance as you get started contributing to Apache Mesos.
http://git-wip-us.apache.org/repos/asf/mesos/blob/55b66c62/docs/developer-guide.md
----------------------------------------------------------------------
diff --git a/docs/developer-guide.md b/docs/developer-guide.md
new file mode 100644
index 0000000..3dbc93e
--- /dev/null
+++ b/docs/developer-guide.md
@@ -0,0 +1,200 @@
+---
+title: Apache Mesos - Developer Guide
+layout: documentation
+---
+
+# Developer Guide
+
+This document is distinct from the [C++ Style Guide](c++-style-guide.md) as it
+covers best practices, design patterns, and other tribal knowledge, not just how
+to format code correctly.
+
+# General
+
+## When to Introduce Abstractions
+
+Don't introduce an abstraction just for code de-duplication. Always think about
+if the abstraction makes sense.
+
+## Include What You Use
+
+IWYU: the principle that if you use a type or symbol from a header file, that
+header file should be included.
+
+While IWYU should always be followed in C++, we have a problem specifically with
+the `os` namespace. Originally, all functions like `os::realpath` were
+implemented in `stout/os.hpp`. At some point, however, each of these were moved
+to their own file (i.e. `stout/os/realpath.hpp`). Unfortunately, it is very easy
+to use an `os` function without including its respective header because
+`stout/posix/os.hpp` includes almost all of these headers. This tends to break
+other platforms, as `stout/windows/os.hpp` _does not_ include all of these
+headers. The guideline is to Include What You Use, especially for
+`stout/os/*.hpp`.
+
+## Error message reporting
+
+The general pattern is to just include the reason for an error, and to not
+include any information the caller already has, because otherwise the callers
+will double log:
+
+```
+namespace os {
+
+Try<Nothing> copyfile(string source, string destination)
+{
+ if (... copying failed ...) {
+ return Error("Failed to copy '" + source + "' to '" + destination + "'");
+ }
+
+ return Nothing();
+}
+
+} // namespace os
+
+Try<Nothing> copy = os::copyfile(source, destination);
+
+if (copy.isError()) {
+ return ("Failed to copy '" + source + "'"
+ " to '" + destination + "': " + copy.error();
+}
+```
+
+This would emit:
+
+> Failed to copy 's' to 'd': Failed to copy 's' to 'd': No disk space left
+
+A good way to think of this is: "what is the 'actual' error message?"
+
+An error message consists of several parts, much like an exception: the "reason"
+for the error, and multiple "stacks" of context. If you're referring to the
+"reason" when you said "actual", both approaches (the one Mesos uses, or the
+above example) include the reason in their returned error message. The
+distinction lies in _where_ the "stacks" of context get included.
+
+The decision Mesos took some time ago was to have the "owner" of the context be
+responsible for including it. So if we call `os::copyfile` we know which
+function we're calling and which `source` and `destination` we're passing into
+it. This matches POSIX-style programming, which is likely why this approach was
+chosen.
+
+The POSIX-style code:
+
+```
+int main()
+{
+ int fd = open("/file");
+
+ if (fd == -1) {
+ // Caller logs the thing it was doing, and gets the reason for the error:
+ LOG(ERROR) << "Failed to initialize: Failed to open '/file': " << strerror(errno);
+ }
+}
+```
+
+is similar to the following Mesos-style code:
+
+```
+int main()
+{
+ Try<int> fd = open("/file");
+
+ if (fd.isError()) {
+ // Caller logs the thing it was doing, and gets the reason for the error:
+ LOG(ERROR) << "Failed to initialize: Failed to open '/file': " << fd.error();
+ }
+}
+```
+
+If we use the alternative approach to have the leaf include all the information
+it has, then we have to compose differently:
+
+```
+int main()
+{
+ Try<int> fd = os::open("/file");
+
+ if (fd.isError()) {
+ // Caller knows that no additional context needs to be added because callee has all of it.
+ LOG(ERROR) << "Failed to initialize: " << fd.error();
+ }
+}
+```
+
+The approach we chose was to treat the error as just the "reason" (much like
+`strerror`), so if the caller wants to add context to it, they can. Both
+approaches work, but we have to pick one and apply it consistently as best we
+can. So don't add information to an error message that the caller already has.
+
+# Windows
+
+## Unicode
+
+Mesos is explicitly compiled with `UNICODE` and `_UNICODE` preprocess
+defintions, forcing the use of the wide `wchar_t` versions of ambiguous APIs.
+Nonetheless, developers should be explicit when using an API: use
+`::SetCurrentDirectoryW` over the ambiguous macro `::SetCurrentyDirectory`.
+
+When converting from `std::string` to `std::wstring`, do not reinvent the wheel!
+Use the `wide_stringify()` and `stringify()` functions from
+[`stringify.hpp`](https://github.com/apache/mesos/blob/master/3rdparty/stout/include/stout/stringify.hpp).
+
+## Long Path Support
+
+Mesos has built-in NTFS long path support. On Windows, the usual maximum path is
+about 255 characters (it varies per API). This is unusable because Mesos uses
+directories with GUIDs, and easily exceeds this limitation. To support this, we
+use the Unicode versions of the Windows APIs, and explicitly preprend the long
+path marker `\\?\` to any path sent to these APIs.
+
+The pattern, when using a Windows API which takes a path, is to:
+
+1. Use the wide version of the API (suffixed with `W`).
+2. Ensure the API supports long paths (check MSDN for the API).
+3. Use `::internal::windows::longpath(std::string path)` to safely convert the path.
+4. Only use the `longpath` for Windows APIs, or internal Windows API wrappers.
+
+For an example, see
+[`chdir.hpp`](https://github.com/apache/mesos/blob/master/3rdparty/stout/include/stout/os/windows/chdir.hpp).
+
+The long path helper is found in
+[`longpath.hpp`](https://github.com/apache/mesos/blob/master/3rdparty/stout/include/stout/internal/windows/longpath.hpp).
+
+### Windows CRT
+
+While it is tempting to use the Windows CRT to ease porting, we explicitly avoid
+using it as much as possible for several reasons:
+
+* It does not interact well with Windows APIs. For instance, an environment
+ variable set by the Win32 API `SetEnvironmentVariable` will not be visible in
+ the CRT API `environ`.
+
+* The CRT APIs tend to be difficult to encapsulate properly with RAII.
+
+* Parts of the CRT have been deprecated, and even more are marked unsafe.
+
+It is almost always preferable to use Win32 APIs, which is akin to "Windows
+system programming" rather than porting Mesos onto a POSIX compatibility layer.
+It may not always be possible to avoid the CRT, but consider the implementation
+carefully before using it.
+
+## Handles
+
+The Windows API is flawed and has multiple invalid semantic values for the
+`HANDLE` type, i.e. some APIs return `-1` or `INVALID_HANDLE_VALUE`, and other
+APIs return `nullptr`. It is simply
+[inconsistent](https://blogs.msdn.microsoft.com/oldnewthing/20040302-00/?p=40443),
+so developers must take extra caution when checking handles returned from the
+Windows APIs. Please double check the documentation to determine which value
+will indicate it is invalid.
+
+Using raw handles (or indeed raw pointers anywhere) in C++ is treachorous. Mesos
+has a `SafeHandle` class which should be used immediately when obtaining a
+`HANDLE` from a Windows API, with the deleter likely set to `::CloseHandle`.
+
+## Nano Server Compatibility
+
+We would like to target Microsoft Nano Server. This means we are restricted to
+the set of Windows APIs available on Nano,
+[Nano Server APIs](https://msdn.microsoft.com/en-us/library/mt588480(v=vs.85).aspx).
+An example of an *excluded and unavailable* set of APIs is `Shell32.dll` AKA
+`<shlobj.h>`.
http://git-wip-us.apache.org/repos/asf/mesos/blob/55b66c62/docs/home.md
----------------------------------------------------------------------
diff --git a/docs/home.md b/docs/home.md
index 4c1fb37..f5b65cc 100644
--- a/docs/home.md
+++ b/docs/home.md
@@ -100,6 +100,7 @@ layout: documentation
* [Engineering Principles and Practices](engineering-principles-and-practices.md) to serve as a shared set of project-level values for the community.
* Style Guides:
* [Documentation Style Guide](documentation-guide.md)
+ * [Developer Guide](developer-guide.md) for best practices and patterns used in Mesos.
* [C++ Style Guide](c++-style-guide.md)
* [Clang-Format](clang-format.md) for automatic formatting.
* [Doxygen Style Guide](doxygen-style-guide.md)