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

Refactor places connection management #718

Merged
merged 16 commits into from Mar 5, 2019

Conversation

Projects
None yet
4 participants
@thomcc
Copy link
Contributor

commented Feb 27, 2019

This is a follow up from #666 (as sad as I am to lose the extremely metal PR number), since for whatever reason I can't push to markh's branch.

It still provides no way of clearing out the APIS HashMap, although it fixes most of the other issues I had that are still relevant. I'll file a follow up for the APIS HashMap before landing, we expect this to be a global singleton most of the time anyway.

In the mean time, this needs some form of sign-off from @grigoryk, specifically for the shape of the Kotlin API, since it's a huge breaking change to the places API.

It also needs a CHANGELOG entry, which I'll do once it has R+ to avoid churn on it.

mhammond and others added some commits Feb 14, 2019

This is a proof-of-concept for how we might manage connections.
It is currently in a good enough state to see if the idea is worth pursuing.
At a minimum, and before landing, we'd:

* split the schema.rs changes into its own PR - they aren't strictly needed
  for what this patch does.

* Probably move PlacesAPI into its own source file, and better consider
  threading considerations - the PlacesAPI instance will end up being called
  from multiple threads (although the stuff we hand out shouldn't be)

* Manage the "sync" connection similarly to how we manage the "write"
  connection. The PlacesAPI object is probably also where the sync "store"
  objects can live (as currently we create brand new stores each sync, which
  isn't as optimized as it should be)

* FFI changes needed to support this - this would be a breaking change, but
  shouldn't be too difficult to manage.

* Lots of other cleanup - I figured I'd get comments on the general approach
  before spending much time cleaning things up.

So by all means, offer all suggestions, but the main thing I'd like from this
is general approval that we should go ahead with this.

@thomcc thomcc requested review from mhammond and grigoryk Feb 27, 2019

@mhammond
Copy link
Member

left a comment

Thanks heaps for taking this over Thom!

Show resolved Hide resolved ...ents/places/android/src/main/java/org/mozilla/places/PlacesConnection.kt Outdated
Show resolved Hide resolved ...ents/places/android/src/main/java/org/mozilla/places/PlacesConnection.kt Outdated
Show resolved Hide resolved ...ents/places/android/src/main/java/org/mozilla/places/PlacesConnection.kt Outdated
*/
interface PlacesAPI {
interface PlacesApiInterface {

This comment has been minimized.

Copy link
@mhammond

mhammond Feb 28, 2019

Member

Thom pointed this out earlier, but it's worth re-flagging for @grigoryk - is there a better naming convention for these interfaces (and more to the point, are they even required?)

This comment has been minimized.

Copy link
@thomcc

thomcc Mar 1, 2019

Author Contributor

I don't think we'll hear back on this soon, since Grisha is currently in Russia IIUC.

This comment has been minimized.

Copy link
@grigoryk

grigoryk Mar 5, 2019

Contributor

These interfaces are very useful at the consuming side of this, as we can write code which is defined in terms of the interfaces, making testing much easier. I notice that Kotlin's own sources omit words like "interface", and generally follow a "SpecificThing" (class) implements "Thing" (interface) convention.

Show resolved Hide resolved components/places/ffi/src/lib.rs Outdated
Show resolved Hide resolved components/places/src/api/places_api.rs Outdated

thomcc added some commits Mar 4, 2019

@thomcc

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

I'm not sure we should land this right away. In particular, it would block an update of application services in the android-components repo, and we'd somewhat like to get @fzzzy up to speed with how to make changes across the two repos (there are a number of fairly simple bugs requiring these sorts of changes, however they could not be addressed if we land this).

@mhammond mhammond added the review label Mar 4, 2019

@rfk rfk modified the milestones: rust, Rustmarks Mar 5, 2019

@rfk rfk added the 📚Bookmarks label Mar 5, 2019

@grigoryk
Copy link
Contributor

left a comment

This generally looks good to me. I've only glanced through the rust bits. Left some inline comments regarding safe usage guidelines. Also, it's not clear to me (yet) what is the distinction between sync and write connection types. Different rusqlite flags are used, but the difference is still quite opaque. How does one obtain a sync connection specifically? Kotlin API doesn't expose that type, nor a method to get a connection of that type, but the Rust side branches on it.

Show resolved Hide resolved ...ents/places/android/src/main/java/org/mozilla/places/PlacesConnection.kt Outdated
// Places Rust library does not yet support schema migrations; as a very temporary quick
// fix to avoid crashes of our upstream consumers, let's delete the database file
// entirely and try again.
// FIXME https://github.com/mozilla/application-services/issues/438

This comment has been minimized.

Copy link
@grigoryk

grigoryk Mar 5, 2019

Contributor

nit: you probably want to remove the other side of this as well https://github.com/mozilla/application-services/blob/master/components/places/src/db/schema.rs#L157-L159 which iiuc shouldn't throw anymore.

This comment has been minimized.

Copy link
@thomcc

thomcc Mar 5, 2019

Author Contributor

It still can throw in the case of very old upgrades, actually.

I missed that this was taken out.

This is tricky to support from rust so I filed a follow up to support it (#730)

@@ -35,22 +35,68 @@ pub extern "C" fn places_enable_logcat_logging() {
}

lazy_static::lazy_static! {
static ref APIS: ConcurrentHandleMap<Arc<PlacesApi>> = ConcurrentHandleMap::new();

This comment has been minimized.

Copy link
@grigoryk

grigoryk Mar 5, 2019

Contributor

If PlacesApi is intended to ensure a single writer, why does APIS maintain a set of multiple PlacesApi instances?

This comment has been minimized.

Copy link
@thomcc

thomcc Mar 5, 2019

Author Contributor

Multiple databases. In general we don't care about this case so much in the kotlin API (we care in test code though, in particular we don't want to end up in a situation where to run integration tests we need multiple processes), but using a handlemap gives us an additional memory and thread safety barrier.

*/
interface PlacesAPI {
interface PlacesApiInterface {

This comment has been minimized.

Copy link
@grigoryk

grigoryk Mar 5, 2019

Contributor

These interfaces are very useful at the consuming side of this, as we can write code which is defined in terms of the interfaces, making testing much easier. I notice that Kotlin's own sources omit words like "interface", and generally follow a "SpecificThing" (class) implements "Thing" (interface) convention.

* Syncs the places stores.
*
* Note that this function blocks until the sync is complete, which may
* take some time due to the network etc. Because only 1 thread can be

This comment has been minimized.

Copy link
@grigoryk

grigoryk Mar 5, 2019

Contributor

You mention threading in context of PlacesApi here. Can you expand on that, ideally in a docstring for PlacesApiInterface (or its impl..)? Quickly reading through this, I'm uncertain as to how this API should be used safely - e.g. threading distinction between an api object, specific writers, readers, etc.

This comment has been minimized.

Copy link
@thomcc

thomcc Mar 5, 2019

Author Contributor

In general everything is thread safe, and locking is done internally.

I'll make it more clear when this happens, though.

This comment has been minimized.

Copy link
@thomcc

thomcc Mar 5, 2019

Author Contributor

It's not clear to me how to rename these to follow SpecificThing vs Thing convention, so for now I guess the awkward names will stay.

@thomcc

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

We don't expose a sync connection, only write connections. In order to sync, you need to call the sync method on the API itself. This simplifies the implementation for us a decent amount.

@thomcc thomcc merged commit 6a77745 into master Mar 5, 2019

1 of 6 checks passed

Taskcluster (pull_request) TaskGroup: Pending (for pull_request.synchronize)
Details
ci/circleci: Rust benchmarks CircleCI is running your tests
Details
ci/circleci: Rust tests - beta CircleCI is running your tests
Details
ci/circleci: Rust tests - stable CircleCI is running your tests
Details
ci/circleci: Sync integration tests CircleCI is running your tests
Details
ci/circleci: Check Rust formatting Your tests passed on CircleCI!
Details

@mhammond mhammond deleted the connection_types branch Mar 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.