make twemcache deal with partial request value #124

thinkingfish · 2017-01-24T20:09:19Z

This is the first diff to address issue #122

Subsequent diff will add more unit tests, as well as handle partial value when writing responses (when wbuf is full)

seppo0010

I feel as this change could use a different type of test, with a client sending messages instead of the unit tests we've been writing.

seppo0010 · 2017-01-26T16:43:48Z

test/storage/slab/check_slab.c

@@ -23,6 +23,7 @@ test_setup(void)
 {
    option_load_default((struct option *)&options, OPTION_CARDINALITY(options));
    slab_setup(&options, &metrics);
+    printf("foo\n");


uhm, probably not meant to be commited

seppo0010 · 2017-01-26T16:52:28Z

test/storage/slab/check_slab.c

+    ck_assert_int_eq(it->klen, sizeof(KEY) - 1);
+    ck_assert_int_eq(it->dataflag, dataflag);
+    ck_assert_int_eq(it->vlen, val.len);
+    ck_assert_msg(strspn(item_data(it), "A") == val.len, "item_data contains wrong value %.*s", it->vlen, item_data(it));


I don't think item_data(it) is null terminated

seppo0010 · 2017-01-26T16:57:18Z

test/storage/slab/check_slab.c

+    key = str2bstr(KEY);
+
+    vlen = 1000 * KiB;
+    val.len = vlen / 2;


I feel the test would be better if val.len was not exactly the half of vlen as it will make both sides to be the same length coincidentally, and an error may be missed because of it.

seppo0010 · 2017-01-26T17:03:28Z

test/storage/slab/check_slab.c

+    free(val.data);
+    ck_assert_msg(!it->is_linked, "item linked by mistake");
+    ck_assert_int_eq(it->vlen, vlen);
+    ck_assert_msg(strspn(item_data(it) + vlen - val.len, "B") == val.len,


add an assertion on the first part of the item_data to make sure it was not modified?

seppo0010 · 2017-01-26T17:14:54Z

test/storage/slab/check_slab.c

+}
+END_TEST
+
+START_TEST(test_evict_refcount)


isn't this test exactly the same as the one above?

seppo0010 · 2017-01-26T17:39:01Z

src/server/twemcache/data/process.c

-        item_delete(key);
-        status = item_insert(key, &(req->vstr), req->flag, time_reltime(req->expiry));
+    if ((it = item_get(key)) != NULL) {
+        status =_generic_set(req, true);


missing space after =

seppo0010 · 2017-01-26T17:39:06Z

src/server/twemcache/data/process.c

@@ -262,8 +288,7 @@ _process_cas(struct response *rsp, struct request *req)
        rsp->type = RSP_EXISTS;
        INCR(process_metrics, cas_exists);
    } else {
-        item_delete(key);
-        status = item_insert(key, &(req->vstr), req->flag, time_reltime(req->expiry));
+        status =_generic_set(req, true);


missing space after =

seppo0010 · 2017-01-26T17:40:06Z

src/server/twemcache/data/process.c

@@ -207,7 +233,7 @@ _process_add(struct response *rsp, struct request *req)
        rsp->type = RSP_NOT_STORED;
        INCR(process_metrics, add_notstored);
    } else {
-        status = item_insert(key, &(req->vstr), req->flag, time_reltime(req->expiry));
+        status =_generic_set(req, false);


missing space after =

seppo0010 · 2017-01-26T17:40:15Z

src/server/twemcache/data/process.c

-    item_delete(key);
-    status = item_insert(key, &(req->vstr), req->flag, time_reltime(req->expiry));
+
+    status =_generic_set(req, true);


missing space after =

seppo0010 · 2017-01-26T17:51:50Z

src/protocol/data/memcache/request.c

@@ -39,6 +39,10 @@ request_reset(struct request *req)
    req->delta = 0;
    req->vcas = 0;

+    req->partial = false; /* only set to true when partial value received */


having a rstate that may be REQ_PARTIAL this sees either redundant, or at least confusing... looking at the change I can see partial being set to false only when rstate going from REQ_PARTIAL to REQ_PARSED and set to true only when rstate is set to REQ_PARTIAL... isn't then this boolean redundant?

seppo0010 · 2017-02-08T11:29:50Z

We can have test dependencies that are not used for the binaries, in fact we should already have that for libcheck, and not require them for building the binaries. I believe it should be on the same repo, as they may need to get updated with code changes and that seems to be the common practice in other projects (e.g.: node projects have a devDependencies in package.json, rust projects have dev-dependencies in Cargo.toml and so on; also for example Redis repo has its tcl integration tests included).

Given this specific use case for partial requests, I don't believe any memcached client will expose an appropriated interface for the test we need, which is unfortunate, so I believe any scripting language will be good enough.

seppo0010 · 2017-02-08T23:00:43Z

src/protocol/data/memcache/parse.c

-                buf, buf_rsize(buf), vlen + CRLF_LEN);
-
-        return PARSE_EUNFIN;
+    if (rsize < nbyte + CRLF_LEN) {


since rsize and nbyte do not change before checking the value of complete, can you move this branch below, get rid of complete and use an else instead of if (complete) ?

I don't know if I made myself clear, something like this:

diff --git a/src/protocol/data/memcache/parse.c b/src/protocol/data/memcache/parse.c index cf3c06b..66cff18 100644 --- a/src/protocol/data/memcache/parse.c +++ b/src/protocol/data/memcache/parse.c @@ -277,21 +277,18 @@ _parse_val(struct bstring *val, struct buf *buf, uint32_t nbyte) { parse_rstatus_t status; uint32_t rsize = buf_rsize(buf); - bool complete = true; log_verb("parsing val (string) at %p", buf->rpos); - if (rsize < nbyte + CRLF_LEN) { - complete = false; - status = PARSE_EUNFIN; - } - val->len = MIN(nbyte, rsize); val->data = (val->len > 0) ? buf->rpos : NULL; buf->rpos += val->len; - /* verify CRLF */ - if (complete) { /* _try_crlf cannot return EUNFIN in this case */ + + if (rsize < nbyte + CRLF_LEN) { + status = PARSE_EUNFIN; + } else { + /* _try_crlf cannot return EUNFIN in this case */ status = _try_crlf(buf, buf->rpos); if (status == PARSE_OK) { buf->rpos += CRLF_LEN;

seppo0010 · 2017-02-08T23:12:52Z

src/protocol/data/memcache/parse.c

+                    buf->rpos - old_rpos);
+            /* return and start from beginning next time */
+            request_reset(req);
+            buf->rpos = old_rpos;


I'm not sure, but I think we're missing a break; here? otherwise it will enter the next if (as status is not PARSE_OK) and it will set the rstate to REQ_PARSED.

seppo0010 · 2017-02-09T00:03:59Z

src/server/twemcache/data/process.c

-    key = array_first(req->keys);
-    if (item_get(key) != NULL) {
+    it = (struct item *)req->reserved;
+    key = (struct bstring){it->klen, item_key(it)};


if item_reserve failed in _put, req->reserved will be null, and all this code will fail; check for status != PUT_ERROR earlier

seppo0010 · 2017-02-09T00:06:06Z

src/server/twemcache/data/process.c

-        item_delete(key);
-        status = item_insert(key, &(req->vstr), req->flag, time_reltime(req->expiry));
-        if (status == ITEM_OK) {
+    it = (struct item *)req->reserved;


same here ^^

seppo0010 · 2017-02-09T00:08:45Z

src/server/twemcache/data/process.c

-    key = array_first(req->keys);
-    it = item_get(key);
-    if (it == NULL) {
+    it = (struct item *)req->reserved;


seppo0010 · 2017-02-09T00:13:09Z

src/server/twemcache/data/process.c

-            item_delete(key);
-            status = item_insert(key, &nval, dataflag, it->expire_at);
+            status = item_reserve(&it, key, &nval, nval.len, dataflag,
+                    it->expire_at);


check for status before inserting?

seppo0010 · 2017-02-10T15:08:33Z

src/server/twemcache/data/process.c

        _error_rsp(rsp, istatus);
-        INCR(process_metrics, set_ex);
+        INCR(process_metrics, cas_ex);


seppo0010 · 2017-02-10T15:13:42Z

src/server/twemcache/data/process.c

@@ -242,23 +245,25 @@ _process_set(struct response *rsp, struct request *req)
    struct item *it;
    struct bstring key;

-    status = _put(&istatus, req);
+    status = _put(&istatus, req, true);


why are you deleting the existing item if set fails? If I understand this correctly, failing to set key to value2 will delete existing value1, is that so?

seppo0010

lgtm, a couple of minor comments

I'd love an integration test 😄 and it'd be nice if someone else can review it, as this change is quite large

seppo0010 · 2017-02-12T13:07:49Z

test/protocol/data/memcache/check_memcache.c

+    int ret;
+    struct bstring key = str2bstr(KEY);
+    struct bstring val1 = str2bstr(VAL1);
+    struct bstring val2 = str2bstr(VAL2);


I prefer if VAL1 and VAL2 were not the same length as it can hide some arithmetic bug like subtracting remaining number of bytes instead of received bytes

seppo0010 · 2017-02-12T13:10:06Z

test/protocol/data/memcache/check_memcache.c

+#undef EXPIRY
+#undef FLAG
+#undef VAL
+#undef KEY


EXPIRY, FLAG, VAL and KEY were not defined, remove undef

remove install instructions; get rid of cmake warning

4a3a0fd Merge pull request twitter#124 from thinkingfish/remove_install d2964bd Merge pull request twitter#122 from paegun/fix-ubuntu-test-linkage 481597c remove install instructions (will re-add later); get rid of cmake warning 74d9db8 reordered pthread dependency to follow check, the earliest dependent lib. d5c2fec fixed test linkage for ubuntu, tested on 14.04. added support for test-coverage.sh to use xdg-open rather than open when present, ie on ubuntu. git-subtree-dir: deps/ccommon git-subtree-split: 4a3a0fd

seppo0010 reviewed Jan 26, 2017

View reviewed changes

thinkingfish force-pushed the partial_value branch from 2d6869f to 8a05490 Compare February 7, 2017 21:49

This comment was marked as spam.

Sign in to view

thinkingfish self-assigned this Feb 8, 2017

seppo0010 reviewed Feb 9, 2017

View reviewed changes

thinkingfish force-pushed the partial_value branch from 8a05490 to 868d074 Compare February 10, 2017 03:39

seppo0010 reviewed Feb 10, 2017

View reviewed changes

seppo0010 reviewed Feb 12, 2017

View reviewed changes

This comment was marked as spam.

Sign in to view

thinkingfish force-pushed the partial_value branch from efca3df to 7261897 Compare February 23, 2017 07:47

Yao Yue added 12 commits February 24, 2017 20:27

add partial request test

4c0cbed

make twemcache deal with partial value in rbuf (more tests to come)

37c55aa

fixing various bugs

5433f32

adding unit test for partial parsing

5400a1f

more bug fixes, improved logging

52b6fb0

make rsp object part of the processing state

7478436

modify tests after feedback

25a3229

update comment

f9f977b

fixing multiple bugs

dd2c450

wrong metric incremented

4e20980

do not delete on server error

037705d

update memcache partial value test

37fe4e5

thinkingfish force-pushed the partial_value branch from 7261897 to 37fe4e5 Compare February 25, 2017 04:28

disable integration test in travis to avoid flaky results

e0a24e2

thinkingfish merged commit a06894f into twitter:master Feb 25, 2017

thinkingfish deleted the partial_value branch February 25, 2017 04:48

swlynch99 pushed a commit to swlynch99/pelikan-twitter that referenced this pull request Sep 30, 2019

Merge pull request twitter#124 from thinkingfish/remove_install

4a3a0fd

remove install instructions; get rid of cmake warning

make twemcache deal with partial request value #124

make twemcache deal with partial request value #124

Conversation

thinkingfish commented Jan 24, 2017

seppo0010 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as spam.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as spam.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as spam.

seppo0010 commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as spam.

Choose a reason for hiding this comment

This comment was marked as spam.

Choose a reason for hiding this comment

This comment was marked as spam.

Choose a reason for hiding this comment

This comment was marked as spam.

Choose a reason for hiding this comment

This comment was marked as spam.

Choose a reason for hiding this comment

This comment was marked as spam.

Choose a reason for hiding this comment

This comment was marked as spam.

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

seppo0010 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as spam.

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.