weixin_39532754
weixin_39532754
2020-11-30 10:17

Update logicalClock of tombstoned and previous version entities

This ensures all synchronized entities that are updated/deleted are added to the upcoming RevisionRecord so their changes are pushed to the remote.

Fix #446

该提问来源于开源项目:carekit-apple/CareKit

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答

10条回答

  • weixin_39532754 weixin_39532754 5月前

    It looks like the 2 added tests could have possibly been added to TestStoreBuildRevisions instead of TestStoreSync since the problem was populating the RevisionRecord to be synced to the remote. Since these tests use syncAndWait along with testing if the remote gets the correct revisions, are they okay in TestStoreSync? I see the other way would be to use store.computeRevision() instead of DummyEndpoint2

    点赞 评论 复制链接分享
  • weixin_39532754 weixin_39532754 5月前

    I deleted my previous post as it looks like I was wrong about the problem being fixed. I have checked this merge locally and it works.

    I have an example using the WWDC20 OCKSample with the AppleWatch code that should show a delete synchronizing across watch and phone, that I'll post in few...

    点赞 评论 复制链接分享
  • weixin_39532754 weixin_39532754 5月前

    Here's the latest OCKSample app using the this PRs fixes. It works a little different than the WWDC video as the watch and the iOS are both using ParseCareKit to sync through the cloud remote (parse-hipaa running locally on your computer via docker). The tombstoning of tasks from watch to iPhone works the same as the video (the watch pings (through WCSession) the iOS device to sync with the parse-hipaa remote whenever it successfully pushes to the remote). ~Changes from the iPhone to watch can't be seen unless you exit the watch app and comeback (for some reason I can't get the iPhone to talk back to the watch via WCSession. Not sure if it's the beta I'm using or if I setup something wrong with WCSession).~

    The version of the sample app using the current CareKit branch is here. Visually, this looks the same as deletes will disappear as I mentioned earlier, but the tombstoning of the original outcome on the remote doesn't happen which can be seen on the parse-hipaa dashboard.

    ~I couldn't get the watch and iPhone as remotes to each other to work properly, I think because WCSession issue I mentioned earlier)~

    Update: I suspect this is down the line, but adding support for multiple remote will be useful. Specifically with this watchOS case as there are times when a watch and phone are thought to be connected to the Internet, but for some reason don't sync. Allowing them to sync with each other and with a cloud remote could solve the problem. Since the whole knowledge vector is being passed, it seems like could work. From the way it looks like the coordinator works, maybe it would use something similar?

    点赞 评论 复制链接分享
  • weixin_39581972 weixin_39581972 5月前

    Thanks for this PR Corey! We'll go over it in more detail soon, but at first blush it looks promising.

    It works a little different than the WWDC video as the watch and the iOS are both using ParseCareKit to sync through the cloud remote

    Your way is actually better than the way we demonstrated in our WWDC session in some respects. Apple Watch apps can be stand alone apps, or the phone might be out of range, so communicating directly with the server is typically a more robust approach.

    I suspect this is down the line, but adding support for multiple remote will be useful. Specifically with this watchOS case as there are times when a watch and phone are thought to be connected to the Internet, but for some reason don't sync. Allowing them to sync with each other and with a cloud remote could solve the problem. Since the whole knowledge vector is being passed, it seems like could work. From the way it looks like the coordinator works, maybe it would use something similar?

    That's an interesting idea!

    点赞 评论 复制链接分享
  • weixin_39581972 weixin_39581972 5月前

    Thanks for all the time you've put into this one Corey. If you can get the build passing again, I think this one is ready to merge!

    点赞 评论 复制链接分享
  • weixin_39532754 weixin_39532754 5月前

    -apple all of these tests pass locally. I think the random failures have a little to with Travis (slower resources) and Xcode 12 beta (I’m going to take a guess at what’s happening, though I haven’t looked into proof to verify what I’m about to say...).

    For Xcode 12 beta, when running the unit tests locally on MacBook Pro, it “looks” like the unit tests are executed asynchronously, not synchronously (the tests themselves, not the fetching/saving, etc to the CareKitStore.) I say this because when I run a test, I can see each test quickly spin like they are being executed for a second and this occurs in a round robin way until all of the tests are complete. I’m not sure if Xcode 11 operated in the async manner because I don’t remember seeing the round-robin spinning on each test. Xcode 11 “looked” like it executed tests synchronously. Since the CareKitStore has the largest amount of tests (390) and CareKitTests has the second largest (128), sometimes these tests timeout when running async . When you rerun they pass. If my assumptions above are correct, it may be worthwhile to extend the times to wait for async behavior above 10 seconds (maybe 15 or 20?). This should help the tests complete on slower systems like Travis and maybe even GitHubActions

    点赞 评论 复制链接分享
  • weixin_39581972 weixin_39581972 5月前

    That's a pretty fair guess I think. Maybe we can bump that up to like 30 seconds.

    点赞 评论 复制链接分享
  • weixin_39532754 weixin_39532754 5月前

    -apple the TestHealthKitStoreTasks tests looks to be finicky on travis. The latest Xcode travis has is 12 beta 2. I don't see any issues on beta 4 locally. I believe GitHub actions is on beta 4 and they seem to upgrade whenever the latest is released.

    点赞 评论 复制链接分享
  • weixin_39581972 weixin_39581972 5月前

    I pushed a few small changes to match the code style with conventions we follow elsewhere in the codebase. If you're okay with the changes, I'll merge this.

    点赞 评论 复制链接分享
  • weixin_39532754 weixin_39532754 5月前

    Looks good to me

    点赞 评论 复制链接分享

相关推荐