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

WIP Fixes(app): #1015 Command Migration: ('DEL', 'EXISTS', 'PERSIST', 'TYPE') #1146

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

mohit-nagaraj
Copy link

Pull Request Description for Issue #1015

Description:
This pull request addresses issue #1015 by migrating the 'DEL', 'EXISTS', 'PERSIST', and 'TYPE' commands to make them compatible across the three protocols supported by DiceDB: RESP, HTTP, and WebSocket. The current implementation of these commands is specific to the RESP protocol. The migration ensures protocol-agnostic behavior by refactoring the evaluation functions to return raw responses instead of protocol-specific responses.

Changes Made:

  1. Refactored Evaluation Functions:

    • Implemented new generic evaluation functions for the mentioned commands in internal/eval/store_eval.go.
    • Ensured that the evaluation functions do not include protocol-specific logic.
    • Used raw types without encoding in the return statements.
    • Incorporated errors from errors/migrated_errors.go and predefined responses from internal/clientio/resp.go.
  2. Command/Worker Specific Changes:

    • Set the IsMigrated flag to true in the command information under internal/eval/commands.go.
    • Assigned the new evaluation functions to the NewEval parameter in the command structure.
    • Removed the old evaluation functions from internal/eval/eval.go.
    • Added migrated commands to the CommandsMeta map in internal/worker/CommandsMeta with the type SingleShard.
  3. Unit and Integration Tests:

    • Refactored existing unit tests to accommodate the new implementation.
    • Added new unit tests to cover all possible cases.
    • Ran all integration tests to ensure successful migration and correct functionality for all protocols (RESP, HTTP, WebSocket).

Checklist:

  • Migrated the evalXXX function with the latest definition.
  • Updated or added unit tests for the new implementation.
  • Ensured all unit tests pass successfully.
  • Verified all integration tests pass successfully.

Additional Notes:

  • Edge cases and protocol-specific optimizations are handled within the wrappers.
  • Any questions or concerns about this migration can be mentioned here.

Related Issues/PRs:

  • Sample implementation for the Get, Set, GetSet, and SetEx commands can be found in a related pull request.

This pull request ensures the command logic is protocol-independent, allowing all three protocols to call the same core functionality seamlessly.

@mohit-nagaraj mohit-nagaraj marked this pull request as ready for review October 19, 2024 11:03
@mohit-nagaraj mohit-nagaraj changed the title WIP Fixes(app): #1015 Command Migration: ('DEL', 'EXISTS', 'PERSIST', 'TYPE') Fixes(app): #1015 Command Migration: ('DEL', 'EXISTS', 'PERSIST', 'TYPE') Oct 19, 2024
@JyotinderSingh
Copy link
Collaborator

Thanks for this PR. It looks like the CI is throwing some errors; please look into it. Additionally, please ensure the following checklist is part of your PR description and all items are checked off. At first glance, I can see that this PR does not add any new integration tests for the resp server.

  • Migrated the evalXXX function with the latest definition
  • Update or add unit tests for the new implementation.
  • All unit tests pass successfully.
  • Ensure all integration tests pass successfully.
  • Move Integration tests for the respective commands under the RESP integration tests directory from Async directory
  • Please validate that the documentation for the respective commands is up to date. If not then consider adding them.

@mohit-nagaraj
Copy link
Author

Hey @JyotinderSingh I have asked few doubts in the orginal issue could you please help me out with it

@mohit-nagaraj mohit-nagaraj changed the title Fixes(app): #1015 Command Migration: ('DEL', 'EXISTS', 'PERSIST', 'TYPE') WIP Fixes(app): #1015 Command Migration: ('DEL', 'EXISTS', 'PERSIST', 'TYPE') Oct 21, 2024
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.

2 participants