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

comsat-mongodb-allanbanks #13

Merged
merged 14 commits into from Jul 21, 2014
Merged

comsat-mongodb-allanbanks #13

merged 14 commits into from Jul 21, 2014

Conversation

circlespainter
Copy link
Member

Hi!

I implemented a mongodb integration module for comsat based on http://www.allanbank.com/mongodb-async-driver/index.html version 2.0 and a fairly complete essential testsuite for it based on Embed MongoDB https://github.com/flapdoodle-oss/de.flapdoodle.embed.mongo ; I think you might be interested in main comsat inclusion.

The module takes a subclassing approach, so existing code based on the driver should require minimal to no code changes. The overridden methods from the original driver are just the blocking and future ones in order to make them fiber-blocking. The fiber-blocking overrides are implemented by bridging the async, callback-based corresponding methods in the original driver through Quasar's FiberAsync.

I can think of further TODOs both for driver and tests but I think inclusion could be possible already (the 99 JUnit tests covering the overridden methods run all ok here); you can find those ideas in the classes' JavaDocs comments, but shortly:

  • Test non-async suspendable methods (already declared in "suspendables"); anyway some are already used as part of test setup
  • Test ListenableFuture functionality
  • Add new fiber-blocking APIs for now async-only operations (with lock-in disadvantage for code starting to use them)
  • Test new fiber-blocking APIs for now async-only operations

Incidentally (and luckily), starting with version 2.0, the license for the basic driver functionality has changed to Apache 2.0 http://www.allanbank.com/mongodb-async-driver/license.html which, to the extent of my knowledge, allows for liberal use even in commercial projects (previously commercial use required a commercial licence, which I think was a more strict policy than comsat's). Maybe the author decided such a change because of 10gen's OSS driver starting to consider/implement an async interface in the 3.0.x branch https://github.com/mongodb/mongo-java-driver/tree/3.0.x/async-driver

…ntation of FiberMongoDatabaseImpl wrapper, skeleton of FiberMongoCollectionImpl wrapper
* Renamed project "comsat-mongodb" -> "comsat-mongodb-allanbanks" in order to stress the underlying
implementation (with dual-licence policy)
* Converted from partial wrapping approach to implementation subclassing and overriding approach. Previous
blocking methods exported by the original implementation are fiber-blocking in this subclassing
implementation. Interface compatibility is retained, so drop-in replacement becomes possible. Disadvantage is
that this implementation now depends on non-API (implementation) from the allanbanks driver
* Test classes structure in place together with first tests
MAJOR: upgraded to AllanBanks driver version 2.0, with more favorable licensing scheme as well
@eitan101
Copy link
Contributor

Wow !!
Thank you very much.
I'll take a look in the code to see if I have something to comment.

@eitan101
Copy link
Contributor

eitan101 commented Jul 8, 2014

The following error appears during the tests:

8:38:10.621 [DEBUG] [TestEventLogger]     WARNING: Uninstrumented methods on the call stack (marked with **): 
18:38:10.622 [DEBUG] [TestEventLogger]      at co.paralleluniverse.fibers.Fiber.checkInstrumentation(Fiber.java:1587)
18:38:10.623 [DEBUG] [TestEventLogger]      at co.paralleluniverse.fibers.Fiber.verifySuspend(Fiber.java:1565)
18:38:10.623 [DEBUG] [TestEventLogger]      at co.paralleluniverse.fibers.FiberAsync.run(FiberAsync.java:117)
18:38:10.624 [DEBUG] [TestEventLogger]      at co.paralleluniverse.fibers.mongodb.FiberMongoDatabaseImpl.runCommand(FiberMongoDatabaseImpl.java:70)
18:38:10.624 [DEBUG] [TestEventLogger]      at com.allanbank.mongodb.client.MongoDatabaseImpl.drop(MongoDatabaseImpl.java:152) **
18:38:10.625 [DEBUG] [TestEventLogger]      at co.paralleluniverse.fibers.mongodb.AbstractTestFiberMongo$2.run(AbstractTestFiberMongo.java:89)
18:38:10.625 [DEBUG] [TestEventLogger]      at co.paralleluniverse.strands.SuspendableUtils$VoidSuspendableCallable.run(SuspendableUtils.java:42)
18:38:10.625 [DEBUG] [TestEventLogger]      at co.paralleluniverse.strands.SuspendableUtils$VoidSuspendableCallable.run(SuspendableUtils.java:30)
18:38:10.626 [DEBUG] [TestEventLogger]      at co.paralleluniverse.fibers.Fiber.run(Fiber.java:994)
18:38:10.626 [DEBUG] [TestEventLogger]      at co.paralleluniverse.fibers.Fiber.run1(Fiber.java:989)

The drop method itself is marked as suspendable, but runCommand is not, So the instrumentor doesn't find a reason to instrument it. In order to solve that, the following method has to be added to META-INF/suspendables:
com.allanbank.mongodb.client.MongoDatabaseImpl.runCommand

@circlespainter
Copy link
Member Author

I'm not sure why I'm not getting the error when running fork's tests... I seem to have "verifyInstrumentation" enabled and I can see that the "scanSuspendables" task is correctly listing "com.allanbank.mongodb.client.MongoDatabaseImpl.runCommand" in "META-INF/suspendable-supers". Have you got it there?

@eitan101
Copy link
Contributor

eitan101 commented Jul 9, 2014

I am also running in your fork.
Yes it is listed in the "META-INF/suspendable-supers", but it is not enough.
You may not see that because it doesn't fail the test, it's only a warning (but an important one).
It can be seen with gradle -i test, or in the test results report located in: ./comsat-mongodb-allanbanks/build/reports/tests/classes/co.paralleluniverse.fibers.mongodb.FiberMongoCollTest.html

…spendables list as per test warnings and eitan101's suggestion
@circlespainter
Copy link
Member Author

Thanks! I've found the warning and added the new suspendable method, then the warning has disappeared

@circlespainter
Copy link
Member Author

Hi! I added some additional asserts for the future-based API in order to make sure the addListener+callback functionality works ok.

Still clean merge and the stderr and stdout are still clean apart from some CPU hogging warnings coming from blocking calls in the tests (and ultimately driver's connection) setup stages.

pron added a commit that referenced this pull request Jul 21, 2014
comsat-mongodb-allanbanks

Thanks again!
@pron pron merged commit 0b093ca into puniverse:master Jul 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants