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(PE-2256): define chunk data cache interface and implementation #12

Merged
merged 17 commits into from
Sep 22, 2022

Conversation

dtfiedler
Copy link
Collaborator

@dtfiedler dtfiedler commented Sep 20, 2022

Details

  • Creates new class FsChunkCache that implements ChunkSource and ChunkDataCache interfaces
  • Checks data/chunks/<data_root>/data/<relativeOffset> for cached chunk data
  • Falls back to retrieving chunk data from ChunkSource if chunk data is not found in the cache, then saves them to cache as unencoded Buffer's for future retrieval

@dtfiedler dtfiedler marked this pull request as ready for review September 20, 2022 19:35
@dtfiedler dtfiedler marked this pull request as draft September 20, 2022 19:40
@dtfiedler dtfiedler marked this pull request as ready for review September 20, 2022 20:25
@ar-io ar-io deleted a comment from ario-bot Sep 20, 2022
@ar-io ar-io deleted a comment from ario-bot Sep 20, 2022
@dtfiedler dtfiedler changed the base branch from main to develop September 20, 2022 21:25
src/arweave/composite-client.ts Outdated Show resolved Hide resolved
src/arweave/composite-client.ts Show resolved Hide resolved
}

function chunkCachePath(dataRoot: Buffer, relativeOffset: number) {
return `${chunkCacheDir(dataRoot, relativeOffset)}/${relativeOffset}.msgpack`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This ticket is intended to only address data caching (see the "Note: ..." at the bottom of the ticket description). We should be storing raw data bytes without any MessagePack encoding.

src/types.d.ts Outdated
@@ -132,6 +138,16 @@ export interface PartialJsonTransactionCache {
set(tx: PartialJsonTransaction): Promise<void>;
}

export interface JsonChunkCache {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above, and we'll likely want a different interface interface for metadata when the time comes for that. I think a combo of my insufficient explanation + taking queues from header caching is leading you astray. 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed this here f917a07

@@ -194,6 +196,24 @@ export function msgpackTxToJsonTx(
};
}

export function msgpackChunkToJsonChunk(
Copy link
Collaborator

Choose a reason for hiding this comment

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

These aren't technically needed since this ticket is only about data rather than metadata caching, but we may need something similar for upcoming tickets. However, for the metadata use case, they shouldn't include the actual chunk data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

going to remove for now, will add back with that ticket

src/types.d.ts Outdated Show resolved Hide resolved
relativeOffset: number,
): Promise<JsonChunk> {
try {
const chunkPromise = this.get(dataRoot, relativeOffset).then(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the caching logic from this and let it always fall through to the network. Metadata caching will be addressed in a future ticket.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

modified in c1de268

dataRoot: Buffer,
relativeOffset: number,
): Promise<Readable> {
const { chunk } = await this.getChunkByRelativeOrAbsoluteOffset(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add logic here to cache just the chunk data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

now caching only chunk data as unencoded buffer c1de268

@dtfiedler dtfiedler changed the title feat(PE-2256): define chunk cache interface and implementation feat(PE-2256): define chunk data cache interface and implementation Sep 21, 2022
Copy link
Collaborator

@djwhitt djwhitt left a comment

Choose a reason for hiding this comment

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

Yeah, this is what I was trying to describe in the ticket. Thanks for the update!

@dtfiedler dtfiedler merged commit 151bd15 into develop Sep 22, 2022
@dtfiedler dtfiedler deleted the PE-2256-chunk_cache branch September 22, 2022 14: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.

None yet

2 participants