Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2553143002:
Create a strict order in ReadingListSpecifics (Closed)
DescriptionCreate a strict order in ReadingListSpecifics
To avoid sync ping-pong where two device disagree on the latest state,
create a strict order for ReadingListSpecifice.
// Entries are in increasing order if all the fields respect increasing order.
// - URL must be the same.
// - title must verify rhs.title.compare(lhs.title) >= 0
// - rhs.creation_time_us >= lhs.creation_time_us
// - rhs.update_time_us >= lhs.update_time_us
// - if rhs.update_time_us > lhs.update_time_us, rhs.state can be anything.
// - if rhs.update_time_us == lhs.update_time_us, rhs.state >= lhs.state in
// the order UNSEEN, UNREAD, READ.
BUG=666232
Committed: https://crrev.com/f623a8a02f98adfb943529b9b25a6824bc62a97d
Cr-Commit-Position: refs/heads/master@{#437251}
Patch Set 1 #Patch Set 2 : for review #
Total comments: 20
Patch Set 3 : comments #
Total comments: 9
Patch Set 4 : order by field #
Total comments: 6
Patch Set 5 : title #Patch Set 6 : rebase #
Dependent Patchsets: Messages
Total messages: 27 (11 generated)
olivierrobin@chromium.org changed reviewers: + gambard@chromium.org, jif@chromium.org
High level comment: I think this is working only because the sync mechanism is intelligent enough to not send the entry if it already exist. If you receive an entry which is the same as one you already have, you will update your entry with the received one (doing nothing). Then you try to sync this entry, which may create a loop if the sync store sends the entry without checking if it is the same or not. WDYT? https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... File components/reading_list/ios/reading_list_entry.cc (right): https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_entry.cc:332: #if !defined(NDEBUG) What is the purpose of this? Can you add a comment? https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... File components/reading_list/ios/reading_list_entry_unittest.cc (right): https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_entry_unittest.cc:319: // Tests that the merging of two ReadingListEntry. Update comment https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_entry_unittest.cc:343: // Tests that the merging of two ReadingListEntry. Update comment https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... File components/reading_list/ios/reading_list_model_impl.cc (right): https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_model_impl.cc:155: // DCHECK(existing_entry->UpdateTime() < entry->UpdateTime()); Remove it? https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... File components/reading_list/ios/reading_list_store.cc (right): https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_store.cc:235: // The entry from sync is more recent. Update comment https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_store.cc:324: // The entry from sync is more recent. Update comment https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_store.cc:436: // The entry is more recent, so it can be submitted to sync. "The entry" here and below is not specific enough. Which one? Update comment :) https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_store.cc:444: if (lhs.url() != rhs.url() || lhs.title() != rhs.title() || Not sure the check on "url()" is necessary here. https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_store.cc:446: lhs.update_time_us() != rhs.update_time_us()) { Actually I don't understand this condition. If the update times are different on the two entries you will enter this block? And you tested it for inequality above. https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_store.cc:455: if (lhs.status() != rhs.status()) { Maybe add a comment for this?
High level comment: When receiving data, you must ACK it to the change_processor, even if you keep it unchanged. You should never receive an entry corresponding exactly to what you have, but in that case, you need to send it to the change processor anyway for acknowledgement.. https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... File components/reading_list/ios/reading_list_entry.cc (right): https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_entry.cc:332: #if !defined(NDEBUG) On 2016/12/06 17:02:22, gambard wrote: > What is the purpose of this? Can you add a comment? Done. https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... File components/reading_list/ios/reading_list_entry_unittest.cc (right): https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_entry_unittest.cc:319: // Tests that the merging of two ReadingListEntry. On 2016/12/06 17:02:22, gambard wrote: > Update comment Done. https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_entry_unittest.cc:343: // Tests that the merging of two ReadingListEntry. On 2016/12/06 17:02:22, gambard wrote: > Update comment Done. https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... File components/reading_list/ios/reading_list_model_impl.cc (right): https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_model_impl.cc:155: // DCHECK(existing_entry->UpdateTime() < entry->UpdateTime()); On 2016/12/06 17:02:22, gambard wrote: > Remove it? Done. https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... File components/reading_list/ios/reading_list_store.cc (right): https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_store.cc:235: // The entry from sync is more recent. On 2016/12/06 17:02:22, gambard wrote: > Update comment Done. https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_store.cc:324: // The entry from sync is more recent. On 2016/12/06 17:02:22, gambard wrote: > Update comment Done. https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_store.cc:436: // The entry is more recent, so it can be submitted to sync. On 2016/12/06 17:02:22, gambard wrote: > "The entry" here and below is not specific enough. Which one? > Update comment :) Done. https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_store.cc:444: if (lhs.url() != rhs.url() || lhs.title() != rhs.title() || On 2016/12/06 17:02:22, gambard wrote: > Not sure the check on "url()" is necessary here. As far as sync is concerned, the key is entry_id. So checking url may be useful. https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_store.cc:446: lhs.update_time_us() != rhs.update_time_us()) { On 2016/12/06 17:02:22, gambard wrote: > Actually I don't understand this condition. > If the update times are different on the two entries you will enter this block? > And you tested it for inequality above. This method ensures that you send correct data to the sync server. It does not know the rules we use to merge entries. If we want to change MergeEntryWith and break the strict order, this will catch it and avoid sending the data to sync and loop. https://codereview.chromium.org/2553143002/diff/20001/components/reading_list... components/reading_list/ios/reading_list_store.cc:455: if (lhs.status() != rhs.status()) { On 2016/12/06 17:02:22, gambard wrote: > Maybe add a comment for this? Done.
olivierrobin@chromium.org changed reviewers: + pavely@chromium.org
Hi pavely, I tried to add guards to avoid ping-pong between devices. Please take a look
lgtm https://codereview.chromium.org/2553143002/diff/40001/components/reading_list... File components/reading_list/ios/reading_list_entry.cc (right): https://codereview.chromium.org/2553143002/diff/40001/components/reading_list... components/reading_list/ios/reading_list_entry.cc:333: // Checks that the result entry respect the sync order. s/respect/respects/ https://codereview.chromium.org/2553143002/diff/40001/components/reading_list... File components/reading_list/ios/reading_list_store.h (right): https://codereview.chromium.org/2553143002/diff/40001/components/reading_list... components/reading_list/ios/reading_list_store.h:75: // Returns if entries respect a strict order for sync and if |rhs| can be Returns true if
https://codereview.chromium.org/2553143002/diff/40001/components/reading_list... File components/reading_list/ios/reading_list_entry.h (right): https://codereview.chromium.org/2553143002/diff/40001/components/reading_list... components/reading_list/ios/reading_list_entry.h:95: // If |other.UpdateTime()| >= |this.UpdateTime()|, all fields are copied from "all fields are copied" is misleading: you do not copy the state field (as explained in the following line). Can you merge the comments? Like: all fields except |state| are copied from |other|. The state is copied only if it is not UNSEEN. https://codereview.chromium.org/2553143002/diff/40001/components/reading_list... File components/reading_list/ios/reading_list_store.cc (right): https://codereview.chromium.org/2553143002/diff/40001/components/reading_list... components/reading_list/ios/reading_list_store.cc:444: lhs.update_time_us() != rhs.update_time_us()) { lhs.update_time_us() != rhs.update_time_us() will always be true and is not consistent with the comment below. https://codereview.chromium.org/2553143002/diff/40001/components/reading_list... components/reading_list/ios/reading_list_store.cc:454: // If the status is different and not UNSEEN, update_time should be Reword this. WDYT of: Update_time should be different if the status are different and |lhs| is not UNSEEN. https://codereview.chromium.org/2553143002/diff/40001/components/reading_list... components/reading_list/ios/reading_list_store.cc:459: DCHECK(lhs.SerializeAsString() == rhs.SerializeAsString()); Maybe return lhs.SerializeAsString() == rhs.SerializeAsString() instead of DCHECK? https://codereview.chromium.org/2553143002/diff/40001/components/reading_list... File components/reading_list/ios/reading_list_store.h (right): https://codereview.chromium.org/2553143002/diff/40001/components/reading_list... components/reading_list/ios/reading_list_store.h:75: // Returns if entries respect a strict order for sync and if |rhs| can be On 2016/12/07 08:25:36, jif wrote: > Returns true if Or "Returns whether". https://codereview.chromium.org/2553143002/diff/40001/components/reading_list... File components/reading_list/ios/reading_list_store_unittest.mm (right): https://codereview.chromium.org/2553143002/diff/40001/components/reading_list... components/reading_list/ios/reading_list_store_unittest.mm:276: syncer::SyncError error = reading_list_store_->ApplySyncChanges( Do you need the |error| variable?
Description was changed from ========== Create a strict order in ReadingListSpecifics To avoid sync ping-pong where two device disagree on the latest state, create a strict order for ReadingListSpecifice. Entries can only be submitted to sync in strictly increasing order. The order is defined by Primary key: update time Secondary key: status. BUG=666232 ========== to ========== Create a strict order in ReadingListSpecifics To avoid sync ping-pong where two device disagree on the latest state, create a strict order for ReadingListSpecifice. // Entries are in increasing order if all the fields respect increasing order. // - URL must be the same. // - title must verify rhs.title.compare(lhs.title) >= 0 // - rhs.creation_time_us >= lhs.creation_time_us // - rhs.update_time_us >= lhs.update_time_us // - if rhs.update_time_us > lhs.update_time_us, rhs.state can be anything. // - if rhs.update_time_us == lhs.update_time_us, rhs.state >= lhs.state in // the order UNSEEN, UNREAD, READ. BUG=666232 ==========
I change the order to be field by field https://codereview.chromium.org/2553143002/diff/40001/components/reading_list... File components/reading_list/ios/reading_list_entry.cc (right): https://codereview.chromium.org/2553143002/diff/40001/components/reading_list... components/reading_list/ios/reading_list_entry.cc:333: // Checks that the result entry respect the sync order. On 2016/12/07 08:25:36, jif wrote: > s/respect/respects/ Done.
lgtm with one comment https://codereview.chromium.org/2553143002/diff/60001/components/reading_list... File components/reading_list/ios/reading_list_store.cc (right): https://codereview.chromium.org/2553143002/diff/60001/components/reading_list... components/reading_list/ios/reading_list_store.cc:443: return false; Why not returning the condition directly?
lgtm https://codereview.chromium.org/2553143002/diff/60001/components/reading_list... File components/reading_list/ios/reading_list_entry.cc (right): https://codereview.chromium.org/2553143002/diff/60001/components/reading_list... components/reading_list/ios/reading_list_entry.cc:355: // Both states are likely the same, but if they are not, READ should wim. typo: wim => win. https://codereview.chromium.org/2553143002/diff/60001/components/reading_list... File components/reading_list/ios/reading_list_store.h (right): https://codereview.chromium.org/2553143002/diff/60001/components/reading_list... components/reading_list/ios/reading_list_store.h:81: // - title must verify rhs.title.compare(lhs.title) >= 0 I don't really understand why title is taken into account for comparison. What if in the future chrome allows to rename articles and user shortens title? Will it be rejected by other clients?
https://codereview.chromium.org/2553143002/diff/60001/components/reading_list... File components/reading_list/ios/reading_list_entry.cc (right): https://codereview.chromium.org/2553143002/diff/60001/components/reading_list... components/reading_list/ios/reading_list_entry.cc:355: // Both states are likely the same, but if they are not, READ should wim. On 2016/12/07 19:16:06, pavely wrote: > typo: wim => win. Done. https://codereview.chromium.org/2553143002/diff/60001/components/reading_list... File components/reading_list/ios/reading_list_store.h (right): https://codereview.chromium.org/2553143002/diff/60001/components/reading_list... components/reading_list/ios/reading_list_store.h:81: // - title must verify rhs.title.compare(lhs.title) >= 0 On 2016/12/07 19:16:06, pavely wrote: > I don't really understand why title is taken into account for comparison. What > if in the future chrome allows to rename articles and user shortens title? Will > it be rejected by other clients? At the moment, title cannot be changed, so the only conflict can appear if one device does have a title and the other does not. If we add the possibility to change the title, we will need to set a new field like "last_title_change_timestamp" to handle conflicts.
The CQ bit was checked by olivierrobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org, gambard@chromium.org, pavely@chromium.org Link to the patchset: https://codereview.chromium.org/2553143002/#ps80001 (title: "title")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for components/reading_list/ios/reading_list_entry.cc:
While running git apply --index -p1;
error: patch failed: components/reading_list/ios/reading_list_entry.cc:327
error: components/reading_list/ios/reading_list_entry.cc: patch does not apply
Patch: components/reading_list/ios/reading_list_entry.cc
Index: components/reading_list/ios/reading_list_entry.cc
diff --git a/components/reading_list/ios/reading_list_entry.cc
b/components/reading_list/ios/reading_list_entry.cc
index
283d78a924fb9a66310822bfcc40c330e0e00458..3e5b974fac83424afc6f90fda448b301248e519d
100644
--- a/components/reading_list/ios/reading_list_entry.cc
+++ b/components/reading_list/ios/reading_list_entry.cc
@@ -8,6 +8,7 @@
#include "base/memory/ptr_util.h"
#include "components/reading_list/ios/offline_url_utils.h"
#include "components/reading_list/ios/proto/reading_list.pb.h"
+#include "components/reading_list/ios/reading_list_store.h"
#include "components/sync/protocol/reading_list_specifics.pb.h"
#include "net/base/backoff_entry_serializer.h"
@@ -327,11 +328,45 @@ std::unique_ptr<ReadingListEntry>
ReadingListEntry::FromReadingListSpecifics(
WAITING, base::FilePath(), 0, nullptr));
}
-void ReadingListEntry::MergeLocalStateFrom(ReadingListEntry& other) {
- distilled_path_ = std::move(other.distilled_path_);
- distilled_state_ = std::move(other.distilled_state_);
- backoff_ = std::move(other.backoff_);
- failed_download_counter_ = std::move(other.failed_download_counter_);
+void ReadingListEntry::MergeWithEntry(const ReadingListEntry& other) {
+#if !defined(NDEBUG)
+ // Checks that the result entry respects the sync order.
+ std::unique_ptr<sync_pb::ReadingListSpecifics> old_this_pb(
+ AsReadingListSpecifics());
+ std::unique_ptr<sync_pb::ReadingListSpecifics> other_pb(
+ other.AsReadingListSpecifics());
+#endif
+ DCHECK(url_ == other.url_);
+ if (title_.compare(other.title_) < 0) {
+ // Take the last in alphabetical order or the longer one.
+ // This ensure empty string is replaced.
+ title_ = std::move(other.title_);
+ }
+ if (creation_time_us_ < other.creation_time_us_) {
+ creation_time_us_ = std::move(other.creation_time_us_);
+ }
+ if (state_ == UNSEEN) {
+ state_ = std::move(other.state_);
+ } else if (other.state_ != UNSEEN) {
+ // Both are not UNSEEN, take the newer one.
+ if (update_time_us_ < other.update_time_us_) {
+ state_ = std::move(other.state_);
+ } else if (update_time_us_ == other.update_time_us_) {
+ // Both states are likely the same, but if they are not, READ should win.
+ if (other.state_ == READ) {
+ state_ = std::move(other.state_);
+ }
+ }
+ }
+ if (update_time_us_ < other.update_time_us_) {
+ update_time_us_ = std::move(other.update_time_us_);
+ }
+#if !defined(NDEBUG)
+ std::unique_ptr<sync_pb::ReadingListSpecifics> new_this_pb(
+ AsReadingListSpecifics());
+ DCHECK(ReadingListStore::CompareEntriesForSync(*old_this_pb, *new_this_pb));
+ DCHECK(ReadingListStore::CompareEntriesForSync(*other_pb, *new_this_pb));
+#endif
}
std::unique_ptr<reading_list::ReadingListLocal>
@@ -423,8 +458,3 @@ ReadingListEntry::AsReadingListSpecifics() const {
return pb_entry;
}
-
-bool ReadingListEntry::CompareEntryUpdateTime(const ReadingListEntry& lhs,
- const ReadingListEntry& rhs) {
- return lhs.UpdateTime() > rhs.UpdateTime();
-}
The CQ bit was checked by olivierrobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gambard@chromium.org, jif@chromium.org, pavely@chromium.org Link to the patchset: https://codereview.chromium.org/2553143002/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2553143002/diff/60001/components/reading_list... File components/reading_list/ios/reading_list_store.cc (right): https://codereview.chromium.org/2553143002/diff/60001/components/reading_list... components/reading_list/ios/reading_list_store.cc:443: return false; On 2016/12/07 13:49:54, gambard wrote: > Why not returning the condition directly? Because we may add other cases after
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481210381390370,
"parent_rev": "3f9e2b70e480f25836db8b3d362db411c3ec3f4d", "commit_rev":
"686679107a162003f58e29d7877f0b4c78deeca2"}
Message was sent while issue was closed.
Description was changed from ========== Create a strict order in ReadingListSpecifics To avoid sync ping-pong where two device disagree on the latest state, create a strict order for ReadingListSpecifice. // Entries are in increasing order if all the fields respect increasing order. // - URL must be the same. // - title must verify rhs.title.compare(lhs.title) >= 0 // - rhs.creation_time_us >= lhs.creation_time_us // - rhs.update_time_us >= lhs.update_time_us // - if rhs.update_time_us > lhs.update_time_us, rhs.state can be anything. // - if rhs.update_time_us == lhs.update_time_us, rhs.state >= lhs.state in // the order UNSEEN, UNREAD, READ. BUG=666232 ========== to ========== Create a strict order in ReadingListSpecifics To avoid sync ping-pong where two device disagree on the latest state, create a strict order for ReadingListSpecifice. // Entries are in increasing order if all the fields respect increasing order. // - URL must be the same. // - title must verify rhs.title.compare(lhs.title) >= 0 // - rhs.creation_time_us >= lhs.creation_time_us // - rhs.update_time_us >= lhs.update_time_us // - if rhs.update_time_us > lhs.update_time_us, rhs.state can be anything. // - if rhs.update_time_us == lhs.update_time_us, rhs.state >= lhs.state in // the order UNSEEN, UNREAD, READ. BUG=666232 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Create a strict order in ReadingListSpecifics To avoid sync ping-pong where two device disagree on the latest state, create a strict order for ReadingListSpecifice. // Entries are in increasing order if all the fields respect increasing order. // - URL must be the same. // - title must verify rhs.title.compare(lhs.title) >= 0 // - rhs.creation_time_us >= lhs.creation_time_us // - rhs.update_time_us >= lhs.update_time_us // - if rhs.update_time_us > lhs.update_time_us, rhs.state can be anything. // - if rhs.update_time_us == lhs.update_time_us, rhs.state >= lhs.state in // the order UNSEEN, UNREAD, READ. BUG=666232 ========== to ========== Create a strict order in ReadingListSpecifics To avoid sync ping-pong where two device disagree on the latest state, create a strict order for ReadingListSpecifice. // Entries are in increasing order if all the fields respect increasing order. // - URL must be the same. // - title must verify rhs.title.compare(lhs.title) >= 0 // - rhs.creation_time_us >= lhs.creation_time_us // - rhs.update_time_us >= lhs.update_time_us // - if rhs.update_time_us > lhs.update_time_us, rhs.state can be anything. // - if rhs.update_time_us == lhs.update_time_us, rhs.state >= lhs.state in // the order UNSEEN, UNREAD, READ. BUG=666232 Committed: https://crrev.com/f623a8a02f98adfb943529b9b25a6824bc62a97d Cr-Commit-Position: refs/heads/master@{#437251} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f623a8a02f98adfb943529b9b25a6824bc62a97d Cr-Commit-Position: refs/heads/master@{#437251} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
