RNET-1083: Add support for progress estimate on progress notifications #3479

papafe · 2023-11-15T13:33:06Z

This PR:

Adds ProgressEstimate to SyncProgress. It's a float value between 0.0 and 1.0 representing the estimate of the current progress.
Deprecates TransferableBytes and TransferredBytes in SyncProgress as these values are not accurate (especially for flexible sync).
Enable using GetProgressObservable with flexible sync.

Fixes #3478

TODO

Changelog entry
Tests (if applicable)

papafe · 2023-11-15T13:34:27Z

This is just a draft PR in case we have some discussing to do. It's also built on top of a PR in core.

papafe · 2023-11-17T11:11:07Z

I'm going to park this for now, as the core implementation is not yet finished, and we can't write reliable tests without it.

# Conflicts: # wrappers/realm-core

nirinchev

Looks good to me - I'd just prefer that we clean up the obsolete stuff in this PR already rather than merge it and have a follow-up PR that removes all the obsolete things as we prepare for 12.0

Realm/Realm/Sync/ProgressNotifications/SyncProgress.cs

papafe · 2024-02-08T15:33:05Z

Completely removed obsolete

nirinchev

Looks good, though it'd be nice to add a test that is a bit more strict about the notifications. Particularly, we should have a test for the download direction, which we don't seem to at the moment.

Realm/Realm/Sync/ProgressNotifications/SyncProgress.cs

Tests/Realm.Tests/Sync/SessionTests.cs

coveralls-official · 2024-02-08T16:50:58Z

Pull Request Test Coverage Report for Build 8630691641

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

6 of 6 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.007%) to 81.021%

Totals
Change from base Build 8598129525:	-0.007%
Covered Lines:	6790
Relevant Lines:	8232

💛 - Coveralls

papafe · 2024-02-16T12:33:16Z

This is going to get parked until the core PR on which it depends it has been merged.
What we need to do then:

Add tests
Remove bytes from progress notification

# Conflicts: # wrappers/realm-core # wrappers/src/async_open_task_cs.cpp # wrappers/src/sync_session_cs.cpp

papafe · 2024-04-09T13:45:18Z

@nirinchev this is mergeable for me (I can fix the lint warning after your review 😁 )

kiburtse · 2024-04-10T07:30:15Z

Realm/Realm/Sync/ProgressNotifications/ProgressMode.cs

-        /// The callback will, upon registration, store the total number of bytes to be transferred. When invoked, it will
-        /// always report the most up-to-date number of transferable bytes out of that original number of transferable bytes.
-        /// When the number of transferred bytes reaches or exceeds the number of transferable bytes, the callback will
+        /// The callback will be active until the current transferable bytes are transferred. When invoked, it will always report the


it's a bit weird now to refer to transferred/transferable after these fields were removed. Also, it makes no sense any more for download direction. Probably, a good idea is to rephrase it in a way that it refers to snapshot committed version for upload on callback registration (that is essentially transferable bytes), and for download it is whatever server decides for the outgoing or next batch of changesets to catch up the client to the latest version.

@kiburtse I agree with you, it's kinda imprecise.
Would you think the following will make sense?

The callback will be active: - For uploads, until all the unsynced changesets available at the moment of the registration of the callback are sent to the server. - For downloads, until the client catches up to the current server data (available at the moment of registration) or the next batch of changesets sent from the server.

@nirinchev I'll fix the other two small things. I suppose this is the latest thing to agree on. Do you have input on this?

@papafe looks good to me. The only additional consideration here is that this notion of "next batch of changesets" is very sync client implementation specific. I'm not sure how familiar sdk users are with this. Maybe it also raises the expectations a bit too much since in steady state this is not the case for sure because every download message is its own batch, and in general it's not defined as of now how it should behave with flexible sync and subscriptions change.

Makes sense. I think I will change "changeset" to "data" to keep on the more generic side

nirinchev

Looks good - there's a few lint warnings you could drive-by fix.

Realm/Realm/Handles/SessionHandle.cs

nirinchev · 2024-04-10T09:39:31Z

