search

cloudant / sync-android

A JSON-based document datastore for Android applications
Apache License 2.0
267 stars 91 forks source link

Finish refactoring of DatastoreImpl functionality into Callables #291

Closed ricellis closed 7 years ago

ricellis commented 8 years ago

From @mikerhodes:

So the first attempt, https://github.com/cloudant/sync-android/compare/54145-hoist-sqlqueuecallables, gives you a good idea of the aims of this task.

BasicDatastore is a huge class, which contains virtually all the logic to manage the MVCC semantics of the database. AttachmentManager breaks out some attachment logic, but really it's all still driven from BasicDatastore. What I'd like to do is to break up BasicDatastore into smaller components, which hopefully can have simpler tasks and can each be understood as a single unit doing a single task. Latterly, we might even be able to start testing things individually.

I think of this a bit like the operations in objective-cloudant -- each SQLCallable is a logical MVCC operation or perhaps lower level database operation. I'm not sure yet whether we want to more formally layer the MVCC semantics vs the database SQL semantics (which would allow us to more easily swap out storage layers later).

For example, https://github.com/cloudant/sync-android/pull/219 pulls out the work of inserting a single revision into the revs table. This might be too small a thing, but the logic is required by several of the higher level operations -- creating, updating and deleting -- so it makes sense to have separately.

Further from that, extracting the database interaction from the BasicDatastore "orchestration" logic would be a good thing. In the first instance, that won't really be possible. The first piece of work will be pulling out all the higher-level things like "create a document" into their own classes, which will do things like checking for MVCC validity (e.g., my update has the right revId of a current leaf node). It'd be nice long term if the database operations could be separated from the business logic.

For example, #219 is a storage level operation, almost. It might actually be slightly too low-level, perhaps it should have some more logic of its own, but we'll see. At least we'll hopefully get to a stage where it's easier to conceptually see what's going on when we have separate classes. If not, then at least we learned that just making classes, however well encapsulated, doesn't help.

Note that BasicDatastore is now DatastoreImpl.

rhyshort commented 8 years ago

393 Introduces a few static inner classes which should be hoisted into their own files and consolidate as appropriate.

rhyshort commented 7 years ago

PRs #460 #461 #463 #464 extract callables from the Query code.

rhyshort commented 7 years ago

I think it's all done now so closing.