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.

Sign up for GitHub

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

Jump to bottom

Node: update commands which return a Record with GlideString. #2207

Merged
merged 18 commits into from
Sep 10, 2024
Merged

Node: update commands which return a Record with GlideString. #2207

merged 18 commits into from
Sep 10, 2024

Conversation

Yury-Fridlyand
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand commented Aug 28, 2024 •
edited
Loading

Note: only return type is changed in scope of this PR. Input arguments for these commands will be updated in other PRs.

TODO:

  • Complete code todos
  • Complete updating commands:
    • xinfo streams
    • function stats
    • function list
    • commands which return ClusterValue
  • Update examples in docs
  • Update description of returned types where changed
  • Update transaction
  • Fix tests
  • Update exports
  • Add new tests
  • changelog
  • fix flaky IT "should pass maps received from socket"

List of commands:

  • LMPOP
  • BLMPOP
  • BZMPOP
  • ZPOPMIN
  • ZPOPMAX
  • ZMPOP
  • ZDIFFWITHSCORES
  • ZINTERWITHSCORES
  • ZUNIONWITHSCORES
  • HGETALL
  • LCSIDX - no return type change
  • PUBSUB NUMSUB - Done
  • PUBSUB SHARDNUMSUB - Done
  • notificationToPubSubMessageSafe: PSubscribe, Subscribe, SSubscribe, Unsubscribe, SUnsubscribe, PUnsubscribe
  • XRANGE - no return type change
  • XREVRANGE - no return type change
  • XINFO GROUPS - no return type change
  • XINFO CONSUMERS - no return type change
  • XINFO STREAM - no return type change
  • XCLAIM - no return type change
  • XAUTOCLAIM - no return type change
  • XREAD
  • XREADGROUP

Notes:

  • Due to value conversion done, the response type differs in client and transaction implementation of all listed commands. For example, HGETALL returns HashDataType, but if called in a transaction, it returns GlideRecord<GlideString>.

@Yury-Fridlyand @acarbonetto
Spike: returning a record replacement with glide strings. (#418)
3e9264f 
* Spike: returning a record replacement with glide strings.

Signed-off-by: Yury-Fridlyand <[email protected]>
Co-authored-by: Andrew Carbonetto <[email protected]>
@Yury-Fridlyand Yury-Fridlyand added the node Node.js wrapper label Aug 28, 2024
@Yury-Fridlyand Yury-Fridlyand mentioned this pull request Aug 29, 2024
Merged
@yipin-chen @Yury-Fridlyand
Node: Add binary variant to PUBSUB NUMSUB and PUBSUB SHARDNUMSUB (#…
ff46c1f 
…419)

* Updated PubSub shardnumsub and numsub commands

---------

Signed-off-by: Yi-Pin Chen <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]>
prateek-kumar-improving
prateek-kumar-improving reviewed Aug 31, 2024
View reviewed changes
node/src/BaseClient.ts Outdated
public async hgetall(key: string): Promise<Record<string, string>> {
return this.createWritePromise(createHGetAll(key));
public async hgetall(
key: string,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This branch is not synced with main? I have this as GlideString in local.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This branch updates only return type of these commands

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Merged and fixed, please validate

Yury-Fridlyand and others added 4 commits September 3, 2024 19:09
@Yury-Fridlyand
Fix tests.
8542806 
Signed-off-by: Yury-Fridlyand <[email protected]>
@Yury-Fridlyand
Fix tests.
e1904ae 
Signed-off-by: Yury-Fridlyand <[email protected]>
@yipin-chen
Add binary variant to PUBSUB subscribe commands (#420)
04e9334 
* Node: add binary support for PubSub commands, part 1 (valkey-io#2201)

* Add binary variant to PUBSUB subscribe commands

---------

Signed-off-by: Yi-Pin Chen <[email protected]>
@Yury-Fridlyand
Merge remote-tracking branch 'upstream/main' into node/integ-valkey-2…
b8bf37b 
…93-return-record-with-glidestring

Signed-off-by: Yury-Fridlyand <[email protected]>
@Yury-Fridlyand Yury-Fridlyand linked an issue Sep 5, 2024 that may be closed by this pull request
Node: Record does not accept GlideString as a key #2152
Closed
This was referenced Sep 5, 2024
Closed
Merged
acarbonetto
acarbonetto reviewed Sep 5, 2024
View reviewed changes
node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/BaseClient.ts Show resolved Hide resolved
Yury-Fridlyand and others added 2 commits September 5, 2024 15:05
@Yury-Fridlyand @acarbonetto
Fix processing cluster response. (#421)
6d832a4 
* Fix processing cluster response.

Signed-off-by: Yury-Fridlyand <[email protected]>
Co-authored-by: Andrew Carbonetto <[email protected]>
@Yury-Fridlyand
Signed-off-by: Yury-Fridlyand <[email protected]>
eac46e9
@Yury-Fridlyand Yury-Fridlyand marked this pull request as ready for review September 6, 2024 01:39
@Yury-Fridlyand Yury-Fridlyand requested a review from a team as a code owner September 6, 2024 01:39
@Yury-Fridlyand Yury-Fridlyand changed the title [WIP] Node: update commands which return a Record with GlideString. Node: update commands which return a Record with GlideString. Sep 6, 2024
GumpacG
GumpacG approved these changes Sep 6, 2024
View reviewed changes
node/src/Commands.ts Outdated Show resolved Hide resolved
node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/BaseClient.ts Show resolved Hide resolved
node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/BaseClient.ts Show resolved Hide resolved
node/src/GlideClusterClient.ts Show resolved Hide resolved
@Yury-Fridlyand Yury-Fridlyand merged commit 71d8558 into valkey-io:main Sep 10, 2024
15 checks passed
@Yury-Fridlyand Yury-Fridlyand deleted the node/integ-valkey-293-return-record-with-glidestring branch September 10, 2024 20:15
janhavigupta007 pushed a commit to janhavigupta007/glide-for-redis that referenced this pull request Sep 11, 2024
@Yury-Fridlyand @acarbonetto
@yipin-chen @janhavigupta007
Node: update commands which return a Record with GlideString. (va…
a6151bd 
…lkey-io#2207)

* returning a record replacement with glide strings.
* Node: Add binary variant to `PUBSUB NUMSUB` and `PUBSUB SHARDNUMSUB` (valkey-io#419)
* Updated PubSub shardnumsub and numsub commands
* Fix tests.
* Add binary variant to PUBSUB subscribe commands
* Fix processing cluster response.

Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yi-Pin Chen <[email protected]>
Co-authored-by: Andrew Carbonetto <[email protected]>
Co-authored-by: Yi-Pin Chen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prateek-kumar-improving prateek-kumar-improving prateek-kumar-improving left review comments

@GumpacG GumpacG GumpacG approved these changes

@acarbonetto acarbonetto acarbonetto approved these changes

@yipin-chen yipin-chen yipin-chen approved these changes

Assignees
No one assigned
Labels
node Node.js wrapper
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Node: Record does not accept GlideString as a key
5 participants
@Yury-Fridlyand @GumpacG @acarbonetto @yipin-chen @prateek-kumar-improving