Add a checksum field to DataResponseMetadata, and use it for timezone consistency
#6046
Conversation
robertbastian
force-pushed
the
checksum
branch
2 times, most recently
from
009cf1a to
8de9ae0
Compare
robertbastian
requested review from
Manishearth,
sffc,
zbraniecki and
a team
as code owners
robertbastian
force-pushed
the
checksum
branch
from
8de9ae0 to
86dd950
Compare
There was a problem hiding this comment.
Mostly looks good; let's get a design LGTM from someone else like @Manishearth since this is the first real use of DataResponseMetadata. Also have a few small questions
| @@ -1997,22 +1997,6 @@ impl<FSet: DateTimeNamesMarker> RawDateTimeNames<FSet> { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| fn load_mz_periods<P>( | |||
There was a problem hiding this comment.
Nit: I prefer the previous style of separate functions for loading separate keys. One important decision here is that we use Default as the DataRequest, and I don't want to inline that assumption to a bunch of places.
Just keep this function and make it return the DataResponseMetadata
| // TODO how does this behave on 32-bit platforms? | ||
| sub_map.insert(CHECKSUM_KEY, checksum as usize); |
There was a problem hiding this comment.
The checksum should be a u64 not a usize right?
There was a problem hiding this comment.
If I put a usize value into a ZeroAsciiTrie on a 64-bit system, and then access it on a 32-bit system, what will have happened to it? How many bits are stored on the wire?
There was a problem hiding this comment.
It uses usize in the API because it doesn't really care what integer is stored; it stores a varint. Most use cases of ZeroTrie have involved storing indices into an array of data sitting on the side, so that's mainly what the API was optimized for.
There was a problem hiding this comment.
We need a way to get 64 bits out regardless off the usize length then.
There was a problem hiding this comment.
I suggest adding an API to ZeroTrie to get/set unsigned ints of specific sizes.
|
| time_zone/bcp47_to_iana@1, <singleton>, 7622B, 7600B, 35e1d2b320b41234 | ||
| time_zone/iana_to_bcp47@3, <singleton>, 9543B, 9500B, 7218ae639c063764 |
There was a problem hiding this comment.
Nit (optional): It's not clear whether these sizes include the checksum. I think they should, at least for postcard.
There was a problem hiding this comment.
They don't, postcard doesn't track indexing overhead either, so I don't think we need to track checksizes
| // TODO how does this behave on 32-bit platforms? | ||
| sub_map.insert(CHECKSUM_KEY, checksum as usize); |
There was a problem hiding this comment.
I suggest adding an API to ZeroTrie to get/set unsigned ints of specific sizes.
|
|
||
| if let Some(checksum) = metadata.checksum { | ||
| write!( | ||
| &mut fs::File::create(format!("{}_checksum", path_buf.display()))?, |
| } else if let Some(checksum) = metadata.checksum { | ||
| write!( | ||
| &mut fs::File::create(path_buf.join(".checksum"))?, | ||
| "{checksum}" |
There was a problem hiding this comment.
Suggestion: write the checksum as a hexadecimal literal.
| } | ||
| .ok() | ||
| .and_then(|s| s.parse().ok()); | ||
| if !marker.is_singleton { |
There was a problem hiding this comment.
Question/Issue: Why did you move the is_singleton condition above the marker_attributes condition
| where | ||
| P: BoundDataProvider<M> + ?Sized, | ||
| Self: Sized, | ||
| { | ||
| let arg_variables = variables; | ||
| match &self.inner { | ||
| OptionalNames::SingleLength { variables, .. } if arg_variables == *variables => { | ||
| return Ok(Ok(())); | ||
| return Ok(Ok(Default::default())); |
There was a problem hiding this comment.
Oof, is this ok? Or should we retain the DataResponseMetadata?
Based on #6043