#1018 Migration: Append_GetRange #1095

bhima2001 · 2024-10-14T19:35:00Z

migrated GETRANGE from eval.go to store_eval.go, modified all the test cases. This pr closes #1018

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.

…t cases

…ng from the master branch for latest changes

internal/eval/eval_test.go

…d-getRange

…also removed the custom test executer with the one already present

benjaminmishra · 2024-10-15T13:22:48Z

LGTM

bhima2001 · 2024-10-15T13:28:56Z

I need to migrate APPEND too.

soumya-codes · 2024-10-15T13:30:24Z

@bhima2001 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.

…o updated exsisting testcases

JyotinderSingh · 2024-10-16T14:30:04Z

Please fix the merge conflicts

…d-getRange

…d websocket context

bhima2001 · 2024-10-17T15:24:58Z

@JyotinderSingh @benjaminmishra I am not able to identify the reason for why the integration tests are failing. can you please nudge into the right direction. Thanks.

benjaminmishra · 2024-10-17T15:40:58Z

@bhima2001 I can see at least one test failing may be more. Can I pull you branch to check it out ?

bhima2001 · 2024-10-17T15:48:17Z

You are talking about this test case right "FAIL: TestGetSet/GETSET_error_when_key_exists_but_does_not_hold_a_string_value". But this testcase is not failing when i testing it individually/

integration_tests/commands/async/getrange_test.go

lucifercr07

LGTM, minor comment.

integration_tests/commands/async/getrange_test.go

…d-getRange

…nto migrate-append-getRange

bhima2001 · 2024-10-19T19:29:25Z

Hey, @soumya-codes I have added docs for both "GETRANGE" and "APPEND" commands using the mentioned format. Please let me know if any further changes need to be made.

apoorvyadav1111

Hi @bhima2001 , Thanks for the changes, Please consider my review comments.

docs/src/content/docs/commands/APPEND.md

docs/src/content/docs/commands/GETRANGE.MD

integration_tests/commands/http/append_test.go

integration_tests/commands/http/getrange_test.go

apoorvyadav1111 · 2024-10-20T06:03:48Z

internal/eval/store_eval.go

+
+	key := args[0]
+	obj := store.Get(key)
+	if obj == nil {


it looks like we will return "" even if the start and end values are invalid. This is a deviation from the expected behavior for GETRANGE. Could you please check on this and add this test case in integration and unit tests. Thanks

@apoorvyadav1111 If the start and end are invalid then we throw the following error "ERR value is not an integer or out of range". The above mention case is when an object with the given key is not present in the store

Yes, but even in that case, we must not return a result but an error

apoorvyadav1111 · 2024-10-20T06:06:10Z

internal/eval/store_eval.go

+		}
+	case object.ObjEncodingInt:
+		str = strconv.FormatInt(obj.Value.(int64), 10)
+	default:


getrange works for other data type such as bytearray as well, please check and update code accordingly. Please add test cases in integration tests and unit tests. Thanks

apoorvyadav1111 · 2024-10-20T06:18:59Z

internal/eval/store_eval.go

+			storedValue, _ = strconv.ParseInt(value, 10, 64)
+		case object.ObjEncodingEmbStr, object.ObjEncodingRaw:
+			storedValue = value
+		default:


append can also work with other data types such as byte array, please check for others

apoorvyadav1111 · 2024-10-20T06:19:13Z

internal/eval/store_eval.go

+	case object.ObjEncodingEmbStr, object.ObjEncodingRaw:
+		// If the encoding is a string, retrieve the string value for concatenation
+		currentValueStr = obj.Value.(string)
+	default:


same as above

internal/eval/store_eval.go

apoorvyadav1111 · 2024-10-20T09:56:43Z

Hi @bhima2001 , please resolve the conversations that have been addressed and let me know if its ready for review.

lucifercr07

LGTM, Minor doc changes suggested.

docs/src/content/docs/commands/GETRANGE.MD

bhima2001 · 2024-10-20T12:32:54Z

@apoorvyadav1111 I would suggest to create a new issue for functionality changes for getrange and assign it to me. Because there are a lot of changes in this PR already and resolving merge conflicts everyday is getting more and more difficult. I would love to pick the code changes issue.
Thanks.

apoorvyadav1111 · 2024-10-20T17:33:06Z

Hi @bhima2001, please create an issue with the functionalities we discussed and I will assign that to you. Please close that issue on priority. This PR is ready to merge once all comments from @lucifercr07 are addressed.

