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.

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

add slot nums to config (#145) #1664

Merged
merged 3 commits into from
Jul 3, 2023
Merged

add slot nums to config (#145) #1664

merged 3 commits into from
Jul 3, 2023

Conversation

luky116
Copy link
Collaborator

@luky116 luky116 commented Jun 30, 2023

No description provided.

# Set backend tcp keepalive period. (0 to disable)
backend_keepalive_period = "75s"

# Set number of databases of backend.
backend_number_databases = 16
backend_number_databases = 1

# If there is no request from client for a long time, the connection will be closed. (0 to disable)
# Set session recv buffer size & timeout.
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 some observations and suggestions for improvement:

  1. The addition of max_slot_num = 1024 sets the maximum number of slots to 1024. Ensure that this value is appropriate for your specific use case.
  2. In the line backend_number_databases = 1, it seems like you reduced the number of databases from 16 to 1. Make sure this change aligns with your application's requirements. Note that reducing the number of databases could impact the functionality or performance of your system.
  3. The comment mentioning "session recv buffer size & timeout" is incomplete. It would be helpful to include the actual configuration settings related to session buffer size and timeout, if applicable, or remove the comment if not needed.

Overall, without a complete understanding of your system's requirements and the context in which these variables are being used, it is difficult to provide a comprehensive code review. It's essential to thoroughly test these changes in your specific environment to ensure they work as intended and do not introduce any unexpected bugs or issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

请忽略

@@ -333,7 +333,7 @@ func (t *cmdAdmin) loadJsonConfigV3(file string) *ConfigV3 {

var slots = make(map[int]*models.SlotMapping)
for _, s := range config.Slots {
if s.Id < 0 || s.Id >= models.MaxSlotNum {
if s.Id < 0 || s.Id >= models.GetMaxSlotNum() {
log.Panicf("invalid slot id = %d", s.Id)
}
if slots[s.Id] != nil {
Copy link

Choose a reason for hiding this comment

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

In the code patch, there are two changes made:

  1. In the handleConfigConvert function:

    • Replaced models.MaxSlotNum with models.GetMaxSlotNum() in the loop condition.
    • Reason: It seems that GetMaxSlotNum() is a function that returns the maximum slot number. Using a function instead of a constant may allow for more flexibility and dynamic behavior.
  2. In the loadJsonConfigV3 function:

    • Replaced models.MaxSlotNum with models.GetMaxSlotNum() in the if condition.
    • Reason: Similar to the previous change, using GetMaxSlotNum() allows for dynamic handling of the maximum slot number.

Overall, these changes seem reasonable and might improve the flexibility of the code by using a function to retrieve the maximum slot number instead of relying on a constant. However, without further context about the models package or the purpose of the code, it's challenging to provide more specific feedback or identify any potential bugs or risks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

请忽略

@@ -145,7 +145,7 @@ func (t *cmdProxy) handleFillSlots(d map[string]interface{}) {
}

for _, m := range slots {
if m.Id < 0 || m.Id >= models.MaxSlotNum {
if m.Id < 0 || m.Id >= models.GetMaxSlotNum() {
log.Panicf("invalid slot id = %d", m.Id)
}
}
Copy link

Choose a reason for hiding this comment

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

In the code patch you provided, there is a change in the condition of the if statement in the handleFillSlots function. Instead of using a hardcoded value (models.MaxSlotNum), it now uses a function call models.GetMaxSlotNum().

Code Review:

  • The code change appears to be related to checking the validity of slot IDs.
  • Previously, the check was performed against a constant value (MaxSlotNum), but now it uses a function call (GetMaxSlotNum()).
  • This change indicates that the maximum slot number may be dynamically determined based on some logic defined in the models package.

Suggestions for Improvement:

  • Without having access to the implementation of the models package, it's not possible to provide a comprehensive review or identify potential bugs.
  • However, here are some general suggestions:
    1. Ensure that the models.GetMaxSlotNum() function returns a reliable and accurate maximum slot number. Verify that it handles all possible scenarios correctly.
    2. Consider adding input validation for the models.GetMaxSlotNum() function to handle edge cases and validate the returned value.
    3. Review the documentation or comments for the models package to understand the purpose and behavior of the GetMaxSlotNum() function.
    4. Test the code thoroughly, considering both valid and invalid inputs for the slot IDs to ensure it behaves as expected.

It's important to keep in mind that without further context or the actual implementation of models and related functions, it's challenging to provide more specific feedback or identify potential bugs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

请忽略

chejinge
chejinge previously approved these changes Jul 1, 2023
@@ -358,7 +356,7 @@ void RPushxCmd::Do(std::shared_ptr<Slot> slot) {
rocksdb::Status s = slot->db()->RPushx(key_, values_, &llen);
if (s.ok() || s.IsNotFound()) {
res_.AppendInteger(llen);
RemKeyNotExists("l", key_, slot);
AddSlotKey("l", key_, slot);
} else {
res_.SetRes(CmdRes::kErrOther, s.ToString());
}
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, here are the suggested improvements:

  1. Removal of AddSlotKey("l", key_, slot); and RemKeyNotExists("l", key_, slot); calls in LPopCmd, LRemCmd, RPopCmd:
    Since these functions are no longer being used in the respective code blocks, you can safely remove them.

  2. Addition of AddSlotKey("k", receiver_, slot); in RPopLPushCmd:
    It appears that this line was missing before, so adding it makes sense to ensure consistent behavior.

  3. Change RemKeyNotExists("l", key_, slot); to AddSlotKey("l", key_, slot); in RPushCmd and RPushxCmd:
    Since the intention is to add a key to the slot rather than removing a key, updating the function call to AddSlotKey aligns with the desired behavior.

Overall, the code review suggests removing unnecessary function calls and making adjustments for consistency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

请忽略

@@ -342,7 +340,6 @@ void SMoveCmd::Do(std::shared_ptr<Slot> slot) {
rocksdb::Status s = slot->db()->SMove(src_key_, dest_key_, member_, &res);
if (s.ok() || s.IsNotFound()) {
res_.AppendInteger(res);
AddSlotKey("s", src_key_, slot);
} else {
res_.SetRes(CmdRes::kErrOther, s.ToString());
}
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 is a brief code review:

  1. In SPopCmd::Do, the line AddSlotKey("s", key_, slot); has been removed. It seems like this line was responsible for adding a key to the slot keys list. If it was intended to be removed, make sure that there are no unintended side effects.

  2. In SRemCmd::Do, the line AddSlotKey("s", key_, slot); has been removed. Similar to the previous point, ensure that removing this line does not have any undesired consequences.

  3. In SMoveCmd::Do, the line AddSlotKey("s", src_key_, slot); has been removed. Verify that the removal of this line is intentional and doesn't introduce any issues.

Without more context about the overall functionality and purpose of these code changes, it's challenging to provide specific bug risks or improvement suggestions. Please review these changes in the broader context of your codebase and consider testing them appropriately to ensure that they work as expected.

Co-authored-by: liuyuecai <liuyuecai@360.cn>
@@ -342,7 +340,6 @@ void SMoveCmd::Do(std::shared_ptr<Slot> slot) {
rocksdb::Status s = slot->db()->SMove(src_key_, dest_key_, member_, &res);
if (s.ok() || s.IsNotFound()) {
res_.AppendInteger(res);
AddSlotKey("s", src_key_, slot);
} else {
res_.SetRes(CmdRes::kErrOther, s.ToString());
}
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 provided, here are the bug risks and improvement suggestions:

Bug Risks:

  1. In SAddCmd::Do, the call to AddSlotKey("s", key_, slot) is placed outside the condition block. This can potentially result in calling AddSlotKey even when an error occurs, which might not be the intended behavior.

Improvement Suggestions:

  1. Consider using const references for string parameters like key_ and members_ to avoid unnecessary copying.
  2. It seems that AddSlotKey is called in multiple functions. You may want to refactor this code to avoid duplication and improve maintainability.
  3. It would be helpful to review any error handling mechanisms and ensure that all potential error scenarios are handled correctly.

Note: More specific feedback and suggestions could be provided with a better understanding of the overall code context and requirements.

@AlexStocks AlexStocks merged commit f8da20c into OpenAtomFoundation:unstable Jul 3, 2023
10 checks passed
bigdaronlee163 pushed a commit to bigdaronlee163/pika that referenced this pull request Jun 8, 2024
…1664)

* add slot nums to config (OpenAtomFoundation#145)

* fix migrate list slot logic (OpenAtomFoundation#156)

* fix sadd slot (OpenAtomFoundation#157)

Co-authored-by: liuyuecai <liuyuecai@360.cn>

---------

Co-authored-by: liuyuecai <liuyuecai@360.cn>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants