Skip to content

/ sync

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

Revert PR#313 (Merge by id and action) #317

Merged
merged 2 commits into from May 30, 2019
Merged

Revert PR#313 (Merge by id and action) #317

merged 2 commits into from May 30, 2019

Conversation

@AlexeyBarabash
Copy link
Contributor

AlexeyBarabash commented May 30, 2019

This is revert of #313 .

It causes problem with this STR

1. Create chain Android1 <=> Android2
2. Create bookmarks on Android1:
	wikipedia.com
	espn.com
	amazon.com
3. Wait untill bookmarks appear on Android2
4. Delete wikipedia.com from Android1
5. Wait untill bookmark will be deleted on Android2
6. Connect Core to chain
7.
Actual bookmarks:
	wikipedia.com
	espn.com
	amazon.com
Expected bookmarks:
	espn.com
	amazon.com

With these STR brave-core logs show

get-existing-objects:
	CREATE wikipedia.com
	CREATE espn.com
	CREATE amazon.com
	DELETE wikipedia.com
resolve: all pairs with null

[1207:1207:0530/132313.432076:INFO:CONSOLE(1)] "Ignoring DELETE of object 93,212,16,154,213,179,21,41,212,0,57,255,12,41,120,72.", source: chrome-extension://nomlkjnggnifocmealianaaiobmebgil/extension/brave-sync/bundles/bundle.js (1)

resolved-sync-records:
	CREATE wikipedia.com
	CREATE espn.com
	CREATE amazon.com

So it left first CREATE and second DELETE, but then ignored the second DELETE because existing_object==null

PR is not required when there is a fix of do not send records if there is no chain brave/brave-core@6f96849
@AlexeyBarabash AlexeyBarabash added the bug label May 30, 2019
@AlexeyBarabash AlexeyBarabash self-assigned this May 30, 2019
@darkdh
Copy link
Member

darkdh commented May 30, 2019

please separate new test case from revert commit
@AlexeyBarabash
Revert PR#313 (except 89d5f8e)
0e1d127
@AlexeyBarabash
Added test: Create + Delete of non-existing object should be resolve …
Loading status checks…
4d34c1e 
…to none
@AlexeyBarabash AlexeyBarabash force-pushed the revert_separate_create branch from 38f459f to 4d34c1e May 30, 2019
@AlexeyBarabash
Copy link
Contributor Author

AlexeyBarabash commented May 30, 2019

Made a separate commits. CI succeeded.
@darkdh
darkdh approved these changes May 30, 2019
View changes
@AlexeyBarabash AlexeyBarabash merged commit 066c4da into staging May 30, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
AlexeyBarabash added a commit that referenced this pull request May 30, 2019
@AlexeyBarabash
Merge pull request #317 from brave/revert_separate_create
Loading status checks…
75d463e 
Revert PR#313 (Merge by id and action)
@AlexeyBarabash AlexeyBarabash mentioned this pull request May 30, 2019
Merged
@darkdh darkdh deleted the revert_separate_create branch May 30, 2019
darkdh added a commit that referenced this pull request May 30, 2019
@darkdh
Merge pull request #318 from brave/revert_separate_create_master
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
7581f5f 
Merge pull request #317 from brave/revert_separate_create
AlexeyBarabash added a commit that referenced this pull request Jun 21, 2019
@AlexeyBarabash
Merge pull request #317 from brave/revert_separate_create
2966204 
Revert PR#313 (Merge by id and action)
@AlexeyBarabash AlexeyBarabash mentioned this pull request Oct 15, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darkdh darkdh

@diracdeltas diracdeltas

@SergeyZhukovsky SergeyZhukovsky

Assignees

@AlexeyBarabash AlexeyBarabash

Labels
bug
Projects
Sync - All platforms
  
Awaiting triage
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
@AlexeyBarabash @darkdh
You can’t perform that action at this time.