GetMetaData() returns wrong status

[xumingbo]

Search before asking

• I had searched in the issues and found no similar issues.

Version

kvrocks 2.2.0.

Minimal reproduce step

Line 51 of storage/redis_db.cc
GetMetaData() returns "NotFound" when expired.

if (metadata->Expired()) {
metadata->Decode(old_metadata);
return rocksdb::Status::NotFound(kErrMsgKeyExpired);
}

What did you expect to see?

if (metadata->Expired()) {
metadata->Decode(old_metadata);
return rocksdb::Status::Expired(kErrMsgKeyExpired);
}

What did you see instead?

I tried to fix it by changing "NotFound" to "Expired", it caused:
cppunit/t_hash_test.cc:69: Failure, and coredump

Anything Else?

No response

Are you willing to submit a PR?

• I'm willing to submit a PR!

[git-hulk]

Hi @xumingbo

Could you provide the core dump stack or where's crash location is? And for the expiration metadata, I think it's good to regard it as the not found error since we don't need to know if it's an expired key or not found key

[mapleFU]

I wonder why it's neccessary to change it to rocksdb::Status::Expired, is it related to any handling logic outside?

[xumingbo]

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00007fc92c691ad3 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib/x86_64-linux-gnu/libpthread.so.0
[Current thread is 1 (Thread 0x7fc92aa7f700 (LWP 3235))]
(gdb) where
#0  0x00007fc92c691ad3 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib/x86_64-linux-gnu/libpthread.so.0
#1  0x00007fc92c3b292c in std::condition_variable::wait(std::unique_lock<std::mutex>&) ()
   from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#2  0x000055aae810df7d in rocksdb::ThreadPoolImpl::Impl::BGThread (this=this@entry=0x7fc92b441300, 
    thread_id=thread_id@entry=0) at util/threadpool_imp.cc:194
#3  0x000055aae810e2cd in rocksdb::ThreadPoolImpl::Impl::BGThreadWrapper (arg=0x7fc92b42f2c0)
    at util/threadpool_imp.cc:307
#4  0x00007fc92c3b874f in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#5  0x00007fc92c68b6db in start_thread () from /lib/x86_64-linux-gnu/libpthread.so.0
#6  0x00007fc92ba7593f in clone () from /lib/x86_64-linux-gnu/libc.so.6

[xumingbo]

/tests/cppunit/t_hash_test.cc:69: Failure
Value of: s.ok() && static_cast<int>(fvs.size()) == ret
  Actual: false
Expected: true
*** Aborted at 1688233350 (unix time) try "date -d @1688233350" if you are using GNU date ***
PC: @                0x0 (unknown)
*** SIGSEGV (@0x8) received by PID 3234 (TID 0x7fc92d2c4a40) from PID 8; stack trace: ***
    @     0x7fc92c696980 (unknown)
    @     0x55aae7be8c94 RedisHashTest_MGetAndMSet_Test::TestBody()
    @     0x55aae8497628 testing::internal::HandleExceptionsInMethodIfSupported<>()
    @     0x55aae84837af testing::Test::Run()
    @     0x55aae8483b3e testing::TestInfo::Run()
    @     0x55aae84841e0 testing::TestSuite::Run()
    @     0x55aae848fbe5 testing::internal::UnitTestImpl::RunAllTests()
    @     0x55aae84903e6 testing::UnitTest::Run()
    @     0x55aae7b3e412 main
    @     0x7fc92b975c07 __libc_start_main
    @     0x55aae7b8322a _start

[xumingbo]

We (Akamai) are implementing CRDT data type. It needs to handle physical NotFound and Expired (logical not found) differently. We can circumvent this bug, by overriding GetMetaData(). I think it is helpful to report it here.

[git-hulk]

@xumingbo For test case failure, you need to modify the corresponding status check as well. But for this case, how about checking if the key was expired by the error message?

We (Akamai) are implementing CRDT data type. It needs to handle physical NotFound and Expired (logical not found) differently.

Also contribute back the CRDT feature if you would like.

[PragmaTwice]

kvrocks/tests/cppunit/t_hash_test.cc

Lines 68 to 69 in 17b31e7

	rocksdb::Status s = hash->MSet(key_, fvs, false, &ret);
	EXPECT_TRUE(s.ok() && static_cast<int>(fvs.size()) == ret);

From a first glance, the abort in test case seems due to that the Hash::MSet function does not handle the Expired status well, since you changed the status from NotFound to Expired.

[git-hulk]

Close this PR due to lack of inactive, free feel to reopen if needed.