Skip to content

Conversation

vktrl
Copy link
Contributor

@vktrl vktrl commented Oct 23, 2024

Use SCAN instead of KEYS when getting ids from Redis KV store.

From redis docs:

Warning: consider KEYS as a command that should only be used in production environments with extreme care. It may ruin performance when it is executed against large databases. This command is intended for debugging and special operations, such as changing your keyspace layout. Don't use KEYS in your regular application code. If you're looking for a way to find keys in a subset of your keyspace, consider using SCAN or sets.

Add support for ioredis

Added a new IoRedisKvStore. SCAN and raw command method signatures aren't compatible between node-redis and ioredis. Separate class is more maintainable and safer than making the redis client type an union and trying to figure out which client it's using at runtime.

Copy link

changeset-bot bot commented Oct 23, 2024

🦋 Changeset detected

Latest commit: 09544f1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@tus/utils Minor
@tus/server Minor
@tus/azure-store Patch
@tus/file-store Patch
@tus/gcs-store Patch
@tus/s3-store Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

socket-security bot commented Oct 23, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/[email protected] network +8 868 kB ioredis-robot

🚮 Removed packages: npm/@tus/[email protected]

View full report↗︎

Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Added changesets, docs, and ioredis to optionalDependencies otherwise the implementation can expect a different version than is used.

Code LGTM but we don't have automated tests for this. Can you confirm this works as expected locally?

@vktrl
Copy link
Contributor Author

vktrl commented Oct 29, 2024

Code LGTM but we don't have automated tests for this. Can you confirm this works as expected locally?

Yes. I wasn't familiar with how your tests are set up, but I ran some local tests to make sure. Should be easy enough to adapt.

import {createClient} from '@redis/client'
import {RedisKvStore} from './RedisKvStore'
import {IoRedisKvStore} from './IoRedisKvStore'
import {Redis} from 'ioredis'
import {randomUUID} from 'node:crypto'

const ioRedis = new Redis()
const redis = createClient()

const prefix = `kvstore-test:${Date.now()}:`

const store = new RedisKvStore(redis, prefix)
const ioStore = new IoRedisKvStore(ioRedis, prefix)

const run = async () => {
  const values: string[] = []

  for (let i = 0; i < 25; i++) {
    const value = randomUUID()
    values.push(value)
    await redis.set(`${prefix}${value}`, value, {EX: 60})
  }
  const ioList = await ioStore.list()
  const list = await store.list()
  console.log(list.length === values.length)
  console.log(ioList.length === values.length)
}

redis.connect().then(() => {
  run().then(() => {
    redis.disconnect().then(() => {
      process.exit(0)
    })
  })
})

@Murderlon Murderlon merged commit 8f19a53 into tus:main Oct 31, 2024
3 checks 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