-
Notifications
You must be signed in to change notification settings - Fork 868
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
Trigger sync compaction for bookmarks every week #3749
Conversation
bbd6997
to
5061946
Compare
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.
PR looks good
Verified it works with STR of: I. Making bookmarks with lots of updates
paste and run it several times II. Initiating compact operation
III. Verifying compact was correct
|
extensions::api::brave_sync::OnSendCompactSyncCategory::Create( | ||
category_name).release()); | ||
std::unique_ptr<Event> event( | ||
new Event(extensions::events::FOR_TEST, |
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.
FOR_TEST
?
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.
it's been this way since we have BraveSyncEventRouter
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.
it is just a events::HistogramValue and I can't find a suitable value from the existing list. Should we extend the enum? Please note that brave sync will eventually go away and migrated to native implementation
base::Time::Now() - last_compact_time > | ||
base::TimeDelta::FromDays(kCompactPeriodInDays)) { | ||
brave_sync_client_->SendCompactSyncCategory(kBookmarks); | ||
brave_sync_prefs_->SetLastCompactTime(base::Time::Now()); |
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.
shouldn't we wait to do this until the compaction is complete?
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.
You mean, when the objects are really deleted on s3?
The compaction API is designed to have no callback, if we want callback when the object is really deleted, we would need extra listObject query to check. The deletion is not instant, it takes approximately 1 minute
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.
Also, the compaction could take a significant long time to pull down all the record if the recordset is huge
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.
addressed in bb0c6c3
which requires deps update when brave/sync#355 is merged
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.
++
5061946
to
e07c36b
Compare
56d2773
to
aef2165
Compare
bb0c6c3
to
0e2d34f
Compare
bda5e45
to
527100a
Compare
527100a
to
f1b7039
Compare
f1b7039
to
8d915f6
Compare
fix brave/brave-browser#6552
Submitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
#3749 (comment) can be referenced to generate update records easily
Update
chrome://inspect/#extensions
and inspectBrave Sync
on device Acompaction deletes
... on console and wait for extra 3 minschrome://inspect/#extensions
and inspectBrave Sync
on device Cgot 2 decrypted records in BOOKMARKS after 0
on consoleDelete
chrome://inspect/#extensions
and inspectBrave Sync
on device Acompaction deletes
... on console and wait for extra 3 minschrome://inspect/#extensions
and inspectBrave Sync
on device Cgot 1 decrypted records in BOOKMARKS after 0
on consoleReviewer Checklist:
After-merge Checklist:
changes has landed on.