Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Omnibus CDB improvements #194

Merged
merged 10 commits into from
Jul 30, 2018
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -47,3 +47,4 @@ compile_commands.json

*.cmd
core
CMAKE_BINARY_DIR
13 changes: 12 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
@@ -46,7 +46,18 @@ matrix:
- gcc-4.9
- libsubunit-dev

# gcc 5 on linux
# gcc 5 on linux *without rust*
#
- env:
- C_COMPILER=gcc-5
addons:
apt:
<<: *apt
packages:
- gcc-5
- libsubunit-dev

# gcc 5 on linux *with* rust
- env:
- C_COMPILER=gcc-5
- RUST_ENABLED=1
4 changes: 3 additions & 1 deletion ci/before-install.sh
Original file line number Diff line number Diff line change
@@ -9,7 +9,9 @@ TEMP="$(mktemp -d -t TEMP.XXXXXXX)" || die "failed to make tmpdir"
cleanup() { [[ -n "${TEMP:-}" ]] && rm -rf "${TEMP}"; }
trap cleanup EXIT

TOPLEVEL="$(git -C "$(cd "$(dirname "$0")" >/dev/null || exit 1; pwd)" rev-parse --show-toplevel)" || die 'failed to find TOPLEVEL'
realpath() { python -c "import os,sys; print os.path.realpath(sys.argv[1])" "$1"; }

TOPLEVEL="$(cd "$(dirname "$(realpath "$0" >/dev/null || exit 1)")" && git rev-parse --show-toplevel)" || die 'failed to find TOPLEVEL'

# for osx: 0. update brew; 1. install cmake if missing; 2. (gcc) unlink pre-installed gcc; 3. (gcc) install desired version of gcc

4 changes: 3 additions & 1 deletion ci/install-check.sh
Original file line number Diff line number Diff line change
@@ -17,7 +17,9 @@ TEMP="$(mktemp -d -t TEMP.XXXXXXX)" || die "failed to make tmpdir"
cleanup() { [[ -n "${TEMP:-}" ]] && rm -rf "${TEMP}"; }
trap cleanup EXIT

TOPLEVEL="$(git -C "$(cd "$(dirname "$0")" >/dev/null || exit 1; pwd)" rev-parse --show-toplevel)" || die 'failed to find TOPLEVEL'
realpath() { python -c "import os,sys; print os.path.realpath(sys.argv[1])" "$1"; }

This comment was marked as spam.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ugh, ok you're right. i'll fix it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in #199


TOPLEVEL="$(cd "$(dirname "$(realpath "$0" >/dev/null || exit 1)")" && git rev-parse --show-toplevel)" || die 'failed to find TOPLEVEL'

CHECK_VERSION=0.12.0
CHECK_TARBALL="check-${CHECK_VERSION}.tar.gz"
20 changes: 20 additions & 0 deletions ci/local-clean-build-and-test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#!/bin/bash

set -euo pipefail
IFS=$'\n\t'

die() { echo "fatal: $*" >&2; exit 1; }

TEMP="$(mktemp -d -t TEMP.XXXXXXX)" || die "failed to make tmpdir"
cleanup() { [[ -n "${TEMP:-}" ]] && rm -rf "${TEMP}"; }
trap cleanup EXIT


if [[ -n "$(git status --porcelain)" ]]; then

This comment was marked as spam.

die "dirty working copy state, please commit changes before running this script"
fi

git archive --format=tar --prefix=pelikan/ HEAD .|tar -C "$TEMP" -xf-

This comment was marked as spam.

Copy link
Contributor Author

@slyphon slyphon Jul 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[affixing even larger prosthetic neckbeard over actual neckbeard]

Most shell I've seen does not put spaces around pipes, and I never do. It's not wrong to put spaces around the pipe (and I think some prefer that convention) but I always thought it's kinda amateurish.

cd "$TEMP/pelikan"

bash ./ci/run.sh
19 changes: 7 additions & 12 deletions ci/run.sh
Original file line number Diff line number Diff line change
@@ -15,11 +15,12 @@ export PATH=$HOME/.cargo/bin:$PATH

