Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RCORE-2011: New parameter 'progress_estimate' for sync progress notification #7124

Merged
merged 24 commits into from
Mar 22, 2024
Merged

RCORE-2011: New parameter 'progress_estimate' for sync progress notification #7124

merged 24 commits into from
Mar 22, 2024

Conversation

kiburtse
Copy link
Contributor

@kiburtse kiburtse commented Nov 9, 2023 •
edited
Loading

What, How & Why?

  • This PR introduces new progress estimate (value from 0.0 to 1.0) in existing progress notification's api
  • New field is added to SyncSession::ProgressNotifierCallback which is also used in c-api and in AsycOpenTask
  • Newly exposed field is supposed to provide current progress of synchronization for upload/download until completion
  • Estimate for download with flexible sync configuration is provided by the server-side implementation, for that the new protocol version is introduced
  • For partition based sync and upload progress everything is calculated in sync client code
  • Provided estimate is expected to be a full replacement to any custom implementation based on existing transferred/transferable fields with subsequent deprecation and deletion of these callback parameters

Fixes #7450

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed.

@coveralls-official Coveralls Official
Copy link

coveralls-official bot commented Nov 9, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build kirill.burtsev_158

Details

  • 765 of 784 (97.58%) changed or added relevant lines in 11 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on kb/sync_progress_estimate_api at 91.874%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/realm/sync/client.cpp 141 145 97.24%
src/realm/object-store/c_api/sync.cpp 0 6 0.0%
src/realm/sync/noinst/protocol_codec.hpp 40 49 81.63%
Totals Coverage Status
Change from base Build 2164: 91.9%
Covered Lines: 243606
Relevant Lines: 265152
💛 - Coveralls

@kiburtse kiburtse force-pushed the kb/sync_progress_estimate_api branch from 98be317 to 88756b3 Compare November 14, 2023 13:02
@clementetb clementetb mentioned this pull request Nov 16, 2023
Merged
@papafe papafe mentioned this pull request Dec 28, 2023
Closed
@kiburtse
* Add new field 'progress_estimate'
d53c56d 
* Precompute it for existing parameters for initial crud behaviour
* Adjust existing tests
* Add needed spec definitions
@kiburtse
* Add parsing support to protocol codec for repurposed progress estim…
6b8aaf4 
…ate field

* Refactor to forward parsed parameters from download message to session as is
@kiburtse
Add object-store based test for progress notifications
9df7df8
@kiburtse
Remove noop progress invocation in object-store api and simplify inte…
44155a9 
…rnals
@kiburtse kiburtse force-pushed the kb/sync_progress_estimate_api branch from 88756b3 to 9a55639 Compare February 2, 2024 20:21
@cla-bot cla-bot bot added the cla: yes label Feb 2, 2024
@kiburtse kiburtse force-pushed the kb/sync_progress_estimate_api branch from 9a55639 to e5a654e Compare February 8, 2024 22:35
jbreams
jbreams reviewed Feb 15, 2024
View reviewed changes
Copy link
Contributor

@jbreams jbreams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some initial comments. I don't quite get what the transient downloaded bytes tracking does or how it's different from the downloadable bytes we already track in history.

src/realm/sync/client.cpp Outdated Show resolved Hide resolved
src/realm/sync/noinst/client_impl_base.hpp Outdated Show resolved Hide resolved
src/realm/sync/client.cpp Show resolved Hide resolved
src/realm/sync/noinst/protocol_codec.hpp Outdated Show resolved Hide resolved
src/realm/sync/client.cpp Outdated Show resolved Hide resolved
src/realm/sync/client.cpp Outdated Show resolved Hide resolved
src/realm/sync/client.cpp Outdated Show resolved Hide resolved
@kiburtse kiburtse force-pushed the kb/sync_progress_estimate_api branch from 87d946d to 7b47194 Compare March 6, 2024 19:10
@kiburtse kiburtse marked this pull request as ready for review March 6, 2024 22:17
@kiburtse
Copy link
Contributor Author

