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

#1029: json-numincrby-nummultby-toggle-forget-del migration and add integration testcases #1261

Conversation

vpsinghg
Copy link
Contributor

@vpsinghg vpsinghg commented Nov 9, 2024

This PR includes changes for migration of JSON.DEL, JSON.FORGET, JSON.TOGGLE, JSON,NUMINCRBY and JSON.NUMMULTBY command to new Eval method.
This resolves issue #1029

Also introduces fix for oom err in resp protocol integration test on lower spec machines.

Improved Integration Test format.

Tasks 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.

@vpsinghg
Copy link
Contributor Author

vpsinghg commented Nov 9, 2024

@AshwinKul28 @ayadav16 @lucifercr07
make test all integration passing on local but failing in CI pipeline

@apoorvyadav1111
Copy link
Contributor

Hi @vpsinghg , Just to confirm, Have you rebased this with the latest master?

@vpsinghg
Copy link
Contributor Author

vpsinghg commented Nov 9, 2024

Yes, No conflicts as I have created new branch and added my changes.
Can anyone retrigger the pipeline?

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.

Woah! Amazing work. Looking forward to take these changes to master soon. Meanwhile I have left few comments. Please check

integration_tests/commands/async/json_test.go Outdated Show resolved Hide resolved
integration_tests/commands/http/json_test.go Outdated Show resolved Hide resolved
integration_tests/commands/resp/json_test.go Outdated Show resolved Hide resolved
integration_tests/commands/resp/setup.go Show resolved Hide resolved
integration_tests/commands/websocket/json_test.go Outdated Show resolved Hide resolved
internal/server/cmd_meta.go Outdated Show resolved Hide resolved
testutils/json.go Outdated Show resolved Hide resolved
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.

edit: requesting changes, could'nt edit previous comment

@apoorvyadav1111
Copy link
Contributor

@vpsinghg , checks are okay now, I guess we just need to update some minor details and we are good to close this.

@vpsinghg vpsinghg force-pushed the migration/json-numincrby-nummultby-toggle-forget-del branch from 4f46b42 to 2e7e599 Compare November 10, 2024 03:49
@vpsinghg
Copy link
Contributor Author

@apoorvyadav1111 changes done. and check is also successful. Please go ahead

@vpsinghg
Copy link
Contributor Author

@apoorvyadav1111 please review

@apoorvyadav1111 apoorvyadav1111 changed the title migration: json-numincrby-nummultby-toggle-forget-del and add integration testcases #1029: json-numincrby-nummultby-toggle-forget-del migration and add integration testcases Nov 12, 2024
@apoorvyadav1111 apoorvyadav1111 merged commit cf14637 into DiceDB:master Nov 12, 2024
1 check passed
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