Thanks for taking this humongous task and closing it.

apoorvyadav1111

Please rebase your PR. Changes are approved but cannot merge until conflicts

apoorvyadav1111

Hi @bhima2001 , Thank you these changes and addressing all the comments Approving the PR with an additional commit to fix some typos + addressing review from @lucifercr07 .

For Reviewer's Checklist for Migration label PR/Issues

moved code from eval.go to store_eval.go
updated cmd meta in eval, worker & server
moved tests from async to resp
added/updated tests in http and websocket
added/updated unit tests in eval folder
audited documentation and updated as per new standards
no changes in legacy async code / eval.go file
rebased with master
verified the code in local

Thanks

docs/src/content/docs/commands/GETRANGE.MD

bhima2001 added 3 commits October 14, 2024 19:33

migrated GETRANGE from eval.go to store_eval.go, modified all the tes…

30702b7

…t cases

resolved merge conflicts

a2f8697

made some lint changes, also resolved some conflicts becuase of pulli…

9ebf440

…ng from the master branch for latest changes

benjaminmishra suggested changes Oct 15, 2024

View reviewed changes

internal/eval/eval_test.go Show resolved Hide resolved

bhima2001 added 2 commits October 15, 2024 12:21

Merge branch 'master' of github.com:bhima2001/dice into migrate-appen…

6e42989

…d-getRange

modified testcases based on the function called to run the testcases …

7307eff

…also removed the custom test executer with the one already present

benjaminmishra approved these changes Oct 15, 2024

View reviewed changes

soumya-codes added the migration -- command Migrates current eval to a refactored eval for all protocols functionality label Oct 15, 2024

migrated append to store_eval.go, added this command ion cmd_meta als…

9d34fa7

…o updated exsisting testcases

bhima2001 added 6 commits October 16, 2024 18:31

Merge branch 'master' of github.com:bhima2001/dice into migrate-appen…

75921cc

…d-getRange

modified a unit testcase

2dba81a

added test cases for both append and getrange in HTTP, resp, async an…

65beb57

…d websocket context

added cmdType for append

7b1f301

modified APPEND and also added http test cases for GET RANGE

acac1ae

resolved merge conflicts

5b3a3c6

lucifercr07 requested changes Oct 17, 2024

View reviewed changes

integration_tests/commands/async/getrange_test.go Outdated Show resolved Hide resolved

removed apppend_test.go from async, also modified variables

cc9e940

bhima2001 requested a review from lucifercr07 October 18, 2024 06:48

lucifercr07 approved these changes Oct 18, 2024

View reviewed changes

integration_tests/commands/async/getrange_test.go Outdated Show resolved Hide resolved

removed the async/getrange_test.go

b3a09a1

soumya-codes self-requested a review October 18, 2024 13:43

bhima2001 added 2 commits October 18, 2024 17:56

Merge branch 'master' of github.com:bhima2001/dice into migrate-appen…

0ba35f1

…d-getRange

modified websockets assert package

455333c

bhima2001 added 2 commits October 19, 2024 19:27

added docs for both getrange and append

e8eed50

Merge branch 'migrate-append-getRange' of github.com:bhima2001/dice i…

490bee8

…nto migrate-append-getRange

apoorvyadav1111 mentioned this pull request Oct 20, 2024

#1036 Preserve TTL after APPEND command #1037

Merged

apoorvyadav1111 requested changes Oct 20, 2024

View reviewed changes

bhima2001 added 3 commits October 20, 2024 07:28

modified a typo in docs

5ec96be

modifed the testing package and also added clean up for all the tests

c0171ff

modified the arity for getrange and append

263dbf3

lucifercr07 requested changes Oct 20, 2024

View reviewed changes

bhima2001 added 2 commits October 20, 2024 11:04

resolved merge conflictrs

8e04343

made changes in getrange DOCs

d927082

bhima2001 and others added 2 commits October 20, 2024 20:02

Merge branch 'master' into migrate-append-getRange

6671f0b

resolved merge conflicts

03cacf1

bhima2001 force-pushed the migrate-append-getRange branch from 6671f0b to 03cacf1 Compare October 20, 2024 14:38

bhima2001 requested a review from apoorvyadav1111 October 20, 2024 14:47

apoorvyadav1111 requested changes Oct 20, 2024

View reviewed changes

bhima2001 added 2 commits October 20, 2024 17:59