kiburtse commented Mar 6, 2024

sync tests need a minor fix to make ci happy, other then that object-store tests should pass and the pr is almost done

@kiburtse kiburtse force-pushed the kb/sync_progress_estimate_api branch from 7b47194 to ad7df00 Compare March 7, 2024 14:44
@kiburtse
Copy link
Contributor Author

kiburtse commented Mar 7, 2024

sync tests need a minor fix to make ci happy, other then that object-store tests should pass and the pr is almost done

fixed, ci should be green now.

@danieltabacaru @jbreams please, take a look. All commits here a logically separated, it could be easier to review them one by one. The only missing thing is final master merge and changelog. I'll do it after a few ci runs.

@kiburtse
Copy link
Contributor Author

Maybe we should print the progress estimate with a fixed number of decimals: Realm.Sync.Client.Session - Connection[1]: Session[1]: Progress handler called, downloaded = 75, downloadable = 75, estimate = 1, uploaded = 78, uploadable = 5365424, estimate = 1.45375e-05, snapshot version = 17

(from the server spec: Download Progress Percentage will be a percentage representation of how much progress has been made so far, of type float, ranging from 0 to 1 with up to 4 points of precision.)

There also seems to be a division problem: Realm.Sync.Client.Session - Connection[1]: Session[2]: Progress handler called, downloaded = 5365700, downloadable = 5365700, estimate = 0.9999, uploaded = 78, uploadable = 78, estimate = 1, snapshot version = 18 The estimate should be 1.0 in this case.

Fixed log entries with the precision agreed on the scope to 4 digits

0.9999 was actually the corner case bug hidden by the workaround for the other issue (there is still fixme about this #7452)

@danieltabacaru @jbreams i believe i've addressed all opened threads, please, take a look again.

jbreams
jbreams reviewed Mar 19, 2024
View reviewed changes
src/realm/object-store/sync/sync_session.cpp Show resolved Hide resolved
src/realm/object-store/sync/sync_session.hpp Outdated Show resolved Hide resolved
src/realm/sync/client.cpp Show resolved Hide resolved
src/realm/sync/client.cpp Outdated Show resolved Hide resolved
src/realm/sync/client.cpp Outdated Show resolved Hide resolved
src/realm/sync/client.cpp Outdated Show resolved Hide resolved
test/object-store/sync/session/progress_notifications.cpp Show resolved Hide resolved
kiburtse added 2 commits March 19, 2024 22:48
@kiburtse
Remove some leftovers and fix typos
4207c39
@kiburtse
Add comment for the effect of progress estimate value calculation and
934cb42 
reset to 0 from 1 after batch sync or when new data is added during
active sync
danieltabacaru
danieltabacaru reviewed Mar 21, 2024
View reviewed changes
test/tsan.suppress Outdated Show resolved Hide resolved
@kiburtse kiburtse merged commit a2034d1 into master Mar 22, 2024
39 checks passed
@kiburtse kiburtse deleted the kb/sync_progress_estimate_api branch March 22, 2024 19:32
kraenhansen added a commit that referenced this pull request Apr 5, 2024
@kraenhansen
Follow-up to make #7124 target iPhone 11 (#7555)
ad44ce9 
* Avoid using features newer than iPhone 11 in client_impl_base.cpp

* Adding a note in the changelog
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rohitn212 rohitn212 rohitn212 left review comments

@papafe papafe papafe left review comments

@sudssm sudssm sudssm left review comments

@nirinchev nirinchev nirinchev left review comments

@kneth kneth kneth left review comments

@danieltabacaru danieltabacaru danieltabacaru approved these changes

@jbreams jbreams jbreams approved these changes

Assignees

@kiburtse kiburtse

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce sync progress estimate to the progress notification callback
8 participants
@kiburtse @danieltabacaru @jbreams @kneth @nirinchev @sudssm @papafe @rohitn212