-
Notifications
You must be signed in to change notification settings - Fork 1k
Split the block cache into block pointer cache and block data cache #6037
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
Changes from all commits
727b845
044a125
7c80bfb
a2acdaa
1f7a117
7cb0690
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -576,10 +576,10 @@ pub trait ChainStore: ChainHeadStore { | |
/// Currently, the timestamp is only returned if it's present in the top level block. This format is | ||
/// depends on the chain and the implementation of Blockchain::Block for the specific chain. | ||
/// eg: {"block": { "timestamp": 123123123 } } | ||
async fn block_number( | ||
async fn block_pointer( | ||
&self, | ||
hash: &BlockHash, | ||
) -> Result<Option<(String, BlockNumber, Option<u64>, Option<BlockHash>)>, StoreError>; | ||
) -> Result<Option<(String, BlockNumber, Option<BlockTime>, Option<BlockHash>)>, StoreError>; | ||
|
||
/// Do the same lookup as `block_number`, but in bulk | ||
async fn block_numbers( | ||
|
@@ -665,10 +665,10 @@ pub trait QueryStore: Send + Sync { | |
/// Returns the blocknumber, timestamp and the parentHash. Timestamp depends on the chain block type | ||
/// and can have multiple formats, it can also not be prevent. For now this is only available | ||
/// for EVM chains both firehose and rpc. | ||
async fn block_number_with_timestamp_and_parent_hash( | ||
async fn block_pointer( | ||
&self, | ||
block_hash: &BlockHash, | ||
) -> Result<Option<(BlockNumber, Option<u64>, Option<BlockHash>)>, StoreError>; | ||
) -> Result<Option<(BlockNumber, Option<BlockTime>, Option<BlockHash>)>, StoreError>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And this could also just be called |
||
|
||
fn wait_stats(&self) -> PoolWaitStats; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
DATABASE_TEST_VAR_NAME := "THEGRAPH_STORE_POSTGRES_DIESEL_URL" | ||
DATABASE_URL := "postgresql://graph-node:let-me-in@localhost:5432/graph-node" | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's a justfile? This should be your local file, not something in the repo There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is similar to a make file, it's intentionally to be in the repo, provides some shortcuts for common operations, you don't need to use it yourself but it's useful to have for others There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
help: | ||
@just -l | ||
|
||
local-deps-up *ARGS: | ||
docker compose -f docker/docker-compose.yml up ipfs postgres {{ ARGS }} | ||
|
||
local-deps-down: | ||
docker compose -f docker/docker-compose.yml down | ||
|
||
test-deps-up *ARGS: | ||
docker compose -f tests/docker-compose.yml up {{ ARGS }} | ||
|
||
test-deps-down: | ||
docker compose -f tests/docker-compose.yml down | ||
|
||
# Requires local-deps, see local-deps-up | ||
test *ARGS: | ||
just _run_in_bash cargo test --workspace --exclude graph-tests -- --nocapture {{ ARGS }} | ||
|
||
runner-test *ARGS: | ||
just _run_in_bash cargo test -p graph-tests --test runner_tests -- --nocapture {{ ARGS }} | ||
|
||
# Requires test-deps to be running, see test-deps-up | ||
it-test *ARGS: | ||
just _run_in_bash cargo test --test integration_tests -- --nocapture {{ ARGS }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These can be just aliases in [alias]
store = "test -p graph-store-postgres"
tst = "test --workspace --exclude graph-tests"
docs = "doc --workspace --document-private-items"
gm = "install --bin graphman --path node --locked"
gmt = "install --bin graphman --path node --locked --root /var/tmp/cargo"
rt = "test -p graph-tests --test runner_tests"
it = "test -p graph-tests --test integration_tests -- --nocapture" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and that's local, this works for everyone. |
||
|
||
local-rm-db: | ||
rm -r docker/data/postgres | ||
|
||
new-migration NAME: | ||
diesel migration generate {{ NAME }} --migration-dir store/postgres/migrations/ | ||
|
||
_run_in_bash *CMD: | ||
#!/usr/bin/env bash | ||
export {{ DATABASE_TEST_VAR_NAME }}={{ DATABASE_URL }} | ||
{{ CMD }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all these
Option
still justified? I think they will all always beSome
. It would also be nicer to have a struct for this. Maybe call itBlockPointer
since it's one row from that table (andBlockPtr
is than a small excerpt from that)Also, this method should be renamed to
block_pointer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's not always a timestamp, on the shared storage model it still can be None
The option BlockTime is a little weird but I kept it because there is a different between Some(epoch time) and None, it's more idiomatic to have Option than checking BlockTime == BlockTime::NONE or MIN which are also in fact the same value (I didn't really get why).