Proton Code Process for Engineers

Proton Code Process for Engineers

This document tries to establish a code process for proton in Timeplus. Proton developers shall apply this process as much as possible since it reduces (remote) collaboration / communication / rebase etc efforts a lot.

PR process

1. Every commit which is going to merge to develop must go through a PR process. The code committer shall include at least one senior engineer from the scrum team to review the code. If the code is complex, please ask Ken Chen to run through a final review.

2. Since Proton is based on ClickHouse code base, to avoid potential merge conflicts with the community code, it is highly recommended to create new C++ source file and header files for your new feature whenever it is possible. Sometimes, we still need to modify existing ClickHouse code, engineers shall include the comments proton: starts and proton: ends around your code changes. Furthermore, for new C++ includes, please create a separate include section and surround the new includes with proton: starts/ends. Following this convention will simplify the rebase work a lot and helps future code re-processing as well.

An example:

/// proton: starts
#include <...>
#include <...>
/// proton: ends

... existing code base ...
/// proton: starts
... <your new code>
/// proton: ends
... existing code base ...

3. For new C++ source / header files, please make sure to use clang-format to format your code before submitting a PR; otherwise the PR can be rejected immediately. Please note that timeplus-io/proton already has a .clang-format template. Please Google around how to use clang format in your IDE. For CLion users, after loading the project, you can use the Code → Reformat code / file menu to format the code. For VIM users, please Google around clang format VIM and integrate the shortcut in your .vimrc. (We will automate this in CI and check for every PR).

4. For code which updates the existing ClickHouse code base, please make sure your style still meets the code style mentioned in the next section. Please don't use clang-format for existing ClickHouse code since it may introduce too many changes for legacy ClickHouse sources/ header files and create lots of rebase burden.

5. Please make sure you have run utils/check-style/check-style before submitting a PR. (We will automate this in CI and check for every PR).

6. Please make sure your code meets the code style mentioned in the Code Style section as much as possible.

7. Engineers who submitted the PR shall continuously resolve the reviewer's comments (Mark them resolved) as the back and forth review session goes on.

8. In your merge commit message, please mention the github issue number. GitHub will correlate the commit with the issue automatically in this way.

An commit log example:

Fix miss processing of watermark block issue. This resolves #13

Header Include Order

Headers in the new C++ source files are divided into 4 different segments and are included in sequence like below for readability and also simpler for engineers to delete unused header files for compilation escalation. Each segment is separated by an empty line. Please use clang format to format the source code.

1. Local headers in the same directory. Using relative import path convention. For example, #include "CatalogService.h". This recommendation shall be revised : always stick to the absolute import path to avoid confusion.

2. Other dependent headers. Using absolute import path convention. For example, #include <DistributedMetadata/CatalogService.h>

3. 3rd party libraries. Using absolute import path convention. For example, #include <boost/noncopyable.h>

4. std libraries. Using absolute import path convention. For example, #include <memory>

An example

#include <DistributedMetadata/CatalogService.h>

#include <Columns/ColumnsNumber.h>
#include <Core/Block.h>
#include <DataStreams/AsynchronousBlockInputStream.h>
#include <DataStreams/BlockIO.h>
#include <IO/WriteBufferFromString.h>
#include <Interpreters/Context.h>

#include <Poco/Util/AbstractConfiguration.h>

#include <regex>

Code Style

Please read the ClickHouse code style . Here are some emphasis.

1. Avoid trailing underscores for the names of data members of a class. For example, storage instead of storage_

2. For function parameter, if the parameter name is the same as the name of a data member of a class, add trailing underscore to the parameter name.

For example

void setStorage(const StoragePtr & storage_) 
{
    storage = storage_;
}

3. Braces are always starting / ending in a new line unless it is one liner function or C++ lambda. (This will be enforced by CI)

Logging

For logging, the first letter in the first word of a log shall be capitalized and please use LOG_TRACE, LOG_DEBUG, LOG_INFO, LOG_ERROR etc judiciously, like avoiding logging trace in loops and for critical logs, at least use INFO level logging. Whenever an exception is caught, please try to log the stacktrace.

Here are some examples:

LOG_INFO(log, "This is a good log. {} starts", service_name);
LOG_INFO(log, "this is not a good log as the first letter `t` in the first word `this`");
 
LOG_INFO(log, std::format("This is a very bad log as the formatting is greedy. {}", msg));
try
{
    doCoolStuff();
}
catch (...) 
{
    LOG_ERROR(log, "Failed to do cool stuff, error={}", getCurrentExceptionMessage(true));
    throw; 
}

Exception Message

To throw an exception, the exception message follows the same convention : the first letter of the first word of a comment shall be capitalized and please try best to attach a concrete error code.

For examples:

throw Exception("Invalid parameters", ErrorCodes::BAD_ARGUMENTS); /// This is a good throw 

throw Exception("invalid parameters", ErrorCodes::UNKNOWN_EXCEPTION); /// This is a bad one

Comment

For comment, please use /// - three forward slashes instead of 2 and the first letter of the first word of a comment shall be capitalized.

For examples:

/// This is a valid comment.

// This is not recommended as it has two forward slashes and doesn't follow the existing conventions.

/// this one is not recommended as the first letter `t` of the first word `this` is not capitalized.

Class / Interface Design

1. Please pay attention to the order of function parameters. Making them consistent is important. For example, all reference to const parameters start at the first part of your function parameters, all reference to non-const parameters form the second part of your function parameters

2. When designing a class, most of the time, consider inheriting it from boost::noncopyable and most of the time you probably would like to make your class final

Build

Please at least use clang to build the code. It is highly recommended to use both clang and gcc to build the code to catch all possible issues.