-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Run migration to rewrite canonical IDs #7850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Generated by 🚫 Danger |
Coverage ReportAffected SDKs
Test Logs |
#include "absl/strings/match.h" | ||
#include "target_data.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100%, but I think we mostly use absolute paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to blame Clion for this one.
StringReader reader{target_it->value()}; | ||
auto message = Message<firestore_client_Target>::TryParse(&reader); | ||
if (!reader.ok()) { | ||
HARD_FAIL("Target proto failed to parse: %s", reader.status().ToString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we return a failed status here (and below)? It is probably best not to crash during schema conversion since we load all existing data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
auto query = Query("collection").AddingFilter(Filter("foo", "==", "bar")); | ||
TargetData initial_target_data(query.ToTarget(), | ||
/* target_id= */ 2, | ||
/*sequence_numder*/ 1, QueryPurpose::Listen); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/*sequence_numder*/ 1, QueryPurpose::Listen); | |
/* sequence_numder= */ 1, QueryPurpose::Listen); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
transaction.Delete(it->key()); | ||
std::string empty_buffer; | ||
transaction.Put(new_key, empty_buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to write the old data back (empty_buffer
sounds suspicious).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This table always has empty value: https://osscs.corp.google.com/firebase-sdk/firebase-ios-sdk/+/master:Firestore/core/src/local/leveldb_target_cache.cc;l=110
It serves as an association between canonical id and target id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
|
||
transaction.Delete(it->key()); | ||
std::string empty_buffer; | ||
transaction.Put(new_key, empty_buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
No description provided.