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

✨ Add Redis Db #1559

Merged
merged 4 commits into from Jul 6, 2023
Merged

✨ Add Redis Db #1559

merged 4 commits into from Jul 6, 2023

Conversation

rmtuckerphx
Copy link
Contributor

Proposed Changes

Add Database integration for Redis including entry splitting and TTL. Users of the plugin have the ability to encrypt Redis values without that code being part of the plugin.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

@rmtuckerphx
Copy link
Contributor Author

@jankoenig @aswetlow How do we solve this build error?

/node_modules/@redis/client/dist/lib/client/socket.d.ts(39,5): error TS18028: Private identifiers are only available when targeting ECMAScript 2015 and higher.

@rmtuckerphx
Copy link
Contributor Author

Will removing tsconfig.build.esm5.json from db-redis solve this?
What is the impact?

@aswetlow aswetlow self-requested a review June 12, 2023 09:25
@aswetlow
Copy link
Member

Could you add "skipLibCheck": true to tsconfig.build.esm5.json? This should solve this issue.

@jankoenig jankoenig self-requested a review June 12, 2023 09:48
@aswetlow
Copy link
Member

What is the latency for connect() and disconnect()?

@rmtuckerphx
Copy link
Contributor Author

@aswetlow
Here are some timings for Redis connect and disconnect:

getDbItem - Redis connect: 247.458ms
getDbItem - Redis disconnect: 5.134ms
saveData - Redis connect: 160.305ms
saveData - Redis disconnect: 1.051ms

getDbItem - Redis connect: 238.067ms
getDbItem - Redis disconnect: 0.911ms
saveData - Redis connect: 143.946ms
saveData - Redis disconnect: 0.931ms

@aswetlow
Copy link
Member

aswetlow commented Jul 3, 2023

Would it make sense to keep the connection alive after getDbItem and reuse it in saveData? At least for the time of the request. You could store the client in jovo. This would save ~150ms in latency.

We could improve it, even more, by using connection pools. This is something that could be done in the future.

@rmtuckerphx
Copy link
Contributor Author

@aswetlow I vote to optimize in the future. This is something that I need immediately.

Copy link
Member

@jankoenig jankoenig left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@jankoenig jankoenig merged commit 4ca7c22 into jovotech:v4/dev Jul 6, 2023
6 checks passed
@jankoenig
Copy link
Member

Thank you @rmtuckerphx 🎉

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.

None yet

3 participants