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
Show file tree
Hide file tree
Changes from 7 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
Expand Up @@ -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
Expand Up @@ -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
Expand Down
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
Expand Up @@ -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'
Expand All @@ -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
Expand Down
3 changes: 2 additions & 1 deletion cmake/CMakeCargo.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion deps/ccommon/rust/ccommon_rs/src/bstring.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 _
Expand Down
4 changes: 2 additions & 2 deletions src/server/cdb/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ set(SOURCE
stats.c)

set(MODULES
cdb_ffi_static
cdb_rs_static
core
protocol_admin
protocol_memcache
Expand All @@ -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
Expand Up @@ -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>
Expand All @@ -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);

Expand Down Expand Up @@ -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);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

adding these comments here, because github doesn't support inline comments outside of the diff:

line 78: we can just use bstring_init to reset value_buf
like 104: nit: i think you can just say rsp->vstr = *vstr;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, good call!

I wasn't 100% sure about the semantics for doing that assignment on 104 so i just did the safe thing :)


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);
}

Expand All @@ -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;
Expand Down
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>
Expand Down Expand Up @@ -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);
Expand Down
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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down
17 changes: 9 additions & 8 deletions src/server/cdb/setting.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions src/storage/cdb/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@
*.cdb
perf.data*
*.cdb*
.cargo/
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