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

add command docs impl #1642

Merged
merged 5 commits into from
Jul 1, 2023
Merged

add command docs impl #1642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
33d2689
add command docs impl
tedli Jun 23, 2023
c0fa8f3
add command docs impl, add unit test
tedli Jun 23, 2023
5d248b3
Merge remote-tracking branch 'upstream/unstable' into cmddocs
tedli Jun 29, 2023
60c7d4a
Merge remote-tracking branch 'upstream/unstable' into cmddocs
tedli Jun 29, 2023
3196b3a
add pika specialization
tedli Jul 1, 2023
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
254 changes: 200 additions & 54 deletions include/pika_admin.h
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion include/pika_cmd_table_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@
class PikaCmdTableManager {
public:
PikaCmdTableManager();
virtual ~PikaCmdTableManager(){};
virtual ~PikaCmdTableManager() = default;
std::shared_ptr<Cmd> GetCmd(const std::string& opt);
uint32_t DistributeKey(const std::string& key, uint32_t slot_num);
bool CmdExist(const std::string& cmd) const;

private:
std::shared_ptr<Cmd> NewCommand(const std::string& opt);
Copy link

Choose a reason for hiding this comment

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

Overall, the code patch seems to introduce a few changes related to the PikaCmdTableManager class. Here's an overview of the changes:

  1. The destructor for PikaCmdTableManager has been modified from a virtual non-default destructor (virtual ~PikaCmdTableManager(){}) to a default virtual destructor (virtual ~PikaCmdTableManager() = default;). This change utilizes the default destructor implementation provided by the compiler.

  2. A new member function bool CmdExist(const std::string& cmd) const; has been added to the class. This function is used to check if a command exists.

  3. The existing member function std::shared_ptr<Cmd> NewCommand(const std::string& opt); remains without any apparent changes.

Based on this information, here are some observations and suggestions for improvement:

  1. Since the destructor was changed to a default virtual destructor, you should ensure that there are no explicit calls to delete objects of PikaCmdTableManager through a pointer to the base class. If you don't need polymorphic behavior or subclasses derived from PikaCmdTableManager, making the destructor non-virtual may be more appropriate.

  2. It's difficult to provide detailed feedback or identify potential issues without being able to see the full code context (e.g., definition and usage of Cmd class). Ensure that the class's implementation aligns with its intended functionality.

  3. Consider adding proper error handling and validation mechanisms to GetCmd and DistributeKey member functions to handle exceptional cases or invalid inputs. It's important to ensure that these functions behave predictably and are resilient to unexpected scenarios.

  4. Verify that the addition of CmdExist doesn't introduce unnecessary overhead or redundancy. Ensure that it provides value and is used appropriately in your codebase.

Remember that a thorough code review would require an understanding of the complete codebase, the surrounding context, and the requirements it serves. These suggestions are based solely on the information provided in the code patch you provided.

Expand Down
16 changes: 8 additions & 8 deletions include/pika_command.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@
#ifndef PIKA_COMMAND_H_
#define PIKA_COMMAND_H_

#include <memory>
#include <unordered_map>
#include <unordered_set>
#include <utility>
#include <memory>

#include "net/include/net_conn.h"
#include "net/include/redis_conn.h"
Expand Down Expand Up @@ -47,8 +48,9 @@ const std::string kCmdNamePKPatternMatchDel = "pkpatternmatchdel";
const std::string kCmdDummy = "dummy";
const std::string kCmdNameQuit = "quit";
const std::string kCmdNameHello = "hello";
const std::string kCmdNameCommand = "command";

//Migrate slot
// Migrate slot
const std::string kCmdNameSlotsMgrtSlot = "slotsmgrtslot";
Copy link

Choose a reason for hiding this comment

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

Based on the code patch you provided, here are a few observations and suggestions:

  1. Naming Convention: The variable names like kCmdNamePKPatternMatchDel and kCmdNameSlotsMgrtSlot seem quite long and may be difficult to read and maintain. Consider using more concise and descriptive names.

  2. Sorting Constants: It's good practice to sort constants alphabetically as it makes them easier to locate and maintain. Make sure to sort all the command constants, including the newly added kCmdNameCommand.

  3. Commenting: Consider adding comments to describe the purpose or functionality of certain parts of the code to improve code readability and maintainability.

  4. Error Handling: There doesn't appear to be any error handling or validation for the input commands. It would be beneficial to include error checking mechanisms if required, such as ensuring that the command is valid before processing it.

  5. Code Structure: It's difficult to fully evaluate the code without seeing the entire context. Consider providing more information or additional code if you have specific concerns or areas you'd like to improve.

Remember that this review is based solely on the provided code patch, and there might be other aspects of your software that need attention but aren't visible here.

Copy link

Choose a reason for hiding this comment

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

The code patch you provided appears to be an addition of a new constant string variable called kCmdNameCommand. Here are some observations and suggestions for improvement:

