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.

Sign up for GitHub

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

Jump to bottom

feat: add new hash expire cmd to pika #2883

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from
Open

feat: add new hash expire cmd to pika #2883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ link_directories("/opt/rh/gcc-toolset-13/root/lib/gcc/x86_64-redhat-linux/13")
#set(CMAKE_BUILD_TYPE "Debug")
#set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -fsanitize=thread -O0 -fno-omit-frame-pointer -fno-optimize-sibling-calls")

set(CMAKE_BUILD_TYPE "Debug")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O0 -Wall -g3 -ggdb -fno-inline -fno-builtin-memcmp")
Comment on lines +38 to +39
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove hardcoded Debug build type and compiler flags.

The changes force Debug build type and add debug-specific compiler flags that disable optimizations. This could significantly impact performance in production environments and conflicts with the existing build type logic below (lines 46-55).

Consider one of these alternatives:

  1. Remove these lines and let the existing build type logic handle it:
-set(CMAKE_BUILD_TYPE "Debug")
-set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O0 -Wall -g3 -ggdb -fno-inline -fno-builtin-memcmp")
  1. Move these settings to a development-only configuration:
option(PIKA_DEVELOPMENT_MODE "Enable development mode with debug settings" OFF)
if(PIKA_DEVELOPMENT_MODE)
    set(CMAKE_BUILD_TYPE "Debug")
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O0 -Wall -g3 -ggdb -fno-inline -fno-builtin-memcmp")
endif()


string(TOLOWER ${CMAKE_HOST_SYSTEM_PROCESSOR} HOST_ARCH)

if(NOT CMAKE_BUILD_TYPE)
Expand Down
1 change: 1 addition & 0 deletions include/acl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ enum class AclCategory {
CONNECTION = (1ULL << 18),
TRANSACTION = (1ULL << 19),
SCRIPTING = (1ULL << 20),
PKHASH = (1ULL << 21),
};

