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.

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

[RKOTLIN-1079] Add support for transfer progress estimate #1575

Merged
merged 34 commits into from
May 24, 2024

Conversation

clementetb
Copy link
Contributor

@clementetb clementetb commented Nov 16, 2023

Core now reports progress estimates during a Download/Upload. This PR adds support for it.

The progress estimate is a double ranged between 0 - 1.

Depends on realm/realm-core#7124

Closes #1744

@clementetb clementetb self-assigned this Nov 16, 2023
# Conflicts:
#	packages/cinterop/src/nativeDarwin/kotlin/io/realm/kotlin/internal/interop/RealmInterop.kt
#	packages/external/core
#	packages/jni-swig-stub/src/main/jni/realm_api_helpers.cpp
#	packages/jni-swig-stub/src/main/jni/realm_api_helpers.h
@cla-bot cla-bot bot added the cla: yes label May 15, 2024
@clementetb clementetb changed the title [Sync] Add support for transfer progress estimate [RKOTLIN-1079] Add support for transfer progress estimate May 15, 2024
@clementetb clementetb requested a review from rorbech May 15, 2024 22:33
@clementetb clementetb marked this pull request as ready for review May 16, 2024 07:21
Copy link
Contributor

@rorbech rorbech left a comment

Choose a reason for hiding this comment

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

Haven't done a full review, but we should also enable and test this for FLX sync.

CHANGELOG.md Outdated
@@ -4,11 +4,12 @@
This release will bump the Realm file format from version 23 to 24. Opening a file with an older format will automatically upgrade it from file format v10. If you want to upgrade from an earlier file format version you will have to use Realm Kotlin v1.13.1 or earlier. Downgrading to a previous file format is not possible.

### Breaking changes
* None.
* Sync progress updates no longer report `transferredBytes` and `totalBytes`. (Issue [#1744](https://github.com/realm/realm-kotlin/issues/1744) [RKOTLIN-1079](https://jira.mongodb.org/browse/RKOTLIN-1079)).
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the new values are the enhancement, but I think I would have combined both into one breaking change entry and put emphasis on the new better precision ... And support for flx-sync, right?

// Await the flow actually being active, this requires actual data transfer as
// we arent guaranteed any initial events.
writerRealm.writeSampleData(TEST_SIZE, timeout = TIMEOUT)
mutex.lock()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note this will not timeout (if the test fails) only if the Job is cancelled. https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.sync/-mutex/lock.html
maybe wrap with withTimeout(TIMEOUT)

@clementetb clementetb requested a review from rorbech May 24, 2024 00:21
Copy link
Contributor

@rorbech rorbech left a comment

Choose a reason for hiding this comment

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

LGTM - There are a number of places where the tests could hang if they fail in unexpected ways. Don't think it is critical, so maybe we could just do that in a later PR.

// `flow.collect()` can be called.
channel.receiveOrFail()
realm.close()
job.await()
Copy link
Contributor

Choose a reason for hiding this comment

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

This could hang forever if the test fails in some unexpected way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created a ticket to address this after the PR. We should revisit these tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@clementetb clementetb merged commit e8949fe into main May 24, 2024
6 checks passed
@clementetb clementetb deleted the ct/progress-estimates branch May 24, 2024 07:38
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement progress notifications
3 participants