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

Migrate Commands: ('SETBIT', 'GETBIT', 'BITCOUNT', 'BITPOS', 'BITFIELD', 'BITFIELD_RO') #1089

Merged
merged 26 commits into from
Nov 9, 2024

Conversation

vishnuchandrashekar
Copy link
Contributor

@vishnuchandrashekar vishnuchandrashekar commented Oct 14, 2024

Migrates the eval functions for SETBIT, GETBIT, BITPOS, BITFIELD and BITFIELD_RO to the new eval function type independent of protocol.
Closes #1017

Checklist:

  • 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.
  • Ensure integration tests are added for the migrated command on multi-threaded resp server.
  • 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.

Notes:
Since the BITFIELD and BITFIELD_RO commands depend on the same function (bitfieldEvalGeneric) I have gone ahead and migrated both the commands.

@AshwinKul28 AshwinKul28 added the migration -- command Migrates current eval to a refactored eval for all protocols functionality label Oct 14, 2024
Copy link
Contributor

@AshwinKul28 AshwinKul28 left a comment

Choose a reason for hiding this comment

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

Huge changes. Thanks a lot for this @vishnuchandrashekar . Left few minor comments please check and also check CI runs.

@AshwinKul28 AshwinKul28 marked this pull request as ready for review October 15, 2024 13:16
@AshwinKul28
Copy link
Contributor

HI @vishnuchandrashekar thanks a lot for the changes. Looks good. Please rebase your branch with master to resolve conflicts.

@soumya-codes
Copy link
Contributor

soumya-codes commented Oct 15, 2024

@vishnuchandrashekar lets ensure that the corresponding integration tests are migrated as well. cc @AshwinKul28

I have added the checklist to the PR description. Please fill the same.
Please let me or Ashwin know if you have any doubts/concern.

@AshwinKul28
Copy link
Contributor

HI @vishnuchandrashekar hope you are doing well. Do you still have any blockers on this PR? please let me or others know on the Discord.

@vishnuchandrashekar
Copy link
Contributor Author

Hey @AshwinKul28, I've moved all the tests related to BIT to the file integration_tests/commands/resp/bit_test.go. Is this fine or do I need to do it the same way as it was done in async?

@apoorvyadav1111
Copy link
Contributor

Hi @vishnuchandrashekar , you can rename the file. Additionally, we do not required the async test cases anymore, if you have migrated all the tests to resp, please remove the async tests. Additionally, please add tests in websocket and http folder.

Thanks

@vishnuchandrashekar
Copy link
Contributor Author

Hey @AshwinKul28 / @apoorvyadav1111, some of the websocket and http tests seems to be failing. I am unsure as to why. Any suggestions?

@apoorvyadav1111
Copy link
Contributor

Hi @vishnuchandrashekar, I checked the lint run but there I can only see unit tests failing, Could you please share logs of failing websocket and HTTP tests on discord and tag me.
Thanks

@vishnuchandrashekar
Copy link
Contributor Author

vishnuchandrashekar commented Oct 22, 2024

Hi @apoorvyadav1111, apologies I meant the tests that I added. Mainly the http BITPOS tests, could you have a look at those? I'll share the logs on discord soon. Thanks.
EDIT: The same tests pass for resp and websocket. Maybe I'm doing something wrong in the http tests?

@apoorvyadav1111
Copy link
Contributor

Yes, please share your logs on discord. Thanks

@JyotinderSingh
Copy link
Collaborator

Hi @vishnuchandrashekar are you still blocked on this issue?

@vishnuchandrashekar
Copy link
Contributor Author

Hi @vishnuchandrashekar are you still blocked on this issue?

Hi @JyotinderSingh, yes I'm still facing issues. I've sent a message on discord. Currently working on updating/adding the documentation for the commands.

@AshwinKul28
Copy link
Contributor

HI @vishnuchandrashekar Changes look great. Please have a look at this reference doc for the documentation changes. I see a few tests are failing, consider fixing them as well. Again, thanks a lot for the commendable efforts

@AshwinKul28
Copy link
Contributor

HI @vishnuchandrashekar hope you are doing great. Can you please have a look at the documentation part as mentioned above? Also, is it possible to close this as early as possible? since we need to make DiceDB production ready soon for the upcoming major version release.

Also more delay causes more conflicts 😭 to resolve as well. Let me know if you need any sort of help. Thanks again!

@vishnuchandrashekar
Copy link
Contributor Author

Hey @AshwinKul28, I'll make sure to push the changes by the end of the day. Thanks for your patience.

@vishnuchandrashekar
Copy link
Contributor Author

Hey @AshwinKul28, some of the integration tests that I've added are failing namely:

HTTP:

  1. TestBitOpsString/Bitop_not_of_a_key_containing_a_string
  2. TestBitOpsString/Bitop_not_of_a_key_containing_an_integer
  3. TestBitOpsString/BITOP_OR_of_keys_containing_strings_and_a_bytearray_and_get_the_destkey

RESP:

  1. TestBitOpsString/BITOP_XOR_of_keys_containing_strings_and_get_the_destkey
  2. TestBitOpsString/BITOP_XOR_of_keys_containing_strings_and_a_bytearray_and_get_the_destkey

Websocket:

  1. TestBitOpsString/Bitop_not_of_a_key_containing_a_string
  2. TestBitOpsString/Bitop_not_of_a_key_containing_an_integer
  3. TestBitOpsString/BITOP_OR_of_keys_containing_strings_and_a_bytearray_and_get_the_destkey

Also I am unsure as to why the build tests are failing, the tests pass on my local.
I am currently looking into this, would like your suggestions.

@vishnuchandrashekar
Copy link
Contributor Author

Hey @AshwinKul28, I've fixed some of the failing test cases. I am still unsure why the RESP tests fail, would like your inputs.
@lucifercr07 - The http and websocket had some marshalling issues while returning the data, I've come up with a quick work around. Let me know what you think. I'm sure there is a better way to fix this.

@apoorvyadav1111
Copy link
Contributor

Hi @vishnuchandrashekar , #1245 is closed. Lets move ahead.

@apoorvyadav1111 apoorvyadav1111 changed the title Migrate Commands: ('SETBIT', 'GETBIT', 'BITCOUNT', 'BITOP', 'BITPOS', 'BITFIELD', 'BITFIELD_RO') Migrate Commands: ('SETBIT', 'GETBIT', 'BITCOUNT', 'BITPOS', 'BITFIELD', 'BITFIELD_RO') Nov 9, 2024
Copy link
Contributor

@apoorvyadav1111 apoorvyadav1111 left a comment

Choose a reason for hiding this comment

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

.....faaantastic work! Thanks for such a great effort.

@apoorvyadav1111 apoorvyadav1111 merged commit 44779c2 into DiceDB:master Nov 9, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migration -- command Migrates current eval to a refactored eval for all protocols functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command Migration: ('SETBIT', 'GETBIT', 'BITCOUNT', 'BITPOS', 'BITFIELD')
5 participants