Mongo #731
Mongo #731
Conversation
Basic Driver tests pass when there is a local running Mongo node now
|
@sdesmond46 Thanks for your contribution. Do you have benchmark test or data for the implementation? |
I didn't do any formal benchmark testing on it, but I didn't see any real performance issues compared to one LevelDB when Mongo was running on the same machine. There is obviously a bit more latency when Mongo is running on a different machine, but it certainly wasn't something that made the kernel unusable. I'm happy to add some more formal testing if that would help though. |
Thanks for your info update. |
There was a problem hiding this comment.
Overall the PR is awesome! Thanks for your contribution.
Few things I think need to be changed before getting this PR to merge into the aion repo:
- Some of the logs should be put into "DEBUG" instead of all "INFO"
- the binary file inside the testing resource is too big, could you find a way to put the link instead of put whole binary into it? Or can it be built by source code?
- The Aion repo is going to switch license from GPL_V3 to MIT, are you agree your contribution be a part of the MIT license?
- I am getting some errors when I launch the kernel with mondb in the "custom" network, could you give me the correct settings for eliminating the following message?Thanks.
18-12-03 18:00:14.905 INFO DB [main]: Initializing MongoDB at mongodb://localhost:37017
18-12-03 18:00:14.907 INFO DB [main]: Creating new mongo client to connect to mongodb://localhost:37017
18-12-03 18:00:14.991 INFO DB [main]: Finished opening the Mongo connection
18-12-03 18:00:14.991 ERROR DB [main]: Database cannot be saved to disk for .
18-12-03 18:00:14.991 INFO DB [main]: Initializing MongoDB at mongodb://localhost:37017
18-12-03 18:00:14.991 INFO DB [main]: Reusing existing mongo client for mongodb://localhost:37017
18-12-03 18:00:14.991 INFO DB [main]: Finished opening the Mongo connection
18-12-03 18:00:14.992 ERROR DB [main]: Database cannot be saved to disk for .
|
@AionJayT thanks for taking a look! Couple of responses to your questions:
|
|
For point 2. |
| @@ -33,7 +33,7 @@ if [ "$noGui" != "true" ] && [ ! -d "$JAVAFX_PATH" ]; then | |||
| fi | |||
|
|
|||
| module_path=$JDK_PATH/jmods | |||
| add_modules="java.base,java.xml,java.logging,java.management,jdk.unsupported,jdk.sctp" | |||
| add_modules="java.base,java.xml,java.logging,java.management,jdk.unsupported,jdk.sctp,java.security.sasl" | |||
There was a problem hiding this comment.
@aion-kelvin I think this change will impact the GUI PR. Just making a note here to not lose track of the change.
There was a problem hiding this comment.
LGTM. Thank you for the contribution! Approved pending @AionJayT's approval.
There was a problem hiding this comment.
Hey @sdesmond46, thank you for your work on this. Overall it looks pretty good. I have a suggestion for your connection manager class. Also, I like the shell script for starting up a MongoDb via Docker. I was thinking -- would it make more sense to have the shell script install MongoDb from Docker Hub, instead of checking in a Docker binary into Github?
| * class keeps track of reference counting active instances and will close the connection once all instances | ||
| * are done being used. | ||
| */ | ||
| public class MongoConnectionManager { |
There was a problem hiding this comment.
Is this class meant to be thread-safe? I presumed yes due to the ConcurrentHashMap.
If yes, I think getMongoClientInstance and closeMongoClientInstance need to be marked synchronized. For instance, it would be a problem if two threads call closeMongoClientInstance("myURL") around the same time when activeClientCount of "myURL" is 1, then the following sequence can happen:
- thread 1 runs the first if check and doesn't enter the exception entering code
- thread 2 runs the first if check and doesn't enter the exception entering code
- thread 1 completes the rest of the method, which means it executes the mongoUriToClientMap.remove(mongoClientUri) in the second if-block
- thread 2 then continues and throws NullPointerException on
this.activeClientCountMap.get(mongoClientUri).decrementAndGet()because thread 1 already removed it
I believe getMongoClientInstance has a similar issue with its "check if in map; otherwise put in map" logic
There was a problem hiding this comment.
Ah you're totally right, it is meant to be thread safe but I forgot the synchronized keyword. I'll add that in, thanks for catching it.
|
@aion-kelvin - Re: using Docker and the Mongo binaries -- The script I added runs mongo with the docker image available on DockerHub, The Update: I looked into building mongo from source, but that doesn't seem like a great option because their source is ~220MB and it's still not done building after an hour on my machine which doesn't seem acceptable. The other option which could work is to download the Mongo tarball from https://fastdl.mongodb.org/linux/mongodb-linux-x86_64-ubuntu1404-3.6.9.tgz, extract that and use that in the tests. That file is about 100MB |
|
Thanks for the changes! Code looks good to me -- once we figure out how to package the mongo binary I think we're good to go.
I'll defer to @AionJayT for the final decision, but IMO if the test needs to call an external program, it is an integration test, not a unit test. And I think it is ok for an integration test to need extra set-up. |
|
@sdesmond46 I total agree with using docker for testing the mongo. Could you able to do this change? |
This changes switches to use docker instead of running the raw mongo binary when executing the mongo driver tests. It uses Spotify's Docker controller library to interact with the docker service
Yep, I just made the changes. They assume that there is a running docker daemon on the machine, and exec and cleanup the mongo container, but remove the mongo binaries from the repo |
|
@sdesmond46 the ciBuild has tailed tests, could you able to fix it? org.aion.db.impl.DriverBaseTest > initializationError FAILED org.aion.db.impl.ConcurrencyTest > initializationError FAILED |
Yeah .I spent some time digging it this and trying to figure out a solution,. The root cause is the tests can't connect to a running Docker service to start up the mongo container when running in the CI environment. I'm not super familiar with Jenkins and the way the runners are set up for Aion, so I wasn't sure if that's because there isn't a docker service running, or I don't know the right DOCKER_HOST to connect to it. Is there any info on how the Jenkins agents are configured for the Aion CI environment? Thanks for the help! |
|
merge this PR in and will try (or ignore) to fix the fail tests. |
Notice
It is not allowed to submit your PR to the master branch directly, please submit your PR to the master-pre-merge branch.
Description
This change adds support for a new database vendor - MongoDB. This will be the first database which is not stored in the file system where the kernel is running, which allows for more flexibility when deploying and running kernels.
The change introduces a runtime dependency on the Mongo Java Driver, and a test dependency on Mongo DB itself so we can test against a localhost Mongo instance.
To use the Mongo driver to connect to a Mongo database running on your local machine, you would update the
config.xmlwith<vendor>mongodb</vendor>and<path>mongodb://localhost:27017</path>Type of change
Insert x into the following checkboxes to confirm (eg. [x]):
Testing
Please describe the tests you used to validate this pull request. Provide any relevant details for test configurations as well as any instructions to reproduce these results.
Verification
Insert x into the following checkboxes to confirm (eg. [x]):