enum class AclUserFlag {
Expand Down
31 changes: 26 additions & 5 deletions include/pika_command.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,27 @@ const std::string kCmdNameHScanx = "hscanx";
const std::string kCmdNamePKHScanRange = "pkhscanrange";
const std::string kCmdNamePKHRScanRange = "pkhrscanrange";

// PKHash
const std::string kCmdNamePKHSet = "pkhset";
const std::string kCmdNamePKHExpire = "pkhexpire";
const std::string kCmdNamePKHExpireat = "pkhexpireat";
const std::string kCmdNamePKHExpiretime = "pkhexpiretime";
const std::string kCmdNamePKHTTL = "pkhttl";
const std::string kCmdNamePKHPersist = "pkhpersist";
const std::string kCmdNamePKHGet = "pkhget";
const std::string kCmdNamePKHExists = "pkhexists";
const std::string kCmdNamePKHDel = "pkhdel";
const std::string kCmdNamePKHLen = "pkhlen";
const std::string kCmdNamePKHStrlen = "pkhstrlen";
const std::string kCmdNamePKHIncrby = "pkhincrby";
const std::string kCmdNamePKHMSet = "pkhmset";
const std::string kCmdNamePKHSetex = "pkhmsetex";
const std::string kCmdNamePKHMGet = "pkhmget";
const std::string kCmdNamePKHKeys = "pkhkeys";
const std::string kCmdNamePKHVals = "pkhvals";
const std::string kCmdNamePKHGetall = "pkhgetall";
const std::string kCmdNamePKHScan = "pkhscan";

// List
const std::string kCmdNameLIndex = "lindex";
const std::string kCmdNameLInsert = "linsert";
Expand Down Expand Up @@ -247,7 +268,6 @@ const std::string kCmdNameXInfo = "xinfo";

const std::string kClusterPrefix = "pkcluster";


/*
* If a type holds a key, a new data structure
* that uses the key will use this error
Expand Down Expand Up @@ -290,7 +310,8 @@ enum CmdFlags {
kCmdFlagsOperateKey = (1 << 19), // redis keySpace
kCmdFlagsStream = (1 << 20),
kCmdFlagsFast = (1 << 21),
kCmdFlagsSlow = (1 << 22)
kCmdFlagsSlow = (1 << 22),
kCmdFlagsPKHash = (1 << 23),
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Flag Inconsistencies Found: Missing kCmdFlagsPKHash

The following files are missing the kCmdFlagsPKHash flag where it should be applied:

  • tools/pika-port/pika_port_3/pika_command.h
  • include/pika_hyperloglog.h
  • src/pika_zset.cc
  • include/pika_set.h
  • include/pika_kv.h
  • include/pika_bit.h
  • src/pika_list.h
  • src/pika_cmd_table_manager.cc
  • src/acl.cc

Please ensure that kCmdFlagsPKHash is consistently added to all relevant command definitions to maintain proper functionality.

🔗 Analysis chain

LGTM: New PKHash flag added.

The kCmdFlagsPKHash flag has been added to the CmdFlags enum, which is consistent with the addition of new PKHash commands. This flag will allow for proper identification and handling of PKHash commands in the system.

To ensure the new flag is being utilized correctly, please run the following script to check for its usage across the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of the new kCmdFlagsPKHash flag

# Search for uses of the new flag
echo "Searching for uses of kCmdFlagsPKHash:"
rg "kCmdFlagsPKHash" --type cpp

# Search for potential places where the flag should be used
echo "Potential places where kCmdFlagsPKHash should be used:"
rg "kCmdFlags(Kv|Hash|List|Set|Zset|Bit|HyperLogLog|Geo|PubSub|Stream)" --type cpp

Length of output: 23556

Script:

#!/bin/bash
# Description: Verify that kCmdFlagsPKHash is used in all relevant command definitions

# List of potential flag definitions where kCmdFlagsPKHash should be applied
potential_flags=(
  "kCmdFlagsKv"
  "kCmdFlagsHash"
  "kCmdFlagsList"
  "kCmdFlagsSet"
  "kCmdFlagsZset"
  "kCmdFlagsBit"
  "kCmdFlagsHyperLogLog"
  "kCmdFlagsGeo"
  "kCmdFlagsPubSub"
  "kCmdFlagsStream"
)

echo "Verifying usage of kCmdFlagsPKHash in relevant command definitions:"

for flag in "${potential_flags[@]}"; do
  echo "Checking for commands using ${flag}:"
  rg "${flag}" --type cpp | while read -r line; do
    file=$(echo "$line" | cut -d':' -f1)
    if ! grep -q "kCmdFlagsPKHash" "$file"; then
      echo "Missing kCmdFlagsPKHash in $file"
    fi
  done
done

Length of output: 47799

};

void inline RedisAppendContent(std::string& str, const std::string& value);
Expand Down Expand Up @@ -483,7 +504,7 @@ class CmdRes {
struct UnblockTaskArgs {
std::string key;
std::shared_ptr<DB> db;
net::DispatchThread* dispatchThread{ nullptr };
net::DispatchThread* dispatchThread{nullptr};
UnblockTaskArgs(std::string key_, std::shared_ptr<DB> db_, net::DispatchThread* dispatchThread_)
: key(std::move(key_)), db(db_), dispatchThread(dispatchThread_) {}
};
Expand Down Expand Up @@ -572,7 +593,7 @@ class Cmd : public std::enable_shared_from_this<Cmd> {
std::shared_ptr<std::string> GetResp();

void SetStage(CmdStage stage);
void SetCmdId(uint32_t cmdId){cmdId_ = cmdId;}
void SetCmdId(uint32_t cmdId) { cmdId_ = cmdId; }

virtual void DoBinlog();

Expand Down Expand Up @@ -614,7 +635,7 @@ class Cmd : public std::enable_shared_from_this<Cmd> {

private:
virtual void DoInitial() = 0;
virtual void Clear(){};
virtual void Clear() {};

Cmd& operator=(const Cmd&);
};
Expand Down
Loading
Loading