Enhance TDH decoder to support manifest-based events and improve schema caching logic#313
Conversation
…or unsupported events
brianrob
left a comment
brianrob
left a comment
There was a problem hiding this comment.
Thanks for putting this together. A few questions / comments below.
| let map = if is_32bit { &self.cache_32 } else { &self.cache_64 }; | ||
| /// Returns the index of a cached TraceLogging schema, or `None` on a | ||
| /// miss. The key is hashed exactly once. | ||
| fn index_of_tl(&self, key: &[u8], is_32bit: bool) -> Option<usize> { |
There was a problem hiding this comment.
Question - should we consider a scheme that preserves index_of and insert instead of having TL and manifest specific methods? In this scheme, the methods would take metadata as a single parameter, perhaps an enum that would contain either the TL key info or the ManifestKey as well as is_32bit and event_type which could be TraceLogging or Manifest. This would allow for selection of the cache in a single method instead of having to pick the right one. This would simplify the API design.
There was a problem hiding this comment.
Good idea. The enum is actually already here as SchemaSource, and lookup_cached / insert_cached match on it today, so we're most of the way to what you're describing.
The remaining question is whether to push that match down into SchemaCache itself with a unified index_of(meta) / insert(meta, schema).
This relocates the branch rather than removing it. The two positive maps have different key types (HashMap<Vec<u8>, _> for TL vs HashMap<ManifestEventKey, _> for manifest), so a unified index_of still needs a 2-arm match inside to select the map pair, the "pick the right cache" step just moves into the cache instead of disappearing.
Additionally, lookup wants to borrow the TL key (&[u8]) while insert must own it (Vec<u8>), and the negative cache (is_manifest_unsupported) is manifest-only with no TL analog, so it stays separate regardless.
If we do fold it, I'd reuse the existing SchemaSource variant as the discriminant (rather than adding a parallel event_type field, which could drift out of sync with the variant) and keep is_32bit as its own parameter since it's orthogonal to the key.
Since this is purely an internal API-shape change with no behavior difference, I'd suggest we land it as a small follow-up so this PR stays scoped to the manifest/caching functionality, happy to open that up right after.
Let me know your thoughts.
There was a problem hiding this comment.
Sounds great. Your latest commit looks good too. Happy to consider the API shape change in a subsequent PR.
1370e9c to
46aa0df
Compare
3 participants
Enhance TDH decoder to support manifest-based events and improve schema caching logic
Issue: #310