Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Resume file source to complete resources cache path change #14546

Merged
merged 2 commits into from
May 24, 2019

Conversation

LukasPaczos
Copy link
Member

Closes #14334.

I've initially made DefaultFileSource::setResourceCachePath synchronous and managed threading on the java side, but now that I think about it, it might be cleaner to use a callback and do the threading on the core side.

@LukasPaczos LukasPaczos added Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl labels Apr 30, 2019
@LukasPaczos LukasPaczos added this to the release-n milestone Apr 30, 2019
@LukasPaczos LukasPaczos marked this pull request as ready for review May 14, 2019 12:56
@LukasPaczos
Copy link
Member Author

I've made the changes mentioned in the OP and this is ready for a review.

platform/android/src/file_source.cpp Outdated Show resolved Hide resolved
platform/android/src/file_source.cpp Outdated Show resolved Hide resolved
platform/android/src/file_source.cpp Outdated Show resolved Hide resolved
platform/android/src/file_source.hpp Outdated Show resolved Hide resolved
@@ -17,6 +17,7 @@ template <typename T> class Thread;
} // namespace util

class ResourceTransform;
using PathChangeCallback = std::function<void ()>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a way to communicate failure?

Right now, void OfflineDatabase::changePath(const std::string&) can throw exception, should developer be notified when setResourceCachePath fails?

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the way for void OfflineDatabase::changePath(const std::string&) to throw? Is that only the assert(!db) and assert(statements.empty()) assertions in OfflineDatabase::initialize()? If that's the case, we are logging a failure to cleanup beforehand, so I don't think we need to send failure callback just before we crash anyway. Same if we fail to open the database.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the underlying problem here is that we don't have a proper mechanism for detecting errors when we failed to open the database/

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this something that we'd be able to follow up on in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@LukasPaczos almost every line in OfflineDatabase::initialize throws an exception (db->*, file deletion). ResourcesCachePathChangeCallback already provides onError, that might be used on SDK side to notify developer when changePath failed.

This PR did not introduce that issue, so, I think fix for propagating an error on initialize can be done in separate PR. May be worth creating issue + adding TODO note for PathChangeCallback that refers to an issue.

@tmpsantos wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #14759 and added a TODO.

@LukasPaczos LukasPaczos force-pushed the lp-path-update-14334 branch 2 times, most recently from 4094844 to bf759f6 Compare May 20, 2019 16:26
@LukasPaczos
Copy link
Member Author

Comments addressed.

Copy link
Contributor

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

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

lgtm from my side, please don't forget to create an issue for possible failure when database initialization fails.

@LukasPaczos LukasPaczos merged commit 1bef3b7 into master May 24, 2019
@LukasPaczos LukasPaczos deleted the lp-path-update-14334 branch May 24, 2019 13:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verify database path update after resuming the thread
4 participants