Realm/Realm/Handles/SessionHandle.cs

        {
            var token = (ProgressNotificationToken?)GCHandle.FromIntPtr(tokenPtr).Target;
-            token?.Notify(transferredBytes, transferableBytes);
+            // This is used to provide a reasonable progress estimate until the core work is done
+            double managedProgressEstimate = transferableBytes > 0.0 ? transferredBytes / transferableBytes : 0.0;


If transferable is 0, shouldn't progress be 1?

Added stub for progress notifications

2cd0022

github-actions bot assigned papafe Nov 15, 2023

Added test for flexible sync

3a98f21

papafe added 2 commits February 8, 2024 13:19

Merge branch 'main' into fp/progress-notifications

d666645

# Conflicts: # wrappers/realm-core

Using correct core branch

02417ef

cla-bot bot added the cla: yes label Feb 8, 2024

papafe added 3 commits February 8, 2024 15:28

Fixed test

e7a3f7b

Small fix

728f148

Small fixes

febeb00

papafe marked this pull request as ready for review February 8, 2024 14:53

papafe added 2 commits February 8, 2024 15:54

Corrected changelog

e14c6bc

Removed extra space

d6c5928

papafe requested a review from nirinchev February 8, 2024 14:57

nirinchev reviewed Feb 8, 2024

View reviewed changes

Realm/Realm/Sync/ProgressNotifications/SyncProgress.cs Outdated Show resolved Hide resolved

Removed obsolete

f65e9e0

papafe added 3 commits February 8, 2024 16:42

Last fix

fce8d1e

Merge branch 'main' into fp/progress-notifications

fd93f00

Fixed changelog

7187f58

nirinchev reviewed Feb 8, 2024

View reviewed changes

papafe added 6 commits February 9, 2024 11:03

Various fixes

8ce794d

Small fix

8d8dc5a

Fix for E2E tests

d67355d

Fixes

6a8ef77

Updated to latest commit

f24e587

Removed ignored

d86c69f

Fixed test [skip-ci]

1c4d112

nirinchev changed the title ~~Add support for progress estimate on progress notifications~~ RNET-1083: Add support for progress estimate on progress notifications Mar 7, 2024

nirinchev changed the title ~~RNET-1083: Add support for progress estimate on progress notifications~~ RNET-1083 Add support for progress estimate on progress notifications Mar 7, 2024

nirinchev changed the title ~~RNET-1083 Add support for progress estimate on progress notifications~~ RNET-1083? Add support for progress estimate on progress notifications Mar 7, 2024

nirinchev changed the title ~~RNET-1083? Add support for progress estimate on progress notifications~~ RNET-1083: Add support for progress estimate on progress notifications Mar 7, 2024

papafe added 6 commits April 9, 2024 13:21

Merge branch 'main' into fp/progress-notifications

3ecbb55

# Conflicts: # wrappers/realm-core # wrappers/src/async_open_task_cs.cpp # wrappers/src/sync_session_cs.cpp

Various fixes

1c80ff1

Added ignore

c1ed213

Removed unused

42cec9f

Removed unused test

a817705

Fixes

3a2ddcd

papafe requested a review from nirinchev April 9, 2024 13:44

kiburtse reviewed Apr 10, 2024

View reviewed changes

nirinchev approved these changes Apr 10, 2024

View reviewed changes

Corrected documentation and fixed linting

ae1db72

papafe merged commit cc537e6 into main Apr 16, 2024
66 of 67 checks passed

papafe deleted the fp/progress-notifications branch April 16, 2024 13:37

github-actions bot locked as resolved and limited conversation to collaborators May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RNET-1083: Add support for progress estimate on progress notifications #3479

RNET-1083: Add support for progress estimate on progress notifications #3479

papafe commented Nov 15, 2023 •

edited

Loading

papafe commented Nov 15, 2023

papafe commented Nov 17, 2023

nirinchev left a comment

papafe commented Feb 8, 2024

nirinchev left a comment

coveralls-official bot commented Feb 8, 2024 •

edited

Loading

papafe commented Feb 16, 2024

papafe commented Apr 9, 2024

kiburtse Apr 10, 2024

papafe Apr 10, 2024 •

edited

Loading

papafe Apr 10, 2024

kiburtse Apr 10, 2024

papafe Apr 10, 2024

nirinchev left a comment

nirinchev Apr 10, 2024

papafe Apr 10, 2024

RNET-1083: Add support for progress estimate on progress notifications #3479

RNET-1083: Add support for progress estimate on progress notifications #3479

Conversation

papafe commented Nov 15, 2023 • edited Loading

TODO

papafe commented Nov 15, 2023

papafe commented Nov 17, 2023

nirinchev left a comment

Choose a reason for hiding this comment

papafe commented Feb 8, 2024

nirinchev left a comment

Choose a reason for hiding this comment

coveralls-official bot commented Feb 8, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8630691641

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

papafe commented Feb 16, 2024

papafe commented Apr 9, 2024

kiburtse Apr 10, 2024

Choose a reason for hiding this comment

papafe Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

papafe Apr 10, 2024

Choose a reason for hiding this comment

kiburtse Apr 10, 2024

Choose a reason for hiding this comment

papafe Apr 10, 2024

Choose a reason for hiding this comment

nirinchev left a comment

Choose a reason for hiding this comment

nirinchev Apr 10, 2024

Choose a reason for hiding this comment

papafe Apr 10, 2024

Choose a reason for hiding this comment

papafe commented Nov 15, 2023 •

edited

Loading

coveralls-official bot commented Feb 8, 2024 •

edited

Loading

papafe Apr 10, 2024 •

edited

Loading