Update captured_transferrable when downloadable bytes value changes. #4989
Conversation
leemaguire
force-pushed
the
lm/sync-progress
branch
3 times, most recently
from
82340cb to
962e4d5
Compare
| // We need to account for when server side compaction runs. This will change the | ||
| // value of `current_progress.downloadable` so any math that checks sync progress will | ||
| // then be invalid because of the old value. | ||
| if (is_download && (*captured_transferrable != current_progress.downloadable)) { |
There was a problem hiding this comment.
so i think this is a behavioral change that i'm not sure is what the user is going to want/expect. the transferrable number that used to be a fixed value that you worked up to is now going to be constantly decreasing. this is probably the correct behavior, but is definitely different from how this has worked in the past. and maybe we should just get rid of the captured_transferrable variable and report that value to the user as the estimated remaining downloadable bytes?
There was a problem hiding this comment.
I have kept captured_transferrable because totally removing would change the behaviour.
leemaguire
force-pushed
the
lm/sync-progress
branch
from
ee0110e to
7025670
Compare
| @@ -1125,11 +1125,23 @@ std::function<void()> SyncProgressNotifier::NotifierPackage::create_invocation(P | |||
| transferrable = is_download ? current_progress.downloadable : current_progress.uploadable; | |||
| } | |||
| else if (captured_transferrable) { | |||
| if (is_download && (current_progress.downloadable == 0)) { | |||
There was a problem hiding this comment.
Without this we will never be able to pass 0 downloadable bytes to the SDK
There was a problem hiding this comment.
Right, but do we want to pass 0 to the SDK? We didn't used to. We just want this to expire when downloadable reaches zero.
There was a problem hiding this comment.
yes because that is the only thing we can do that does not involve removing captured_transferrable
There was a problem hiding this comment.
If this was to just expire the SDK will not be notified of it.
author Jason Flax <[email protected]> 1632927798 +0200 committer Jason Flax <[email protected]> 1635527380 +0100 parent 0a0d02a author Jason Flax <[email protected]> 1632927798 +0200 committer Jason Flax <[email protected]> 1635527271 +0100 Replace all references of StringView/BasicStringView with std::string_view/std::basic_string_view Address @jbreams comments revert query_flex fix tests Revert query_flex.hpp compile on windows compile on windows Update captured_transferrable when downloadable bytes value changes. (#4989) Prepare release Remove unused variables Updated release notes Use originating realm schema for frozen realm (#4965) Co-authored-by: Ferdinando Papale <> Collapase ConstTableView and TableView Some further cleanup in TableView Update CHANGELOG.md re-remove files
|
This appears to have broken streaming download notifiers. Adding a streaming notifier before there is anything to download now results in it just never firing. |
|
I don't really understand what this PR was even trying to do. The PR description and changelog message don't have any obvious connection, and the changes to the tests make this look like just a change in behavior that breaks things rather than a bug fix. |
What, How & Why?
The sync server can perform compaction on collections during a sync session. This means that the downloadable bytes size can change during sync progress. This PR updates the progress notifier to handle the server side compaction logic.
☑️ ToDos