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

Introducing JSONL Store #22

Merged
merged 5 commits into from
Dec 17, 2024
Merged

Introducing JSONL Store #22

merged 5 commits into from
Dec 17, 2024

Conversation

taras
Copy link
Member

@taras taras commented Nov 24, 2024

Motivation

It's useful to organize data into JSONL files on the file system. I'm introducing JSONL Store, which allows me to store the content from external sources as files on the file system.

Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

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

I would consider making the json cache implementation a resource rather than having every operation go from a cold start. It's probably less important for something on the file system, but for remote caches, or DB caches, or redis caches, initializing once and then having instance access to the hot connection for every operation is a good practice. The way you can say:

let impl = yield* initMyCacheImpl() // <== impl is bound to this scope.
CacheContext.with(impl, function*() {
  // do stuff
});

cache/types.ts Outdated Show resolved Hide resolved
cache/jsonl.ts Outdated Show resolved Hide resolved
@taras
Copy link
Member Author

taras commented Nov 24, 2024

I would consider making the json cache implementation a resource rather than having every operation go from a cold start. It's probably less important for something on the file system, but for remote caches, or DB caches, or redis caches, initializing once and then having instance access to the hot connection for every operation is a good practice. The way you can say:

let impl = yield* initMyCacheImpl() // <== impl is bound to this scope.
CacheContext.with(impl, function*() {
  // do stuff
});

@cowboyd is this something that I need to provide now, or can we wait until we have a cache adapter that requires a hot connection?

@taras taras requested a review from cowboyd November 24, 2024 22:27
@cowboyd
Copy link
Member

cowboyd commented Nov 24, 2024

Are you sure you don't need a hot connection in this case? What is the expectation around concurrent reads and writes.

@taras
Copy link
Member Author

taras commented Nov 24, 2024

Are you sure you don't need a hot connection in this case? What is the expectation around concurrent reads and writes.

I'm really not sure. Currently, I don't have any use cases where multiple things are reading/writing the cache simultaneously. Most of it is very serial. Most of the time, it's essentially just reading data over the network, caching it to the file system so it can be read from the file system instead of going over the network again.

@taras taras changed the title Introducing Cache with useCache helper Introducing JSONL Store Dec 14, 2024
Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

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

Approving. I'm still a little bit worried about synchronization between reads and writes. I.e. if you have multiple reads going, what happens if a write happens in parallel, or should there be a queue so that writes cause things to happen in parallel.

jsonl-store/jsonl.ts Show resolved Hide resolved
@taras taras merged commit 0e38873 into main Dec 17, 2024
3 checks passed
@taras taras deleted the tm/add-persistent-cache branch December 17, 2024 19:19
@cowboyd
Copy link
Member

cowboyd commented Dec 17, 2024

Approving. I'm still a little bit worried about synchronization between reads and writes. I.e. if you have multiple reads going, what happens if a write happens in parallel, or should there be a queue so that writes cause things to happen in parallel.

@taras You may want to put a heads up in the readme that it has not been tested with concurrent reads and writes.

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