  1. Naming Convention: It's generally recommended to follow a consistent naming convention for variables. The existing variables are using a camelCase naming convention, so it would be better to name the new variable as kCmdNameCommand (as you have done) to maintain consistency.

  2. Comment: It would be good practice to add a comment explaining the purpose or usage of the new constant (kCmdNameCommand) if it's not already clear from the context. This can help other developers understand the code.

  3. Organization: Consider keeping the constants in alphabetical order to improve readability and maintain a consistent style. You can place kCmdNameCommand in the appropriate position based on its alphabetical order with respect to the other constants.

  4. Error Handling: Depending on how these constant strings are used, ensure that error handling is implemented appropriately when dealing with user input or command processing. This could involve checking for valid input, handling unexpected cases, and providing meaningful error messages.

  5. Additional Review: It's difficult to provide a comprehensive code review without the full context of the codebase. Other factors like coding conventions, adherence to design patterns, encapsulation, and error handling should also be considered during a thorough code review.

Remember, code reviews benefit from multiple perspectives, so it's always valuable to have your peers or experienced developers review your code as well.

const std::string kCmdNameSlotsMgrtTagSlot = "slotsmgrttagslot";
const std::string kCmdNameSlotsMgrtOne = "slotsmgrtone";
Expand Down Expand Up @@ -410,15 +412,13 @@ class Cmd : public std::enable_shared_from_this<Cmd> {
};
struct ProcessArg {
ProcessArg() = default;
ProcessArg(std::shared_ptr<Slot> _slot, std::shared_ptr<SyncMasterSlot> _sync_slot,
HintKeys _hint_keys)
ProcessArg(std::shared_ptr<Slot> _slot, std::shared_ptr<SyncMasterSlot> _sync_slot, HintKeys _hint_keys)
: slot(std::move(_slot)), sync_slot(std::move(_sync_slot)), hint_keys(std::move(_hint_keys)) {}
std::shared_ptr<Slot> slot;
std::shared_ptr<SyncMasterSlot> sync_slot;
HintKeys hint_keys;
};
Cmd(std::string name, int arity, uint16_t flag)
: name_(std::move(name)), arity_(arity), flag_(flag) {}
Cmd(std::string name, int arity, uint16_t flag) : name_(std::move(name)), arity_(arity), flag_(flag) {}
virtual ~Cmd() = default;

virtual std::vector<std::string> current_key() const;
Expand Down Expand Up @@ -463,6 +463,7 @@ class Cmd : public std::enable_shared_from_this<Cmd> {
void SetStage(CmdStage stage);

virtual void DoBinlog(const std::shared_ptr<SyncMasterSlot>& slot);

protected:
// enable copy, used default copy
// Cmd(const Cmd&);
Expand All @@ -478,7 +479,6 @@ class Cmd : public std::enable_shared_from_this<Cmd> {
int arity_ = -2;
uint16_t flag_ = 0;


protected:
CmdRes res_;
PikaCmdArgsType argv_;
Expand All @@ -496,7 +496,7 @@ class Cmd : public std::enable_shared_from_this<Cmd> {
Cmd& operator=(const Cmd&);
};

using CmdTable = std::unordered_map<std::string, std::unique_ptr<Cmd>>;
using CmdTable = std::unordered_map<std::string, std::unique_ptr<Cmd>>;

// Method for Cmd Table
void InitCmdTable(CmdTable* cmd_table);
Copy link

Choose a reason for hiding this comment

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

The provided code patch appears to be a section of code related to command handling in a project. Here are some suggestions for improvement:

  1. Header file inclusion:

    • Headers <memory>, <unordered_set>, and <utility> are included but not used in the code. You can remove them to reduce unnecessary dependencies.

  2. Formatting:

    • The comments in the code have inconsistent indentation. Ensure consistent formatting throughout the codebase.

  3. Variable naming:

    • The variable names could be more descriptive. For example, _slot and _sync_slot in the ProcessArg constructor could be renamed to better reflect their purpose.

  4. Class member initialization:

    • In the Cmd class, consider using initializer lists to initialize members in the constructor instead of assignments in the body of the constructor.

  5. Remove unused functions:

    • The function current_key() is declared but not defined in the code. Remove it if it's not being used.

  6. Consider refactoring duplicated code:

    • There are instances of repeated code, such as the assignment operator and the copy constructor of the Cmd class. Instead of marking them as private and not implementing them, you can use the default keyword to let the compiler generate them automatically.

  7. Documentation:

    • Add comments, especially for complex logic or non-obvious functionality, to improve code readability and maintainability.

These suggestions aim to enhance the code quality, but ultimately, the specific improvements depend on the context and requirements of your project.

Expand Down
Loading