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

Enhancements and Refactor for Maintainability, and Performance #1616

Closed
wants to merge 4 commits into from

Conversation

stav-bentov
Copy link
Contributor

Feature Addition:

  • Added IP address validation in the node address update logic to ensure robust input handling and prevent invalid configurations.
    Commit: a7178a383

Refactoring:

  • Cluster Size Computation: Extracted cluster size computation into a helper function for improved readability and reusability.
    Commit: a3dd42bf4
  • File Event Processing: Encapsulated loop logic into a dedicated function processSingleFileEvent() to isolate responsibilities and simplify the main code flow.
    Commit: 215dcd602
  • Key Expiration Check: Optimized the key expiration check logic with branch prediction hints, enhancing performance during runtime.
    Commit: 733ef16c6

Copy link
Member

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

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

we don't do this refactor thing for this small reason, it breaks the git blame log

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

This PR contains three changes unrelated to each other:

  1. Config validation, makes CONFIG SET return an error in some case? Where are the tests?
  2. Branch prediction help. Some benchmark numbers or flame graph to motivate this change would be appreciated.
  3. Move code. Reduced nesting is arguably a good idea, but some might say it is moved out of its context. It does not objectively make the code better. IMO the benefit does not outweigh the cost, considering extra effort by reviewers (not easy to see if the code is changed or just moved, it takes time) and the risk of merge conflicts on PRs and downstream forks.

1 and 2 can maybe be merged, but they're unrelated to each other so the should be in separate PRs. For 3 I agree with @enjoy-binbin.

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.

3 participants