CMAKE_ARGS=(
-DBUILD_AND_INSTALL_CHECK=yes
-DTARGET_CDB=yes
-DHAVE_RUST=yes
-DRUST_VERBOSE_BUILD=1
)

if [[ -n "${RUST_ENABLED:-}" ]]; then
CMAKE_ARGS+=( -DTARGET_CDB=yes -DHAVE_RUST=yes -DRUST_VERBOSE_BUILD=1 )
fi

# TODO: run cmake3 on centos hosts

mkdir -p _build && ( cd _build && cmake ${CMAKE_ARGS[@]} .. && make -j && make check ) || die 'make failed'
@@ -30,18 +31,12 @@ egrep -r ":F:|:E:" . |grep -v 'Binary file' || true

set +e

( cd src/storage/cdb && env RUST_BACKTRACE=full cargo test )
if [[ -n "${RUST_ENABLED:-}" ]]; then
( cd src/storage/cdb && env RUST_BACKTRACE=full cargo test )
fi

RESULT=$?

if [[ "$(uname -s)" == "Darwin" ]]; then
if [[ $RESULT -ne 0 ]]; then
echo "Rust test failed on OSX, but this does not fail the build" >&2
fi

exit 0
fi

if [[ $RESULT -ne 0 ]]; then
echo "Build failure" >&2
exit $RESULT
3 changes: 2 additions & 1 deletion cmake/CMakeCargo.cmake
Original file line number Diff line number Diff line change
@@ -38,7 +38,8 @@ function(cargo_build)
cmake_parse_arguments(CARGO "" "NAME" "" ${ARGN})
string(REPLACE "-" "_" LIB_NAME ${CARGO_NAME})

get_filename_component(CARGO_TARGET_DIR "${CMAKE_CURRENT_BINARY_DIR}" ABSOLUTE)
get_filename_component(CARGO_TARGET_DIR "${CMAKE_BINARY_DIR}/target" ABSOLUTE)
message(STATUS "CARGO_TARGET_DIR ${CARGO_TARGET_DIR}")

cargo_set_lib_target(LIB_NAME)

5 changes: 4 additions & 1 deletion deps/ccommon/ci/before-install.sh
Original file line number Diff line number Diff line change
@@ -9,7 +9,10 @@ TEMP="$(mktemp -d -t TEMP.XXXXXXX)" || die "failed to make tmpdir"
cleanup() { [[ -n "${TEMP:-}" ]] && rm -rf "${TEMP}"; }
trap cleanup EXIT

TOPLEVEL="$(git -C "$(cd "$(dirname "$0")" >/dev/null || exit 1; pwd)" rev-parse --show-toplevel)" || die 'failed to find TOPLEVEL'
realpath() { python -c "import os,sys; print os.path.realpath(sys.argv[1])" "$1"; }

TOPLEVEL="$(cd "$(dirname "$(realpath "$0" >/dev/null || exit 1)")" && git rev-parse --show-toplevel)" || die 'failed to find TOPLEVEL'


# for osx: 0. update brew; 1. install cmake if missing; 2. (gcc) unlink pre-installed gcc; 3. (gcc) install desired version of gcc

5 changes: 4 additions & 1 deletion deps/ccommon/ci/install-check.sh
Original file line number Diff line number Diff line change
@@ -17,7 +17,10 @@ TEMP="$(mktemp -d -t TEMP.XXXXXXX)" || die "failed to make tmpdir"
cleanup() { [[ -n "${TEMP:-}" ]] && rm -rf "${TEMP}"; }
trap cleanup EXIT

TOPLEVEL="$(git -C "$(cd "$(dirname "$0")" >/dev/null || exit 1; pwd)" rev-parse --show-toplevel)" || die 'failed to find TOPLEVEL'
realpath() { python -c "import os,sys; print os.path.realpath(sys.argv[1])" "$1"; }

TOPLEVEL="$(cd "$(dirname "$(realpath "$0" >/dev/null || exit 1)")" && git rev-parse --show-toplevel)" || die 'failed to find TOPLEVEL'


CHECK_VERSION=0.12.0
CHECK_TARBALL="check-${CHECK_VERSION}.tar.gz"
2 changes: 1 addition & 1 deletion deps/ccommon/rust/ccommon_rs/src/bstring.rs
Original file line number Diff line number Diff line change
@@ -95,8 +95,8 @@ impl BStr {

// it's ok to ignore the cast_ptr_alignment lint because we're going
// *mut CCbstring -> &BStr -> *mut CCbstring
#[allow(cast_ptr_alignment)]
#[allow(unknown_lints)]
#[allow(cast_ptr_alignment)]
#[inline]
pub fn as_ptr(&self) -> *mut CCbstring {
(&*self) as *const _ as *mut _
4 changes: 2 additions & 2 deletions src/server/cdb/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -8,7 +8,7 @@ set(SOURCE
stats.c)

set(MODULES
cdb_ffi_static
cdb_rs_static
core
protocol_admin
protocol_memcache
@@ -25,7 +25,7 @@ set(TARGET_NAME ${PROJECT_NAME}_cdb)

add_executable(${TARGET_NAME} ${SOURCE})
target_link_libraries(${TARGET_NAME} ${MODULES} ${LIBS})
add_dependencies(${TARGET_NAME} cdb_ffi_static)
add_dependencies(${TARGET_NAME} cdb_rs_static)

install(TARGETS ${TARGET_NAME} RUNTIME DESTINATION bin)
add_dependencies(service ${TARGET_NAME})
16 changes: 6 additions & 10 deletions src/server/cdb/data/process.c
Original file line number Diff line number Diff line change
@@ -3,7 +3,7 @@
#include "process.h"

#include "protocol/data/memcache_include.h"
#include "storage/cdb/cdb.h"
#include "storage/cdb/cdb_rs.h"

#include <cc_array.h>
#include <cc_debug.h>
@@ -26,10 +26,10 @@ static struct bstring value_buf;
static bool process_init = false;
static process_metrics_st *process_metrics = NULL;

static struct CDBHandle *cdb_handle = NULL;
static struct cdb_handle *cdb_handle = NULL;

void
process_setup(process_options_st *options, process_metrics_st *metrics, struct CDBHandle *handle)
process_setup(process_options_st *options, process_metrics_st *metrics, struct cdb_handle *handle)
{
log_info("set up the %s module", CDB_PROCESS_MODULE_NAME);

@@ -68,15 +68,12 @@ process_teardown(void)
}

if (cdb_handle != NULL) {
struct CDBHandle *p = cdb_handle;
cdb_handle = NULL;
cdb_handle_destroy(p);
cdb_handle_destroy(&cdb_handle);
}

if (value_buf.data != NULL) {
char *p = value_buf.data;
value_buf.data = NULL;
value_buf.len = 0;
bstring_init(&value_buf);
cc_free(p);
}

@@ -101,8 +98,7 @@ _get_key(struct response *rsp, struct bstring *key)
rsp->key = *key;
rsp->flag = 0;
rsp->vcas = 0;
rsp->vstr.len = vstr->len;
rsp->vstr.data = vstr->data;
rsp->vstr = *vstr;

log_verb("found key at %p, location %p", key, vstr);
return true;
4 changes: 2 additions & 2 deletions src/server/cdb/data/process.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#pragma once

#include "storage/cdb/cdb.h"
#include "storage/cdb/cdb_rs.h"

#include <buffer/cc_buf.h>
#include <cc_metric.h>
@@ -33,7 +33,7 @@ typedef struct {
PROCESS_METRIC(METRIC_DECLARE)
} process_metrics_st;

void process_setup(process_options_st *options, process_metrics_st *metrics, struct CDBHandle *cdb_handle);
void process_setup(process_options_st *options, process_metrics_st *metrics, struct cdb_handle *cdb_handle);
void process_teardown(void);

int cdb_process_read(struct buf **rbuf, struct buf **wbuf, void **data);
19 changes: 13 additions & 6 deletions src/server/cdb/main.c
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#include "admin/process.h"
#include "setting.h"
#include "stats.h"
#include "storage/cdb/cdb.h"
#include "storage/cdb/cdb_rs.h"

#include "time/time.h"
#include "util/util.h"
@@ -80,17 +80,24 @@ teardown(void)
log_teardown();
}

static struct CDBHandle*
static struct cdb_handle *
setup_cdb_handle(cdb_options_st *opt)
{
cdb_setup();

char *cdb_file_path = opt->cdb_file_path.val.vstr;
if (cdb_file_path == NULL) {
struct cdb_handle_create_config cfg;
bstring_init(cfg.path);

cfg.load_method = (opt->use_mmap.val.vbool) ? CDB_MMAP : CDB_HEAP;

bstring_set_text(cfg.path, opt->cdb_file_path.val.vstr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you tested this? i think bstring_set_text only works for string literals

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think bstring_set_raw is for string literals. I've started up the server with this code and it answers queries.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, you are right. i misread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i had to try both :)

This comment was marked as spam.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some suggestions:

bstring_set_raw might be better named bstring_set_literal
bstring_set_text might be better named bstring_set_char_p or bstring_from_char_ptr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like bstring_set_literal. I think bstring_set_cstr is a clear name to me, not sure how you guys feel about it though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bstring_set_cstr is good. +1

Rust has CStr/CString to refer to a null-terminated string, so it follows that naming convention well.

This comment was marked as spam.


if (cfg.path == NULL) {
log_stderr("cdb_file_path option not set, cannot continue");
exit(EX_CONFIG);
}
return cdb_handle_create(cdb_file_path);

return cdb_handle_create(&cfg);
}

static void
@@ -140,7 +147,7 @@ setup(void)
compose_setup(NULL, &stats.compose_rsp);
klog_setup(&setting.klog, &stats.klog);

struct CDBHandle* cdb_handle = setup_cdb_handle(&setting.cdb);
struct cdb_handle *cdb_handle = setup_cdb_handle(&setting.cdb);
if (cdb_handle == NULL) {
log_stderr("failed to set up cdb");
goto error;
17 changes: 9 additions & 8 deletions src/server/cdb/setting.h
Original file line number Diff line number Diff line change
@@ -16,14 +16,15 @@
#include <channel/cc_tcp.h>
#include <stream/cc_sockio.h>

/* option related */
/* name type default description */
#define CDB_OPTION(ACTION) \
ACTION( daemonize, OPTION_TYPE_BOOL, false, "daemonize the process" )\
ACTION( pid_filename, OPTION_TYPE_STR, NULL, "file storing the pid" )\
ACTION( cdb_file_path, OPTION_TYPE_STR, "db.cdb", "location of the .cdb file" )\
ACTION( dlog_intvl, OPTION_TYPE_UINT, 500, "debug log flush interval(ms)" )\
ACTION( klog_intvl, OPTION_TYPE_UINT, 100, "cmd log flush interval(ms)" )
/* option related */
/* name type default description */
#define CDB_OPTION(ACTION) \
ACTION( daemonize, OPTION_TYPE_BOOL, false, "daemonize the process" )\
ACTION( pid_filename, OPTION_TYPE_STR, NULL, "file storing the pid" )\
ACTION( cdb_file_path, OPTION_TYPE_STR, "db.cdb", "location of the .cdb file" )\
ACTION( use_mmap, OPTION_TYPE_BOOL, false, "use mmap to load the file, false: use the heap" )\
ACTION( dlog_intvl, OPTION_TYPE_UINT, 500, "debug log flush interval(ms)" )\
ACTION( klog_intvl, OPTION_TYPE_UINT, 100, "cmd log flush interval(ms)" )

typedef struct {
CDB_OPTION(OPTION_DECLARE)
2 changes: 2 additions & 0 deletions src/storage/cdb/.gitignore
Original file line number Diff line number Diff line change
@@ -5,3 +5,5 @@
*.cdb
perf.data*
*.cdb*
.cargo/
dist/
5 changes: 4 additions & 1 deletion src/storage/cdb/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,2 +1,5 @@
add_subdirectory(cdb_ffi)
execute_process(COMMAND
env "CMAKE_BINARY_DIR=${CMAKE_BINARY_DIR}" bash scripts/build-cargo-config.sh
WORKING_DIRECTORY "$CMAKE_CURRENT_LIST_DIR}")
add_subdirectory(cdb_rs)
add_dependencies(ccommon_rs_static_target ccommon-static)
Loading