merging after resolving conflicts

3059719

resolved merge conflicts

8a68b13

apoorvyadav1111 requested changes Oct 20, 2024

View reviewed changes

docs/src/content/docs/commands/GETRANGE.MD Outdated Show resolved Hide resolved

docs/src/content/docs/commands/GETRANGE.MD Outdated Show resolved Hide resolved

apoorvyadav1111 added 2 commits October 20, 2024 20:00

Update docs/src/content/docs/commands/GETRANGE.MD

88b0115

Update docs/src/content/docs/commands/GETRANGE.MD

804e9e8

apoorvyadav1111 approved these changes Oct 21, 2024

View reviewed changes

apoorvyadav1111 changed the title ~~Migration: Append_GetRange~~ #1018 Migration: Append_GetRange Oct 21, 2024

apoorvyadav1111 merged commit 26e7e02 into DiceDB:master Oct 21, 2024
2 checks passed

KanniShashankh pushed a commit to KanniShashankh/dice that referenced this pull request Oct 21, 2024

DiceDB#1018 Migration: Append, GetRange (DiceDB#1095)

4e34f46

This was referenced Oct 24, 2024

Enhancement: Append to support other datatypes #1193

Closed

Enhancement GETRANGE to support all available datatypes #1194

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1018 Migration: Append_GetRange #1095

#1018 Migration: Append_GetRange #1095

bhima2001 commented Oct 14, 2024 •

edited

Loading

benjaminmishra commented Oct 15, 2024

bhima2001 commented Oct 15, 2024

soumya-codes commented Oct 15, 2024 •

edited

Loading

JyotinderSingh commented Oct 16, 2024

bhima2001 commented Oct 17, 2024 •

edited

Loading

benjaminmishra commented Oct 17, 2024

bhima2001 commented Oct 17, 2024

lucifercr07 left a comment

bhima2001 commented Oct 19, 2024

apoorvyadav1111 left a comment

apoorvyadav1111 Oct 20, 2024

bhima2001 Oct 20, 2024 •

edited

Loading

apoorvyadav1111 Oct 20, 2024

apoorvyadav1111 Oct 20, 2024

apoorvyadav1111 Oct 20, 2024

apoorvyadav1111 Oct 20, 2024

apoorvyadav1111 commented Oct 20, 2024

lucifercr07 left a comment

bhima2001 commented Oct 20, 2024

apoorvyadav1111 commented Oct 20, 2024

apoorvyadav1111 left a comment

apoorvyadav1111 left a comment •

edited

Loading

#1018 Migration: Append_GetRange #1095

#1018 Migration: Append_GetRange #1095

Conversation

bhima2001 commented Oct 14, 2024 • edited Loading

benjaminmishra commented Oct 15, 2024

bhima2001 commented Oct 15, 2024

soumya-codes commented Oct 15, 2024 • edited Loading

JyotinderSingh commented Oct 16, 2024

bhima2001 commented Oct 17, 2024 • edited Loading

benjaminmishra commented Oct 17, 2024

bhima2001 commented Oct 17, 2024

lucifercr07 left a comment

Choose a reason for hiding this comment

bhima2001 commented Oct 19, 2024

apoorvyadav1111 left a comment

Choose a reason for hiding this comment

apoorvyadav1111 Oct 20, 2024

Choose a reason for hiding this comment

bhima2001 Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

apoorvyadav1111 Oct 20, 2024

Choose a reason for hiding this comment

apoorvyadav1111 Oct 20, 2024

Choose a reason for hiding this comment

apoorvyadav1111 Oct 20, 2024

Choose a reason for hiding this comment

apoorvyadav1111 Oct 20, 2024

Choose a reason for hiding this comment

apoorvyadav1111 commented Oct 20, 2024

lucifercr07 left a comment

Choose a reason for hiding this comment

bhima2001 commented Oct 20, 2024

apoorvyadav1111 commented Oct 20, 2024

apoorvyadav1111 left a comment

Choose a reason for hiding this comment

apoorvyadav1111 left a comment • edited Loading

Choose a reason for hiding this comment

bhima2001 commented Oct 14, 2024 •

edited

Loading

soumya-codes commented Oct 15, 2024 •

edited

Loading

bhima2001 commented Oct 17, 2024 •

edited

Loading

bhima2001 Oct 20, 2024 •

edited

Loading

apoorvyadav1111 left a comment •

edited

Loading