Skip to content

Conversation

@kinyoklion
Copy link
Member

This does not add the package to the release configuration yet.

The implementation contains an architectural overview with diagram, which should help with reviewing the code: https://github.com/launchdarkly/js-core/blob/rlamb/implement-redis-store/packages/store/node-server-sdk-redis/architecture.md

This implements both the redis persistent store as well as the redis big segment store.

registry-url: 'https://registry.npmjs.org'
# We may want to consider moving this build to a docker container instead of installing it
# in the image.
- run: |
Copy link
Member Author

Choose a reason for hiding this comment

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

The test suite needs a redis instance.

!.yarn/versions
yarn-error.log
.DS_Store
.vscode
Copy link
Member Author

Choose a reason for hiding this comment

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

This file is in another PR, and will not be in the diff after it is merged and I update the base.

"scripts": {
"clean": "yarn workspaces foreach -pt run clean",
"build": "yarn workspaces foreach -pt run build",
"build": "yarn workspaces foreach -p --topological-dev run build",
Copy link
Member Author

Choose a reason for hiding this comment

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

This file is in another PR, and will not be in the diff after it is merged and I update the base.

@@ -1,6 +1,15 @@
import ItemDescriptor from './ItemDescriptor';
Copy link
Member Author

Choose a reason for hiding this comment

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

This file is in another PR, and will not be in the diff after it is merged and I update the base.


public readonly diagnosticRecordingInterval: number;

// public readonly featureStore: LDFeatureStore;
Copy link
Member Author

Choose a reason for hiding this comment

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

This file is in another PR, and will not be in the diff after it is merged and I update the base.

@kinyoklion
Copy link
Member Author

kinyoklion commented Jun 13, 2023

I am seeing some errors when shutting down redis in a sample program. So I am checking those out. Functions fine, but errors on close.

Fixed.

close() {
if (this.owned) {
this.client.quit().catch(() => {
// Not any action that can be taken for an error on quit.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe a debug log is useful here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add one. Debug level is good. ioredis basically seems to produce nonsense errors when you quit it.

kinyoklion and others added 4 commits June 15, 2023 13:41
Co-authored-by: Yusinto Ngadiman <yusinto@gmail.com>
Co-authored-by: Yusinto Ngadiman <yusinto@gmail.com>
Co-authored-by: Yusinto Ngadiman <yusinto@gmail.com>
Copy link
Contributor

@yusinto yusinto left a comment

Choose a reason for hiding this comment

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

I'm approving, but I encounter something which is reproducible also in main. At repo root, doing yarn && yarn build freezes at sdk-server-edge. Maybe this is related to the topological flag added?

Screenshot_2023-06-15_at_2_04_16_PM

@kinyoklion
Copy link
Member Author

I'm approving, but I encounter something which is reproducible also in main. At repo root, doing yarn && yarn build freezes at sdk-server-edge. Maybe this is related to the topological flag added?

Screenshot_2023-06-15_at_2_04_16_PM

The second error happens whenever something cannot build. Being as you killed the build, some component didn't build so you got that error.

As for why it is haging that I cannot say. Mine does not. (Though it takes about 28 seconds to build everything.)

@kinyoklion kinyoklion changed the base branch from rlamb/feat/add-redis-persistent-store to main June 15, 2023 21:33
@kinyoklion kinyoklion merged commit fee603c into main Jun 15, 2023
@kinyoklion kinyoklion deleted the rlamb/implement-redis-store branch June 15, 2023 21:47
@github-actions github-actions bot mentioned this pull request Jun 15, 2023
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.

3 participants