GC targets without deserializing Target model #7026
Conversation
| } | ||
| } | ||
|
|
||
| int MemoryTargetCache::RemoveTargets( | ||
| uint64_t MemoryTargetCache::RemoveTargets( |
There was a problem hiding this comment.
Optional: consider returning size_t (size_t is an alias for an unsigned integer type that can store the largest possible object size. Strictly speaking, containers define their own size_type aliases, but they're usually the same as size_t whereas size_t is shorter to type and not tied to a particular container).
There was a problem hiding this comment.
I remember size_t from my good old days on App Engine. Updated.
| ASSERT_TRUE(target_data.target_id() > 20 || | ||
| target_data.target_id() % 2 == 1); | ||
| }); | ||
| int detectedRemoval = 0; |
There was a problem hiding this comment.
Nit: should be snake_case.
| }); | ||
|
|
||
| ASSERT_EQ(10, detectedRemoval); |
There was a problem hiding this comment.
Nit: don't use Yoda-style comparisons, please switch the argument order. Note: a counterargument would be consistency with existing comparisons in this file. Personally, I wouldn't worry about it, but feel free to reject this on those grounds.
There was a problem hiding this comment.
Fixed (this instance alone). I did get my inspiration from other occurrences in this file though.
| /** | ||
| * Parses the given bytes as a `firestore_client_Target` protocol buffer and | ||
| * then converts to the equivalent target data. | ||
| */ | ||
| TargetData DecodeTarget(absl::string_view encoded); | ||
|
|
||
| /** Removes the given targets from the query to target mapping */ | ||
| void RemoveQueryTargetKeyForTargets( |
There was a problem hiding this comment.
Ultranit: please add a full stop.
| @@ -269,6 +291,24 @@ void LevelDbTargetCache::RemoveAllKeysForTarget(TargetId target_id) { | |||
| } | |||
| } | |||
|
|
|||
| void LevelDbTargetCache::RemoveQueryTargetKeyForTargets( | |||
| std::unordered_set<TargetId> target_ids) { | |||
There was a problem hiding this comment.
This argument should be passed by const reference (reference and not value because this function doesn't take ownership of it, const because it only calls const functions on it).
|
|
||
| int resultCount = 0; | ||
| cache_->EnumerateSequenceNumbers([&](ListenSequenceNumber sequence_number) { | ||
| std::cout << sequence_number; |
There was a problem hiding this comment.
Looks like leftover debug code?
There was a problem hiding this comment.
Oops. Thanks for catching.
| int resultCount = 0; | ||
| cache_->EnumerateSequenceNumbers([&](ListenSequenceNumber sequence_number) { | ||
| std::cout << sequence_number; | ||
| ASSERT_TRUE(sequence_numbers.find(sequence_number) != |
There was a problem hiding this comment.
Optional: consider using GMock matchers:
EXPECT_THAT(sequence_numbers, Contains(sequence_number));
(matchers may provide more informative messages upon failure)
There was a problem hiding this comment.
Good suggestion. Done.
| TargetData target_data2 = MakeTargetData(testutil::Query("b")); | ||
| cache_->AddTarget(target_data2); | ||
|
|
||
| std::unordered_map<model::TargetId, TargetData> live_targets; |
There was a problem hiding this comment.
Nit: please include <unordered_map>.
There was a problem hiding this comment.
Done... but removed as per the suggestion below.
| TargetData target_data2 = MakeTargetData(testutil::Query("b")); | ||
| cache_->AddTarget(target_data2); | ||
|
|
||
| std::unordered_map<model::TargetId, TargetData> live_targets; |
There was a problem hiding this comment.
Nit: no need to specify the model:: namespace.
| cache_->AddTarget(target_data2); | ||
|
|
||
| std::unordered_map<model::TargetId, TargetData> live_targets; | ||
| cache_->RemoveTargets(target_data2.sequence_number(), live_targets); |
There was a problem hiding this comment.
Optional: you could use empty braces to simply create the empty map on the fly:
cache_->RemoveTargets(target_data2.sequence_number(), {});Applies below as well.
| index_iterator->Seek(index_prefix); | ||
|
|
||
| LevelDbQueryTargetKey row_key; | ||
| for (; index_iterator->Valid(); index_iterator->Next()) { |
There was a problem hiding this comment.
This is pessimizing target deletion considerably, making it O(targets) where it was previously O(targets-to-delete). Previously, RemoveTarget would construct the query-target key from the target data but now we're scanning all query-target rows.
It seems like we could try to construct the query-target key to delete based on the proto data and do like RemoveTarget always did.
Indeed I'm surprised that this this PR didn't just add a RemoveTarget overload that operated on a const message reference instead of changing this around to reimplement deletion.
Ultimately the workaround we're adding here is for a problem where the query proto has some invalid data in there. We haven't proven that this is happening because of general data corruption issues so I'm disinclined to rewrite all of this code in a way that doesn't trust the relationships built into the data. We should just not validate the Query part of the TargetData when removing a target.
There was a problem hiding this comment.
I agree with your concerns, but I went with this approach for two reasons:
-
We don't actually store the Canonical ID. Instead, we compute it from the Target model. That means that we have to fully deserialize into the model. If we don't validate, we end up with a Model that has an invalid field ordering, which we would then use to create a Canonical ID that is different from the original ID (unless they happen to both be corrupt). Hence, we don't know whether we would be able to delete the row from Level DB.
-
The LRU runs is already O(n) as it has to read all targets to collect the sequence numbers.
That's all I got for now :)
There was a problem hiding this comment.
Somehow I thought we stored the canonical ID, but you're right: the only place it exists is in the keys of the query-target mapping.
Your point re O(n) is valid, but it's a different "table" so it's more I/O. If there were a way to avoid this I'd push for it, but I can see how you arrived here.
I'm still concerned that regular deletion and GC deletion follow different code paths but it's definitely not worth pessimizing single target removal to make this line up, nor is it worth writing a migration to force these to line up. I guess this is as good as it's going to get.
There was a problem hiding this comment.
Regular deletion is actually not used for LevelDB, so we could use the new code path for RemoveTarget without much harm. Let me know if you'd like me to do so.
Firestore/CHANGELOG.md
Outdated
| @@ -1,5 +1,11 @@ | |||
| # Unreleased | |||
| - [fixed] The SDK no longer fully deserializes queries during garbage, | |||
There was a problem hiding this comment.
Extra comma after garbage.
Also consider rephrasing to be less technically detailed. Something like: "Fixed a crash that could happen when the SDK encountered invalid data during garbage collection.
| index_iterator->Seek(index_prefix); | ||
|
|
||
| LevelDbQueryTargetKey row_key; | ||
| for (; index_iterator->Valid(); index_iterator->Next()) { |
There was a problem hiding this comment.
Somehow I thought we stored the canonical ID, but you're right: the only place it exists is in the keys of the query-target mapping.
Your point re O(n) is valid, but it's a different "table" so it's more I/O. If there were a way to avoid this I'd push for it, but I can see how you arrived here.
I'm still concerned that regular deletion and GC deletion follow different code paths but it's definitely not worth pessimizing single target removal to make this line up, nor is it worth writing a migration to force these to line up. I guess this is as good as it's going to get.
This works around a crash during LRU by not converting the Target Proto into Target Models, and instead only using the Target ID and the LastListenSequenceNumber for serialization. It is the more complicated version of #7026, but it doesn't orphan data on disk.
Fix: #6721