chore: change log level to warning for empty keys #3722

kostasrim · 2024-09-17T11:36:29Z

change log level to warning

adiholden · 2024-09-17T11:58:13Z

src/server/rdb_load.cc

@@ -2478,8 +2478,8 @@ void RdbLoader::LoadItemsBuffer(DbIndex db_ind, const ItemsBuf& ib) {
    PrimeValue pv;
    if (ec_ = FromOpaque(item->val, &pv); ec_) {
      if ((*ec_).value() == errc::empty_key) {
-        LOG(ERROR) << "Found empty key: " << item->key << " in DB " << db_ind << " rdb_type "
-                   << item->val.rdb_type;
+        LOG(WARNING) << "Found empty key: " << item->key << " in DB " << db_ind << " rdb_type "


wait, what time of keys does it makes sence tho be empty? only hash and sets with expire for value , all the rest should remain error right?

This is code I added in https://github.com/dragonflydb/dragonfly/pull/3514/files#diff-f290b77c3b769ae2ef0e210094095a2c14f81f57980777e10ce4807cf3bc133cR2468

We can't really have an empty key if it's not a hash on a set as you say so it doesn't really make sense to add to LOG(ERROR) for datatypes that are not hash/set.

It's as simple change to LOG(ERROR) for any other data type. The question is, is there a value in doing so ?

I think that we might have a bug that we saw empty keys not for set and hash set and we want to track this
am I right @chakaz

it was a hash set I think @adiholden but let's wait for @chakaz

We definitely saw this line being printed for other types:

#define RDB_TYPE_HASH_ZIPLIST 13 #define RDB_TYPE_STRING 0 #define RDB_TYPE_SET 2

Then I will turn this into an ERROR for other types

adiholden · 2024-09-19T08:17:48Z

src/server/rdb_load.cc

@@ -228,6 +228,11 @@ string ModuleTypeName(uint64_t module_id) {
  return string{name};
 }

+bool RdbTypeAllowedEmpty(int type) {
+  return type == RDB_TYPE_STRING || type == RDB_TYPE_JSON || type == RDB_TYPE_SBF ||


do we allow empty for this types?
I think this should be only
constexpr uint8_t RDB_TYPE_HASH_WITH_EXPIRY = 31;
constexpr uint8_t RDB_TYPE_SET_WITH_EXPIRY = 32;
No?

nope, we do allow. For strings: SET foo "" --> empty and for SBF we have a test :)

See PrimeValue::TagAllowsEmptyValue

synced with Adi internally, we decided to also not log as errors hash sets and sets with expiry

kostasrim · 2024-09-19T09:08:23Z

src/core/compact_object.cc

@@ -852,7 +852,8 @@ bool CompactObj::HasAllocated() const {

 bool CompactObj::TagAllowsEmptyValue() const {
  const auto type = ObjType();
-  return type == OBJ_JSON || type == OBJ_STREAM || type == OBJ_STRING || type == OBJ_SBF;
+  return type == OBJ_JSON || type == OBJ_STREAM || type == OBJ_STRING || type == OBJ_SBF ||
+         type == OBJ_SET;


We also allow SETS now since they can be encoded as dense sets

chore: change log level to warning for empty keys

63b8764

kostasrim self-assigned this Sep 17, 2024

adiholden reviewed Sep 17, 2024

View reviewed changes

kostasrim requested a review from adiholden September 18, 2024 07:24

comments

450b1c8

kostasrim requested a review from chakaz September 19, 2024 07:53

small refactor

411b324

chakaz previously approved these changes Sep 19, 2024

View reviewed changes

adiholden reviewed Sep 19, 2024

View reviewed changes

skip sets

835c3e3

kostasrim dismissed chakaz’s stale review via 835c3e3 September 19, 2024 09:07

kostasrim commented Sep 19, 2024

View reviewed changes

kostasrim requested review from adiholden and chakaz September 19, 2024 09:09

adiholden approved these changes Sep 19, 2024

View reviewed changes

kostasrim enabled auto-merge (squash) September 19, 2024 09:31

kostasrim merged commit 0e0b2e7 into main Sep 19, 2024
12 checks passed

kostasrim deleted the kpr2 branch September 19, 2024 09:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: change log level to warning for empty keys #3722

chore: change log level to warning for empty keys #3722

kostasrim commented Sep 17, 2024

adiholden Sep 17, 2024

kostasrim Sep 17, 2024

adiholden Sep 18, 2024

kostasrim Sep 18, 2024

chakaz Sep 18, 2024

kostasrim Sep 18, 2024

adiholden Sep 19, 2024

kostasrim Sep 19, 2024

kostasrim Sep 19, 2024

kostasrim Sep 19, 2024

chore: change log level to warning for empty keys #3722

chore: change log level to warning for empty keys #3722

Conversation

kostasrim commented Sep 17, 2024

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

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