Revise Save/Restore for true pit snapshot. #401
Conversation
Signed-off-by: Allen Samuels <allenss@amazon.com>
Signed-off-by: Allen Samuels <allenss@amazon.com>
Signed-off-by: Allen Samuels <allenss@amazon.com>
Signed-off-by: Allen Samuels <allenss@amazon.com>
allenss-amazon
force-pushed
the
saverestore
branch
from
0a657fd to
450e3b3
Compare
Signed-off-by: Allen Samuels <allenss@amazon.com>
allenss-amazon
changed the title
Signed-off-by: Allen Samuels <allenss@amazon.com>
Signed-off-by: Allen Samuels <allenss@amazon.com>
| absl::Status SaveString(const std::string_view s) { | ||
| return SaveChunk(s.data(), s.size()); | ||
| } | ||
| template <typename T, std::enable_if_t<std::is_trivial<T>::value && |
There was a problem hiding this comment.
Do we need enable_if_t ? I saw it limits this function only to certain types. What types does it limit the template to. In future are we expected to serialize any complex information ?
There was a problem hiding this comment.
This implementation won't handle complex information. If the need for a more sophisticated serialization framework comes up we'll tackle it then. The enable_if_t is seat belts for developers.
| : vmsdk::ThreadPool(name, num_threads) { | ||
| ON_CALL(*this, Schedule(testing::_, testing::_)) | ||
| .WillByDefault(testing::Invoke( | ||
| .WillByDefault( |
There was a problem hiding this comment.
I had seen this was causing build warnings , glad this is being fixed, what was the actual issue?
There was a problem hiding this comment.
Not sure why this wasn't caught earlier. I believe that testing::Invoke predates lambda functions and isn't needed for them.
| << tracked_mutated_records_.size(); | ||
| VMSDK_RETURN_IF_ERROR(out.SaveObject(tracked_mutated_records_.size())); | ||
| for (const auto &[key, value] : tracked_mutated_records_) { | ||
| VMSDK_RETURN_IF_ERROR(out.SaveString(key->Str())); |
There was a problem hiding this comment.
We don't save the value of records in the mutation queue?
There was a problem hiding this comment.
The values are not needed because they are "by definition" duplicates of what is in the main database.
src/indexes/numeric.cc
Outdated
| // | ||
| if (tracked_keys_.contains(key_ptr)) { | ||
| DCHECK(false); | ||
| return absl::InternalError("Numeric field save duplicate key"); |
There was a problem hiding this comment.
what happens when tracked/ untracked keys overlap between loading different indexes. I assume it would be handled automatically while backfilling?
There was a problem hiding this comment.
In theory tracked/untracked are disjoint sets. In the save/restore world it's better to crash the save than the restore. In other words, let's not write an invalid RDB file.
| import threading | ||
| from ft_info_parser import FTInfoParser | ||
|
|
||
| index = Index("index", [Vector("v", 3, type="HNSW", m=2, efc=1), Numeric("n"), Tag("t")]) |
There was a problem hiding this comment.
maybe we could add a flat vector field as well just to make testing more extensive. also maybe adding some deletes, updates and then saving restoring would also remove any doubts
There was a problem hiding this comment.
The PR doesn't change the save/restore of a vector index. It does suppress the post load scan for stale keys. But this logic isn't affected by HNSW vs FLAT.
| supplemental_content->mutation_queue_header().backfilling(); | ||
| if (!backfilling) { | ||
| VMSDK_LOG(DEBUG, ctx) << "Backfill suppressed."; | ||
| index_schema->backfill_job_.Get() = std::nullopt; |
There was a problem hiding this comment.
I imagine some consistency check here, but i guess that is a separate task
There was a problem hiding this comment.
Yes, in a perfect world we'd have additional checking code that could be enabled/disabled.
2 participants
Revises the save/restore logic to support saving of all index data and pending mutation queue as discussed earlier.
This new logic is fully backward and forward compatible. RDB files written using the previous binaries (aka V1) will still be loaded and subject to the same stale data correction and forced backfill logic as existed before this PR. In other words, for RDB files written with the old code, the new code handles them exactly the same.
New format files (aka V2) contain additional data that will be ignored by old code. In other words, V2 RDB files can still be loaded into prior code which will ignore the extra data, i.e., they are treated like V1 files and be subjected to stale data correction and forced backfill.
V2 files which are loaded into the new code are not subjected to stale data correction and forced backfill. Once a V2 file is loaded it can "open for business" immediately.
The V2 format contains an additional section for the contents of non-vector indexes (Tags, Numeric). These sections contain the key and attribute-value for each entry in the index. This external format differs from the internal format, allowing the internal data structures to be freely optimized in the future.
The V2 format also has a section for the mutation queue and index backfill status. This additional section records the keys that were in the mutation queue at the time of the save as well as the index backfill status (complete or incomplete). This allows the recreation of the mutation queue and backfill status (backfill needed or not) to match the state at the time of the save. (One small quirk is that the saved mutation queue doesn't bother to record whether the saved keys are in the mutation queue due to a backfill or not. This behavior should not affect the client-visible semantics, but would affect internal counters for the ingestion pipeline as they would be mislead by the source of the mutation.
Two controls are provided to override the normal behavior of this code.
The configurable:
search.rdb_write_v2(default "yes") controls whether a generated RDB file is in the V1 or V2 format.The configurable:
search.rdb_read_v2(default "yes") controls whether the V2 data is used. Setting this to "no" will force the code to treat a V2 file as a V1 file, i.e., to ignore the extra V1 data.