Sync progress non-streaming callback stops reporting values too early

[sync-by-unito]

Non-streaming object-store progress callback is removed (and effectively stops emitting values) as soon as number of transferred >= transferable at the time of registration.

This is suboptimal from the api standpoint and hits a few corner cases. Better approach is to emit values until sync concludes and callback for upload/download completion is supposed to be triggered signaling end of syncing.

[sync-by-unito]

➤ PM Bot commented:

Jira ticket: RCORE-2013

[sync-by-unito]

➤ tgoyne commented:

The idea behind non-streaming notifications is to let you can perform a write and then add the notifier and get progress notifications until the write you just did has finished uploading. Changing this to notify until all uploads are complete would actively break the feature.

[sync-by-unito]

➤ kiburtse commented:

Yeah, i've inferred that from a few tests we have, but the problem is that it essentially is only relevant for upload direction and if number of uplodable bytes matches (it is reported asynchronously). It is irrelevant to the general sync progress (which this object store api represents), and makes the behaviour inconsistent between all other combinations of progress callback registration. I don't think you should rely on reported transferred/transferable as a way to ensure your distinct changes are synced. [~nikola.irinchev@mongodb.com] what are your thoughts on this?

[sync-by-unito]

➤ tgoyne commented:

This has been the documented way to determine if you local changes have been synced the entire time that sync has existed, and it works perfectly well for that.

The entire premise of the flx sync progress project involves making upload and download notifications work differently to work around the server being unable to supply correct download progress, so yes, of course upload and download notifications are going to be inconsistent.

[nirinchev]

Changing this to notify until all uploads are complete would actively break the feature.

When I originally proposed that approach, my understanding was that the wait_for_upload_completion API behaved in the way non-streaming progress notifiers do - i.e. when you invoke it, it would record the current client_version + the callback and as uploads are being acked by the server, we would invoke each upload completion callback depending on the version being uploaded. I skimmed through the code and it doesn't appear like this is the case, which is unfortunate as it's inconsistent with how downloads behave. While fixing this would be a good idea, it's probably not ideal to do that as part of the progress notifications project.

I don't have a suggestion for how to make this work other than change the behavior of wait_for_upload/download_completion to

1. Allow registering multiple upload completion callbacks and invoking them independently when the client version at the time of registration of each one of these is uploaded.
2. Allow registering multiple download completion callbacks and invoking them independently when the server MARK response for the MARK sent at registration is received - similarly allowing different completion callbacks to be invoked at different times depending on when they were registered.

This is probably best discussed with @jbreams as he has much better overview of the mechanics of the sync client and is likely going to be able to come up with a shorter-term solution. I'd be happy to provide input on the client-side expectations from the API, but don't have any opinions on the actual implementation.

[tgoyne]

Even if wait_for_upload_completion() worked for this use case, there'd be no reason to go our of our way to break the existing working feature. There's just no reason to change this behavior.