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

feat: add incrementBatch support #17

Merged
merged 3 commits into from
Oct 18, 2024

Conversation

mrnagydavid
Copy link
Contributor

Implementing the new db-lib addition incrementBatch.

Also disabling hsetWithTTL support because it is only supported >= 7.4.0.
I implemented it on Linux with the latest Redis.
Turns out that:

  • the MacOS supported version is 7.2.6
  • and our Memorystore is 7.0.12.

@@ -157,6 +159,25 @@ export class RedisClient implements CommonClient {
return await this.redis().hincrby(key, field, increment)
}

async hincrBatch(key: string, incrementTuples: [string, number][]): Promise<[string, number][]> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the batch support into our client-wrapper, because the withPipeline() exposes the underlying lib, and I don't think that consumers of our wrapper should be using that (if avoidable).

The consuming logic (redis*KeyValueDb.incrementBatch) has also its own business translating the keys - so this separation of concerns (here we pipe, there we translate ids) is also a plus,

Comment on lines +194 to +201
throw new Error('Not supported until Redis 7.4.0')
// const valueKeys = Object.keys(value)
// const numberOfKeys = valueKeys.length
// const keyList = valueKeys.join(' ')
// const commandString = `HEXPIREAT ${key} ${expireAt} FIELDS ${numberOfKeys} ${keyList}`
// const [command, ...args] = commandString.split(' ')
// await this.redis().hset(key, value)
// await this.redis().call(command!, args)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking open source, this is probably not a good idea.
Maybe a version check, and then throw?

On the other hand, none of OUR usually accessible redises support it.

@mrnagydavid mrnagydavid marked this pull request as ready for review October 18, 2024 08:50
@kirillgroshkov kirillgroshkov merged commit d85ff72 into master Oct 18, 2024
2 checks passed
@kirillgroshkov kirillgroshkov deleted the feat/add-increment-batch-support branch October 18, 2024 09:20
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