-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 decompression traits and a test case #2295
base: master
Are you sure you want to change the base?
Changes from 3 commits
72dd40d
bcf1b6c
cf227f5
a7324f7
a4c891e
5251318
beaee56
f78b718
eadecbe
f8db782
75ae124
8c6996a
aca530a
43f791c
3b1bd25
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 |
---|---|---|
|
@@ -94,6 +94,19 @@ impl CombinedDatabase { | |
}) | ||
} | ||
|
||
/// A test-only temporary rocksdb database with given rewind policy. | ||
#[cfg(feature = "rocksdb")] | ||
pub fn from_state_rewind_policy( | ||
Dentosal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
state_rewind_policy: StateRewindPolicy, | ||
) -> DatabaseResult<Self> { | ||
Ok(Self { | ||
on_chain: Database::rocksdb_temp(state_rewind_policy)?, | ||
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. perhaps we should use 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 the reason behind this? 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. because we have a way to clean up the directory after shutdown.. not sure if this method deletes the db after we are done with it. |
||
off_chain: Database::rocksdb_temp(state_rewind_policy)?, | ||
relayer: Database::rocksdb_temp(state_rewind_policy)?, | ||
Dentosal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
gas_price: Default::default(), | ||
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. out of curiosity, why the difference for the 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. @xgreenx might know, I have no idea 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. Because gas price table and relayer table doesn't need to support state rewind in the case of the temporary database(right now at least). Because we don't have any logic around it. |
||
}) | ||
} | ||
|
||
pub fn from_config(config: &CombinedDatabaseConfig) -> DatabaseResult<Self> { | ||
let combined_database = match config.database_type { | ||
#[cfg(feature = "rocksdb")] | ||
|
@@ -103,7 +116,9 @@ impl CombinedDatabase { | |
tracing::warn!( | ||
"No RocksDB path configured, initializing database with a tmp directory" | ||
); | ||
CombinedDatabase::default() | ||
CombinedDatabase::from_state_rewind_policy( | ||
config.state_rewind_policy, | ||
)? | ||
} else { | ||
tracing::info!( | ||
"Opening database {:?} with cache size \"{}\" and state rewind policy \"{:?}\"", | ||
|
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.
Should this also be
"test-helpers"
if it's "test-only"?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.
Done in 5251318
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.
I reverted this in beaee56 since it seems like we're using it with when empty path is provided, and I'm not sure if we need that outside tests as well.