[core][android]Decouple offline storage vacuum and the deleteRegion API #15899
Conversation
pozdnyakov
changed the title
pozdnyakov
force-pushed
the
mikhail_offline_storage_pack
branch
2 times, most recently
from
3858797
to
5587da4
Compare
|
@LukasPaczos @tobrun could you pls 👀 at the android-related changes, especially OfflineRegion.deleteAndSkipPackDatabase API. Is this API shape quite appropriate for Android, would it be better to just add a flag to the existing |
| * After this operation is complete, the database file does not shrink. | ||
| * Database file packing can be done later with `packDatabase()`. | ||
| * | ||
| * This method is useful optimization e.g. when several regions should be deleted in a row. |
| @@ -82,9 +82,9 @@ class DefaultFileSource::Impl { | |||
| } | |||
| } | |||
|
|
|||
| void deleteRegion(OfflineRegion&& region, std::function<void (std::exception_ptr)> callback) { | |||
| void deleteRegion(OfflineRegion&& region, bool skipPack, std::function<void(std::exception_ptr)> callback) { | |||
There was a problem hiding this comment.
I just don't like the fact this is a double negative.
false here means doing pack. What about just doPack or pack?
There was a problem hiding this comment.
@pozdnyakov I think that adding a separate method is fine 👍
| @@ -429,6 +429,53 @@ public void run() { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Same as the method above but skipping database file packing for performance reasons. | |||
There was a problem hiding this comment.
Could you link the method instead with {@link com.mapbox.mapboxsdk.offline.OfflineRegion#delete(OfflineRegion.OfflineRegionDeleteCallback)}?
| /** | ||
| * Same as the method above but skipping database file packing for performance reasons. | ||
| * <p> | ||
| * This method is useful optimization e.g. when several regions should be deleted in a row. |
There was a problem hiding this comment.
Would be great to highlight here that the database file size will not shrink and you need to call packDatabase to do it later (and link the method like above).
pozdnyakov
force-pushed
the
mikhail_offline_storage_pack
branch
from
a60333a
to
d6a194c
Compare
pozdnyakov
force-pushed
the
mikhail_offline_storage_pack
branch
from
d6a194c
to
2318841
Compare
|
@LukasPaczos @tmpsantos Thanks for your comments! Fixed now in the latest patch set, PTAL. |
- introduce `OfflineDatabase::pack()` standing for incremental vacuum - make pack optional at offline region deletion - update `OfflineDatabase.DeleteRegion` test accordingly to the items above
pozdnyakov
force-pushed
the
mikhail_offline_storage_pack
branch
from
2318841
to
e9940b8
Compare
| * | ||
| * This method is a useful optimization e.g. when several regions should be deleted in a row. | ||
| */ | ||
| void deleteOfflineRegionSkipPackDatabase(OfflineRegion&&, std::function<void(std::exception_ptr)>); |
There was a problem hiding this comment.
Thanks for the catch!
Add `packDatabase()` method and `bool pack` argument to the `deleteOfflineRegion()` method.
pozdnyakov
force-pushed
the
mikhail_offline_storage_pack
branch
from
e9940b8
to
13ed8a8
Compare
Fixes https://github.com/mapbox/mapbox-gl-native-team/issues/102