Chromium Code Reviews Chromium Code Reviews
Issue
1136953013:
Sync: Change Local IDs to GUID based to avoid ID collision (Closed)
DescriptionSync: Change Local IDs to GUID based to avoid ID collision
This changes implementation of Directory::NextId() to no longer depend on
"next_id" counter stored in Sync DB. Even though there was a special precaution
to avoid potential collision of local IDs by fast forwarding next_id by a large
number on every save there are still a couple scenarios leading the the collision
between a local ID generated on a client and originator_client_item_id coming
from the server (which is basically a local ID of an item previously committed
to the server.
The new implementation of Directory::NextId() is based on base::GenerateGUID.
The following changes accompany the transition to the new ID format:
1) The next_id field is no longer needed in Directory::PersistedKernelInfo
2) The code that persists next_id is no longer needed.
3) Forcing the directory to save by marking it with KERNEL_SHARE_INFO_DIRTY
flag right after loading it is no longer necessary. That was done primarily
to increment next_id by a large value as a way to attempt this problem.
4) Fixing #3 revealed a bug in Directory::IncrementTransactionVersion that
doesn't mark the directory with KERNEL_SHARE_INFO_DIRTY when incrementing
a datatype's transaction version. That caused a number of bookmark tests
to fail due to the transaction version check. Fixed it.
5) Changing local IDs to GUIDs had a subtle effect on some tests. For the
types that don't support ordering, sorting of entries in ParentChildIndex
defaults to ID comparison. Since generation of local IDs base on next_id
followed a predictable pattern, some tests relied on order of items returned
by ParentChildIndex. Switching to GUIDs disrupted the expected order
(of entries that are unordered in their nature). To preserve the existing
ordering pattern for unordered entries, I changed ParentChildIndex to
default to META_HANDLE comparison.
6) Another test that failed due to the GUID transition was
SyncableDirectoryTest.BookmarkTagTest. The test relied on a predefined,
repeatable local ID which was no longer the case with the GUID generation.
I found the test to be redundant. There is another tests that verifies
the same functionality - unique bookmark tag generation algorithm.
For now the change leaves next_id in the database even though the field is
no longer used. I should probably file another bug to track its removal.
BUG=362467
Committed: https://crrev.com/51f97d05f1e865f6dee84f568ee44cb0640a08fc
Cr-Commit-Position: refs/heads/master@{#329946}
Patch Set 1 #Patch Set 2 : Test #Patch Set 3 : Fixed test failures #Patch Set 4 : Fixed setting KERNEL_SHARE_INFO_DIRTY when incrementing transaction version #
Total comments: 6
Patch Set 5 : Changed comments for Directory::NextId() #
Messages
Total messages: 9 (3 generated)
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
