store: remove integer values from DBCol variants #7441
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Discussion on Zulip: https://near.zulipchat.com/#narrow/stream/313099-pagoda.2Fstorage/topic/Using.20variant.20name.20as.20col_name @Longarithm, this might mess with any flat state tests you’re doing since it renames the FlatState column family. |
Longarithm
approved these changes
Aug 19, 2022
matklad
approved these changes
Aug 26, 2022
chain/client/src/rocksdb_metrics.rs
Outdated
Comment on lines
36
to
39
| /// Returns column's name as name in the DBCol enum without '_' prefix for deprecated columns. | ||
| fn col_verbose_name(col: near_store::DBCol) -> &'static str { | ||
| let name: &str = col.into(); | ||
| name.strip_prefix("_").unwrap_or(name) | ||
| <&str>::from(col) | ||
| } |
There was a problem hiding this comment.
this fn probably doesn't pull its weight anymore?
There was a problem hiding this comment.
Though, I'd maybe add this as a convenience API to use instead of <str>::from
impl DBCol {
pub fn name(&self) -> &'static str { self.into() }
}There was a problem hiding this comment.
We used to have this function but then it turns out it was only ever used when Display could be used so I got rid of it. At the moment it would be used in just two places so dunno really if it’s worth it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So far we’ve been using ‘col##’ names for the RocksDB column families.
This commit proposes to move away from this pattern for new columns
and instead use variant name as the column family name. For example,
rather than ‘col50’ for flat state we would simply use ‘FlatState’.
This has a couple of advantages. Firstly, there’s no longer need to
manually assign unique numbers to columns. Secondly, troubleshooting
on RocksDB level becomes easier since it’s simpler to map from column
family name to the column name used in source code.
As far as I can tell, RocksDB doesn’t have a simple way to rename
a column family. Because of that, this commit only makes the change
in names for future columns leaving currently existing ones with
‘col##’ names.