Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Ispn 1797 Implement MongoDB based cache store #1473

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
Contributor

gscheibel commented Nov 20, 2012

No description provided.

@tristantarrant tristantarrant and 1 other commented on an outdated diff Nov 27, 2012

cachestore/mongodb/pom.xml
+<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <parent>
+ <artifactId>infinispan-cachestore-parent</artifactId>
+ <groupId>org.infinispan</groupId>
+ <version>5.2.0-SNAPSHOT</version>
+ <relativePath>../pom.xml</relativePath>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>infinispan-cachestore-mongodb</artifactId>
+ <packaging>bundle</packaging>
+ <name>Infinispan MongoDB CacheStore</name>
+ <description>Infinispan MongoBD CacheStore module</description>
@gscheibel

gscheibel Nov 27, 2012

Contributor

where exactly ? the capital S in the "CacheStore" word

@tristantarrant

tristantarrant Nov 27, 2012

Owner

s/MongoBD/MongoDB/

@gscheibel

gscheibel Nov 27, 2012

Contributor

ohh yes, I read it 3 times and didn't see the typo :)

@tristantarrant tristantarrant commented on an outdated diff Nov 27, 2012

...odb/configuration/MongoDBCacheStoreConfiguration.java
+import org.infinispan.util.TypedProperties;
+
+/**
+ * @author Guillaume Scheibel <guillaume.scheibel@gmail.com>
+ */
+@BuiltBy(MongoDBCacheStoreConfigurationBuilder.class)
+public class MongoDBCacheStoreConfiguration extends AbstractStoreConfiguration implements
+ LegacyLoaderAdapter<MongoDBCacheStoreConfig> {
+
+ private String host;
+ private int port;
+ private int timeout;
+ private String username;
+ private String password;
+ private String database;
+ private String collection;
@tristantarrant

tristantarrant Nov 27, 2012

Owner

These should all be final to ensure initialization in the constructor

@tristantarrant tristantarrant and 1 other commented on an outdated diff Nov 27, 2012

...java/org/infinispan/loaders/mongodb/package-info.java
+ * the License, or (at your option) any later version.
+ *
+ * This software is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+
+/**
+ * This package contains a {@link org.infinispan.loaders.CacheStore} implementation based on
+ * persisting to Apache Cassandra
@gscheibel

gscheibel Nov 27, 2012

Contributor

Copy/paste error

@tristantarrant tristantarrant commented on an outdated diff Nov 27, 2012

...odb/configuration/MongoDBCacheStoreConfiguration.java
+ boolean fetchPersistentState, boolean ignoreModifications, TypedProperties properties,
+ AsyncStoreConfiguration asyncStoreConfiguration, SingletonStoreConfiguration singletonStoreConfiguration) {
+ super(purgeOnStartup, purgeSynchronously, purgerThreads, fetchPersistentState, ignoreModifications, properties,
+ asyncStoreConfiguration, singletonStoreConfiguration);
+ this.host = host;
+ this.port = port;
+ this.username = username;
+ this.password = password;
+ this.database = database;
+ this.collection = collection;
+ this.timeout = timeout;
+ }
+
+ @Override
+ public MongoDBCacheStoreConfig adapt() {
+ return new MongoDBCacheStoreConfig(host, port, timeout, username, password, database, collection);
@tristantarrant

tristantarrant Nov 27, 2012

Owner

You need to invoke the LegacyConfigurationAdaptor.adapt() method here so that the common properties are set on the configuration object (e.g purgeSynchronously, purgerThreads, etc)

@tristantarrant tristantarrant and 1 other commented on an outdated diff Nov 27, 2012

...figuration/MongoDBCacheStoreConfigurationBuilder.java
+
+ @Override
+ public MongoDBCacheStoreConfiguration create() {
+ return new MongoDBCacheStoreConfiguration(host, port, timeout, username, password, database, collection,
+ purgeOnStartup, purgeSynchronously, purgerThreads, fetchPersistentState,
+ ignoreModifications, TypedProperties.toTypedProperties(properties), async.create(), singletonStore.create());
+ }
+
+ @Override
+ public MongoDBCacheStoreConfigurationBuilder read(MongoDBCacheStoreConfiguration template) {
+ this.host = template.host();
+ this.username = template.username();
+ this.password = template.password();
+ this.database = template.database();
+ this.collection = template.collection();
+ return this;
@tristantarrant

tristantarrant Nov 27, 2012

Owner

You also need to read all the LockSupportStore and AbstractStore configuration

@gscheibel

gscheibel Nov 29, 2012

Contributor

The mongodb configuration doesn"t implement LockSupportStore, so why should it take care of its conf or should if implement it too ?

@tristantarrant tristantarrant commented on an outdated diff Nov 27, 2012

...loaders/mongodb/configuration/XmlFileParsingTest.java
+ " <connection host=\"localhost\" port=\"27017\" timeout=\"2000\"/>\n" +
+ " <authentication username=\"mongoUser\" password=\"mongoPass\" />\n" +
+ " <storage database=\"infinispan_test_database\" collection=\"infispan_cachestore\" />\n" +
+ " </mongodbStore>\n" +
+ " </loaders>\n" +
+ " </default>\n" +
+ TestingUtil.INFINISPAN_END_TAG;
+
+ MongoDBCacheStoreConfiguration store = (MongoDBCacheStoreConfiguration) buildCacheManagerWithCacheStore(config);
+ assert store.host().equals("localhost");
+ assert store.port() == 27017;
+ assert store.username().equals("mongoUser");
+ assert store.password().equals("mongoPass");
+ assert store.database().equals("infinispan_test_database");
+ assert store.collection().equals("infispan_cachestore");
+ }
@tristantarrant

tristantarrant Nov 27, 2012

Owner

Please add tests for testing whether moving back and forth betweend org.infinispan.configuration and org.infinispan.config using read() and adapt() works

@tristantarrant tristantarrant commented on the diff Nov 27, 2012

...infinispan/loaders/mongodb/MongoDBCacheStoreTest.java
+
+/**
+ * @author Guillaume Scheibel <guillaume.scheibel@gmail.com>
+ */
+@Test(testName = "loaders.remote.MongoDBCacheStoreTest", groups = "functional")
+public class MongoDBCacheStoreTest extends BaseCacheStoreTest {
+
+ private MongoDBCacheStore cacheStore;
+ @Override
+ protected CacheStore createCacheStore() throws Exception {
+ MongoDBCacheStoreConfig config = new MongoDBCacheStoreConfig("127.0.0.1", 27017, 2000, "","", "infinispan_test_database", "infinispan_indexes");
+ config.setPurgeSynchronously(true);
+
+ cacheStore = new MongoDBCacheStore();
+ cacheStore.init(config, getCache(), getMarshaller());
+ cacheStore.start();
@tristantarrant

tristantarrant Nov 27, 2012

Owner

I guess this test needs a running MongoDB instance ? If so we either need a way to start/stop a MongoDB instance for the test in an autonomous fashion or the test should be disabled by default (maybe put it in a different group which can be enabled when an appropriate environment is present)

@gscheibel

gscheibel Nov 27, 2012

Contributor

Yes it does. In OGM we actually have put the MongoDB module in a separate maven profile and we can override connection settings (hostname and port) by using environment variables. What do you think about this approach ?

@tristantarrant

tristantarrant Nov 27, 2012

Owner

Place the test in a group "mongodb", then add a profile to the pom which adds the group to the surefire config:

<plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-surefire-plugin</artifactId>
            <configuration>
               <groups>mongodb</groups>
            </configuration>
</plugin>
@gscheibel

gscheibel Nov 29, 2012

Contributor

Should I remove the MongoDBCacheStoreTest class from the functionnal group ?

@Sanne

Sanne Feb 13, 2013

Owner

@gscheibel you should also activate the group if the environment properties for MongoDB are set, like we did for OGM.
Of course it would be great if you could use the same environment variables, so all team members don't have to review the configuration ;-)

@Sanne Sanne commented on an outdated diff Nov 30, 2012

...org/infinispan/loaders/mongodb/MongoDBCacheStore.java
@@ -0,0 +1,328 @@
+/*
+ * JBoss, Home of Professional Open Source
+ * Copyright 201Z Red Hat Inc. and/or its affiliates and other
@Sanne

Sanne Nov 30, 2012

Owner

201Z ??? I guess it should be 2012 (with a two not a letter Z)

@Sanne

Sanne Nov 30, 2012

Owner

I'm very curious how you can make such a typo :D amazing

@Sanne Sanne and 2 others commented on an outdated diff Nov 30, 2012

...org/infinispan/loaders/mongodb/MongoDBCacheStore.java
+
+ @Override
+ public void init(CacheLoaderConfig config, Cache<?, ?> cache, StreamingMarshaller m) throws CacheLoaderException {
+ super.init(config, cache, m);
+ this.cfg = (MongoDBCacheStoreConfig) config;
+ }
+
+ @Override
+ protected void purgeInternal() throws CacheLoaderException {
+ BasicDBObject searchObject = new BasicDBObject();
+ searchObject.put(TIMESTAMP_FIELD, new BasicDBObject("$gt", 0).append("$lt", System.currentTimeMillis()));
+ this.collection.remove(searchObject);
+ }
+
+ @Override
+ public synchronized void store(InternalCacheEntry entry) throws CacheLoaderException {
@Sanne

Sanne Nov 30, 2012

Owner

why does this need to be synchronized ?

@gscheibel

gscheibel Nov 30, 2012

Contributor

Because of the testConcurrency() method, it avoids duplicate entry errors

@maniksurtani

maniksurtani Dec 4, 2012

Member

store() should definitely not be synchronized. Any concurrency control really should be in MongoDB itself.

@gscheibel

gscheibel Dec 4, 2012

Contributor

Ok, I understand. Do you recommend to check if the entry already exists and
if it is to do nothing or to update it ?
Le 4 déc. 2012 03:29, "Manik Surtani" notifications@github.com a écrit :

In
cachestore/mongodb/src/main/java/org/infinispan/loaders/mongodb/MongoDBCacheStore.java:

  • @override
  • public void init(CacheLoaderConfig config, Cache cache, StreamingMarshaller m) throws CacheLoaderException {
  •  super.init(config, cache, m);
    
  •  this.cfg = (MongoDBCacheStoreConfig) config;
    
  • }
  • @override
  • protected void purgeInternal() throws CacheLoaderException {
  •  BasicDBObject searchObject = new BasicDBObject();
    
  •  searchObject.put(TIMESTAMP_FIELD, new BasicDBObject("$gt", 0).append("$lt", System.currentTimeMillis()));
    
  •  this.collection.remove(searchObject);
    
  • }
  • @override
  • public synchronized void store(InternalCacheEntry entry) throws CacheLoaderException {

store() should definitely not be synchronized. Any concurrency control
really should be in MongoDB itself.


Reply to this email directly or view it on GitHubhttps://github.com/infinispan/infinispan/pull/1473/files#r2301829.

@Sanne

Sanne Dec 4, 2012

Owner

@gscheibel if the entry already exists it should be replaced with the new one.

Checking for existance is tricky, as after you check it might be changed by a different thread. I would expect that MongoDB could provide you a method to insert the element overwriting if needed?

@gscheibel

gscheibel Dec 4, 2012

Contributor

@Sanne ok, I'll change the way we store the entries. From an insert() to an update() with insertion if it does exist.

@Sanne Sanne commented on the diff Nov 30, 2012

...org/infinispan/loaders/mongodb/MongoDBCacheStore.java
+ public Set<InternalCacheEntry> loadAll() throws CacheLoaderException {
+ DBCursor dbObjects = this.collection.find();
+ Set<InternalCacheEntry> entries = new HashSet<InternalCacheEntry>(dbObjects.count());
+ while (dbObjects.hasNext()) {
+ DBObject next = dbObjects.next();
+ try {
+ InternalCacheEntry ice = this.createInternalCacheEntry(next);
+ entries.add(ice);
+ } catch (Exception e) {
+ throw log.unableToCreateCacheEntry(next, e);
+ }
+ }
+ return entries;
+ }
+
+
@Sanne

Sanne Nov 30, 2012

Owner

there's one empty line too far here ;)

@Sanne Sanne commented on an outdated diff Nov 30, 2012

...org/infinispan/loaders/mongodb/MongoDBCacheStore.java
+import org.infinispan.loaders.CacheLoaderException;
+import org.infinispan.loaders.CacheLoaderMetadata;
+import org.infinispan.loaders.mongodb.logging.Log;
+import org.infinispan.marshall.StreamingMarshaller;
+import org.infinispan.util.logging.LogFactory;
+
+import java.io.IOException;
+import java.io.ObjectInput;
+import java.io.ObjectOutput;
+import java.net.UnknownHostException;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Set;
+
+/**
+ * A persistent <code>CacheLoader</code> based on MongoDB project. See http://www.mongodb.org/
@Sanne

Sanne Nov 30, 2012

Owner

"based on MongoDB project"

should either be
"based on the MongoDB project"
or
"based on MongoDB"

@Sanne Sanne commented on an outdated diff Nov 30, 2012

...ispan/loaders/mongodb/MongoDBConfigurationParser.java
+package org.infinispan.loaders.mongodb;
+
+import org.infinispan.configuration.parsing.ConfigurationBuilderHolder;
+import org.infinispan.configuration.parsing.ConfigurationParser;
+import org.infinispan.configuration.parsing.Namespace;
+import org.jboss.staxmapper.XMLExtendedStreamReader;
+
+import javax.xml.stream.XMLStreamException;
+
+/**
+ * @author Guillaume Scheibel <guillaume.scheibel@gmail.com>
+ */
+public class MongoDBConfigurationParser implements ConfigurationParser<ConfigurationBuilderHolder> {
+ @Override
+ public Namespace[] getSupportedNamespaces() {
+ return new Namespace[0]; //To change body of implemented methods use File | Settings | File Templates.
@Sanne

Sanne Nov 30, 2012

Owner

what's the point of this comment?

@Sanne Sanne commented on an outdated diff Nov 30, 2012

...ispan/loaders/mongodb/MongoDBConfigurationParser.java
+
+import javax.xml.stream.XMLStreamException;
+
+/**
+ * @author Guillaume Scheibel <guillaume.scheibel@gmail.com>
+ */
+public class MongoDBConfigurationParser implements ConfigurationParser<ConfigurationBuilderHolder> {
+ @Override
+ public Namespace[] getSupportedNamespaces() {
+ return new Namespace[0]; //To change body of implemented methods use File | Settings | File Templates.
+ }
+
+ @Override
+ public void readElement(XMLExtendedStreamReader xmlExtendedStreamReader, ConfigurationBuilderHolder configurationBuilderHolder)
+ throws XMLStreamException {
+ //To change body of implemented methods use File | Settings | File Templates.
@Sanne

Sanne Nov 30, 2012

Owner

Is this missing some implementation? If not, could you fix the comment and explain why the method should be empty?

@Sanne Sanne commented on an outdated diff Nov 30, 2012

...finispan/loaders/mongodb/configuration/Attribute.java
@@ -0,0 +1,76 @@
+/*
+ *
+ * * JBoss, Home of Professional Open Source
@Sanne

Sanne Nov 30, 2012

Owner

This header looks weird. it's double commented?

@Sanne Sanne commented on an outdated diff Nov 30, 2012

...ation/MongoDBCacheStoreConfigurationChildBuilder.java
@@ -0,0 +1,39 @@
+/*
+ *
+ * * JBoss, Home of Professional Open Source
@Sanne

Sanne Nov 30, 2012

Owner

header double-commented again

@Sanne Sanne commented on an outdated diff Nov 30, 2012

...ation/MongoDBCacheStoreConfigurationChildBuilder.java
+ * * Lesser General Public License for more details.
+ * *
+ * * You should have received a copy of the GNU Lesser General Public
+ * * License along with this software; if not, write to the Free
+ * * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ *
+ */
+
+package org.infinispan.loaders.mongodb.configuration;
+
+/**
+ * @author Guillaume Scheibel <guillaume.scheibel@gmail.com>
+ */
+public interface MongoDBCacheStoreConfigurationChildBuilder {
+ MongoDBCacheStoreConfigurationBuilder host(String host);
@Sanne

Sanne Nov 30, 2012

Owner

a little bit of spacing could make this class more pretty

@Sanne Sanne commented on an outdated diff Nov 30, 2012

.../java/org/infinispan/loaders/mongodb/logging/Log.java
@@ -0,0 +1,104 @@
+/*
+ * JBoss, Home of Professional Open Source.
+ * Copyright 2000 - 2011, Red Hat Middleware LLC, and individual contributors
@Sanne

Sanne Nov 30, 2012

Owner

Took you a long time to write :D

@Sanne Sanne commented on an outdated diff Nov 30, 2012

...java/org/infinispan/loaders/mongodb/package-info.java
@@ -0,0 +1,28 @@
+/*
+ * JBoss, Home of Professional Open Source
+ * Copyright 2010 Red Hat Inc. and/or its affiliates and other
+ * contributors as indicated by the @author tags. All rights reserved.
Owner

Sanne commented Jan 14, 2013

Hi @gscheibel , great steps forward! This looks like almost ready.

Could you rebase this all and squash it in a single commit? As I have some minor comments to do yet but it's hard to find where to comment exactly.

Then to be really perfectionist, these are the things I'd love you to check:

  • I think to have noticed some mixing of tabs and whitespace formatting. I'm not sure it might be an illusion from mis-formatted class: see for example the constructor of MongoDBCacheStoreConfig
  • there are changes in the root pom.xml of Infinispan which I don't think should be part of this patch. Maybe it's just whitespace changes?
  • MongoDBConfigurationParser has some IDE generated comments left over
  • Could you remove the log4j.properties file?

Could you write up a couple of paragraphs in the Infinispan documentation about how to use it, maybe with an example configuration if you think that could help?

Contributor

gscheibel commented Jan 17, 2013

@Sanne I've just perform the code style reformat and the squash, now I get started on the doc

Contributor

gscheibel commented Jan 25, 2013

@Sanne I was about to start the documentation on Confluence when I saw an email from Manik about a refactoring of the doc (probably moving to ascii doc). I think I should probably wait the new way of documenting instead of working on Confluence and then have to move to the new way.
WDYT ?

Owner

Sanne commented Jan 25, 2013

It would be great to have it in Confluence first as otherwise it will be delayed a lot. We'll be able to export from Confluence to ascii doc, so hopefully it will only requires some minor style / formatting.

Looking forward for the docs :)

Contributor

gscheibel commented Jan 29, 2013

@Sanne I don't have the right to add a page into the user guide. Which "workflow" should I follow, I submit a page doc to the team and someone will integrate it ? Should I ask someone (who ?) ?

Owner

Sanne commented Feb 7, 2013

@gscheibel this would be a great time to finally merge this in Infinispan: 5.2 was released and 5.3 will start release train very soon.
Will you have time to rebase this, change the pom to 5.3.0-SNAPSHOT and address Davide's comments?
We could then finally make clustered queries to work for OGM/MongoDB

Contributor

gscheibel commented Feb 13, 2013

@Sanne done. I have mainly refactored the exception management. WDYT ? (cc @DavideD)

@Sanne Sanne and 1 other commented on an outdated diff Feb 13, 2013

parent/pom.xml
@@ -160,6 +160,7 @@
<version.mc4j>1.2.6</version.mc4j>
<version.milyn.smooks>1.2.2</version.milyn.smooks>
<version.mockito>1.8.5</version.mockito>
+ <version.mongodb.driver>2.8.0</version.mongodb.driver>
@Sanne

Sanne Feb 13, 2013

Owner

I see OGM is using version 2.9.0 . Could we use the same?

@gscheibel

gscheibel Feb 13, 2013

Contributor

The latest version is 2.10.1.
In 2.10.0 they have added a new MongoClient class which replace the Mongo class. By default, the new one connects to the server with a WriteConcern at ACKNOWLEGEMENT. https://jira.mongodb.org/browse/JAVA-682
So should I implement this version (and in OGM as well ?)

@Sanne

Sanne Feb 13, 2013

Owner

Right in that case I think we should upgrade OGM too, also people will be happy that we will use the same WriteConcern as the driver by default: less confusing.
You choose if you have time to fix OGM first, for me it mostly important that we can keep using the same versions across this module and OGM, or we won't be able to use them together as we'd like to. Of course we won't always be able to keep them perfectly in sync but since you say they changed the API this looks like an urgent concern. Could you start by opening an OGM issue?

/cc @emmanuelbernard

@Sanne Sanne and 1 other commented on an outdated diff Feb 13, 2013

pom.xml
@@ -261,5 +261,94 @@ pageTracker._trackPageview();
</plugins>
</build>
</profile>
+ <!-- TODO somehow combine this with the 'distribution' profile so all docs are built together -->
@Sanne

Sanne Feb 13, 2013

Owner

Why are you adding this section about javadocs? I assume that's not intentional?

@gscheibel

gscheibel Feb 13, 2013

Contributor

I don't have added this line
It has been added the 4th of June 2009 (commit 11fa39) by Manik.
I think it was there when I have started to work on this cachestore then it has been deleted and the rebase function kept it.
I'll delete it

@Sanne

Sanne Feb 13, 2013

Owner

thanks for checking, yes I expected something like that. I would always suggest checking the output patch from git, it's a good occasion to review your own changes and verify - like in this case - that it's not including unintended or unrelated differences.

WFIW, I think the rebase function included this as you might have had some whitespace changes in some previous patch; I'm not sure of it but that might happen, so you could take this as an example of what happens when reformatting code / whitespace ;-)

@Sanne Sanne and 1 other commented on an outdated diff Feb 13, 2013

pom.xml
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
+ </profile>
+ <profile>
+ <id>mongodb</id>
+ <modules>
+ <module>cachestore/mongodb</module>
+ </modules>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <configuration>
@Sanne

Sanne Feb 13, 2013

Owner

could you explain what the idea is with the test group?

@gscheibel

gscheibel Feb 13, 2013

Contributor

The idea is to be able to don't run MongoDB tests if it's not needed. @tristantarrant suggested it in a comment above.

@Sanne

Sanne Feb 13, 2013

Owner

Ok saw that but we should have this automatically activated when the environment variables are set, as we do with Hibernate OGM.
Please copy the code from OGM and experiment with it?

@Sanne Sanne commented on an outdated diff Feb 13, 2013

cachestore/mongodb/pom.xml
@@ -0,0 +1,40 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <parent>
+ <artifactId>infinispan-cachestore-parent</artifactId>
+ <groupId>org.infinispan</groupId>
+ <version>5.2.0-SNAPSHOT</version>
@Sanne

Sanne Feb 13, 2013

Owner

The version is wrong.

Contributor

gscheibel commented Feb 13, 2013

@Sanne to activate the mongodb module you need to have MONGODB_HOSTNAME defined just like for OGM.
The safemode option doesn't exist anymore so you won't find it in the configruation section.
All tests are passing with version 2.0.2, 2.0.7 and 2.2.3 of the server.

Contributor

gscheibel commented Feb 14, 2013

@Sanne as discussed on OGM-267 (java driver upgrade), do you want me to implement a WriteConcern option on the cachestore ? If yes, what it the default value (to pick here: http://api.mongodb.org/java/current/com/mongodb/WriteConcern.html) ?

Owner

Sanne commented Feb 14, 2013

Good question. @maniksurtani ?

I think in case this is a non-shared CacheStore the reliability guarantee is not very important as there will be other replicas anyway, so 1 would do. But also in that case I would expect the user to not have actually installed any secondary node, so if MongoDB isn't too stupid the option 2 should have the same performance as 1 as there is no replication to perform.

So I think 2 would be safe, and give same benefits as 1. Also, lower values are nice to have but definitely not fit for as a default value.

Owner

Sanne commented Feb 18, 2013

@gscheibel are you sure you pushed the new version? I didn't find the latest changes discussed on IRC

Contributor

gscheibel commented Feb 18, 2013

@Sanne now I sure :)

how can I download full-version sources?

Contributor

gscheibel commented Feb 25, 2013

@verystrongjoe the full sources of the cachestore ? just fork my repo (or wait until it's merged onto the master)

@gscheibel thanks very much for answering my question. I'm not familiar with this github. You mean, If I push the fork button located in upper right part of screen, the sources, infinispan jdbc-cache-store mongo-db implementation, can be downloaed into my repo??

Contributor

gscheibel commented Feb 25, 2013

@verystrongjoe yes that's what I mean, but the mongodb cachestore code is, for the moment, only located on the ISPN-1797 branch of my own repo

@gscheibel I understood. How can I download your sources? I can't find the download button that download "mongodb" folders at one time. and I need to use your sources for using mongo-db in infinispan. thanks. :)

Contributor

gscheibel commented Feb 25, 2013

@verystrongjoe 2 ways, using git and clone the repo in local, or select the ISPN-1797 branch and click on the "zip" button (located on your project page)

@gscheibel thanks for source and help :)

Contributor

gscheibel commented Feb 25, 2013

@verystrongjoe my pleasure

@Sanne Sanne commented on the diff Mar 14, 2013

...org/infinispan/loaders/mongodb/MongoDBCacheStore.java
@@ -0,0 +1,307 @@
+/*
@Sanne

Sanne Mar 14, 2013

Owner

This file has the wrong line endings!
Please fix it as it corrupts all my scripts..

@Sanne Sanne commented on the diff Mar 14, 2013

...finispan/loaders/mongodb/MongoDBCacheStoreConfig.java
@@ -0,0 +1,131 @@
+/*
@Sanne

Sanne Mar 14, 2013

Owner

Also wrong line endings. Kill windows!

@Sanne

Sanne Mar 14, 2013

Owner

my fault sorry, I was checking an old version of your pull.
So my script broke in updating to your latest because of your previously shared wrong-line ending.. which just proves how dangerous that is :-)

@Sanne Sanne commented on the diff Mar 14, 2013

...org/infinispan/loaders/mongodb/MongoDBCacheStore.java
+ BasicDBObject dbObject = new BasicDBObject();
+ dbObject.put(ID_FIELD, key);
+ return dbObject;
+ }
+
+ @Override
+ public void fromStream(ObjectInput inputStream) throws CacheLoaderException {
+ Object objectFromStream;
+ try {
+ objectFromStream = getMarshaller().objectFromObjectStream(inputStream);
+ } catch (IOException e) {
+ throw log.unableToUnmarshall(e);
+ } catch (ClassNotFoundException e) {
+ throw log.unableToUnmarshall(e);
+ } catch (InterruptedException e) {
+ throw log.unableToUnmarshall(e);
@Sanne

Sanne Mar 14, 2013

Owner

when you catch an IE you have to restore the interrupted state of the Thread!
Add this before the throws statement:

Thread.currentThread().interrupt();

@Sanne Sanne commented on an outdated diff Mar 14, 2013

...org/infinispan/loaders/mongodb/MongoDBCacheStore.java
+ } else {
+ this.collection.remove(this.createDBObject(id));
+ return true;
+ }
+ }
+
+ private Object unmarshall(DBObject dbObject, String field) throws CacheLoaderException {
+ try {
+ return getMarshaller().objectFromByteBuffer((byte[]) dbObject.get(field));
+ } catch (IOException e) {
+ throw log.unableToUnmarshall(dbObject, e);
+ } catch (ClassNotFoundException e) {
+ throw log.unableToUnmarshall(dbObject, e);
+ }
+ }
+ private InternalCacheEntry createInternalCacheEntry(DBObject dbObject) throws CacheLoaderException {
@Sanne

Sanne Mar 14, 2013

Owner

seems worth to add a white line here

@Sanne Sanne commented on an outdated diff Mar 14, 2013

...org/infinispan/loaders/mongodb/MongoDBCacheStore.java
+ }
+ }
+ private InternalCacheEntry createInternalCacheEntry(DBObject dbObject) throws CacheLoaderException {
+ return (InternalCacheEntry) unmarshall(dbObject, VALUE_FIELD);
+ }
+
+ private byte[] objectToByteBuffer(Object key) throws CacheLoaderException {
+ try {
+ return getMarshaller().objectToByteBuffer(key);
+ } catch (IOException e) {
+ throw log.unableToUnmarshall(key, e);
+ } catch (InterruptedException e) {
+ throw log.unableToUnmarshall(key, e);
+ }
+ }
+ @Override
@Sanne

Sanne Mar 14, 2013

Owner

white line please

@Sanne Sanne and 2 others commented on an outdated diff Mar 14, 2013

...finispan/loaders/mongodb/MongoDBCacheStoreConfig.java
+ private static final Log log = LogFactory.getLog(MongoDBCacheStoreConfig.class, Log.class);
+ private String host;
+ private int port;
+ private int timeout;
+ private String username;
+ private String password;
+ private String database;
+ private String collection;
+ private int acknowledgment;
+ public MongoDBCacheStoreConfig() {
+ super.setCacheLoaderClassName(MongoDBCacheStore.class.getName());
+ }
+
+ public MongoDBCacheStoreConfig(String host, int port, int timeout, String username, String password, String database, String collection, int acknowledgment) {
+ if (port < 1 || port > 65535) {
+ log.mongoPortIllegalValue(String.valueOf(port));
@Sanne

Sanne Mar 14, 2013

Owner

should you not throw an exception here?

@Sanne

Sanne Mar 14, 2013

Owner

There is no need for String.valueOf() if you use the right formatter in the log message

@danberindei

danberindei Mar 19, 2013

Member

Just change the parameter type, %s works with anything...

@gscheibel

gscheibel Mar 20, 2013

Contributor

I used int instead of String because in MongoDBCacheStoreTest I use this method like this:

catch (NumberFormatException e) {
         throw log.mongoPortIllegalValue(configurationPort);
      }

and configurationPort is a String coming from System.getProperty()

So we have multiples choices:

  • override the mongoPortIllegalValue to have 1 signature with an int and one with a string (but the same message) I don't think it's the right choice.
  • change the parameter from String to Object
  • keep the call above with String.valueOf()

WDYT ?

@Sanne Sanne commented on an outdated diff Mar 14, 2013

...iguration/AbstractMongoDBCacheStoreConfiguration.java
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with builder software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+
+package org.infinispan.loaders.mongodb.configuration;
+
+/**
+ * @author Guillaume Scheibel <guillaume.scheibel@gmail.com>
+ */
+public class AbstractMongoDBCacheStoreConfiguration implements MongoDBCacheStoreConfigurationChildBuilder {
@Sanne

Sanne Mar 14, 2013

Owner

It would be nice to have some more javadocs on this class: people are going to use it and want to know how to configure it.

@Sanne Sanne commented on an outdated diff Mar 14, 2013

...finispan/loaders/mongodb/configuration/Attribute.java
+ PORT("port"),
+ TIMEOUT("timeout"),
+ USERNAME("username"),
+ PASSWORD("password"),
+ DATABASE("database"),
+ COLLECTION("collection"),
+ ACKNOWLEDGMENT("acknowledgment");
+
+ private final String name;
+
+ private Attribute(final String name) {
+ this.name = name;
+ }
+
+ /**
+ * Get the local name of this element.
@Sanne

Sanne Mar 14, 2013

Owner

can you elaborate on what a local name is?

@Sanne Sanne commented on an outdated diff Mar 14, 2013

...odb/configuration/MongoDBCacheStoreConfiguration.java
+package org.infinispan.loaders.mongodb.configuration;
+
+import org.infinispan.configuration.BuiltBy;
+import org.infinispan.configuration.cache.AbstractStoreConfiguration;
+import org.infinispan.configuration.cache.AsyncStoreConfiguration;
+import org.infinispan.configuration.cache.LegacyConfigurationAdaptor;
+import org.infinispan.configuration.cache.LegacyLoaderAdapter;
+import org.infinispan.configuration.cache.SingletonStoreConfiguration;
+import org.infinispan.loaders.mongodb.MongoDBCacheStoreConfig;
+import org.infinispan.util.TypedProperties;
+
+/**
+ * @author Guillaume Scheibel <guillaume.scheibel@gmail.com>
+ */
+@BuiltBy(MongoDBCacheStoreConfigurationBuilder.class)
+public class MongoDBCacheStoreConfiguration extends AbstractStoreConfiguration implements
@Sanne

Sanne Mar 14, 2013

Owner

Some more javadoc needed ;)

@Sanne Sanne commented on an outdated diff Mar 14, 2013

...figuration/MongoDBCacheStoreConfigurationBuilder.java
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+
+package org.infinispan.loaders.mongodb.configuration;
+
+import org.infinispan.configuration.cache.AbstractStoreConfigurationBuilder;
+import org.infinispan.configuration.cache.LoadersConfigurationBuilder;
+import org.infinispan.util.TypedProperties;
+
+/**
+ * @author Guillaume Scheibel <guillaume.scheibel@gmail.com>
+ */
+public class MongoDBCacheStoreConfigurationBuilder extends AbstractStoreConfigurationBuilder<MongoDBCacheStoreConfiguration,
@Sanne

Sanne Mar 14, 2013

Owner

Also javadoc needed

@Sanne Sanne and 1 other commented on an outdated diff Mar 14, 2013

...ation/MongoDBCacheStoreConfigurationChildBuilder.java
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+
+package org.infinispan.loaders.mongodb.configuration;
+
+/**
+ * @author Guillaume Scheibel <guillaume.scheibel@gmail.com>
+ */
+public interface MongoDBCacheStoreConfigurationChildBuilder {
@Sanne

Sanne Mar 14, 2013

Owner

javadoc..

@gscheibel

gscheibel Mar 20, 2013

Contributor

this interface is useful if multiple configuration builder are created (for multiple mongodb server versions) but for the moment, only 1 implementation has been created. So should we keep it ?

@tristantarrant tristantarrant and 1 other commented on an outdated diff Mar 14, 2013

...in/resources/schema/mongodb-cachestore-config-5.2.xsd
+ ~ Lesser General Public License for more details.
+ ~
+ ~ You should have received a copy of the GNU Lesser General Public
+ ~ License along with this software; if not, write to the Free
+ ~ Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ ~ 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ -->
+
+<xs:schema attributeFormDefault="unqualified"
+ elementFormDefault="qualified"
+ version="1.0"
+ targetNamespace="urn:infinispan:config:mongodb:5.2"
+ xmlns:tns="urn:infinispan:config:mongodb:5.2"
+ xmlns:config="urn:infinispan:config:5.2"
+ xmlns:xs="http://www.w3.org/2001/XMLSchema">
+<xs:import namespace="urn:infinispan:config:5.2"
@tristantarrant

tristantarrant Mar 14, 2013

Owner

This now needs to be 5.3

@gscheibel

gscheibel Mar 20, 2013

Contributor

The 5.3 xsd has not been published yet. Won't it cause any issue during the build (for example) ?

@tristantarrant

tristantarrant Mar 20, 2013

Owner

No, it is not used at build time

@Sanne Sanne commented on an outdated diff Mar 14, 2013

...infinispan/loaders/mongodb/MongoDBCacheStoreTest.java
+import org.infinispan.util.logging.LogFactory;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.Test;
+
+/**
+ * @author Guillaume Scheibel <guillaume.scheibel@gmail.com>
+ */
+@Test(testName = "loaders.remote.MongoDBCacheStoreTest", groups = "mongodb")
+public class MongoDBCacheStoreTest extends BaseCacheStoreTest {
+ private static final Log log = LogFactory.getLog(MongoDBCacheStoreTest.class, Log.class);
+
+ private MongoDBCacheStore cacheStore;
+ @Override
+ protected CacheStore createCacheStore() throws Exception {
+ String hostname = System.getProperty("MONGODB_HOSTNAME");
+ if(hostname == null || "".equals(hostname)){
@Sanne

Sanne Mar 14, 2013

Owner

After an if it would be nice to have a space. Also one before the {

@Sanne Sanne commented on an outdated diff Mar 14, 2013

...infinispan/loaders/mongodb/MongoDBCacheStoreTest.java
+
+ private MongoDBCacheStore cacheStore;
+ @Override
+ protected CacheStore createCacheStore() throws Exception {
+ String hostname = System.getProperty("MONGODB_HOSTNAME");
+ if(hostname == null || "".equals(hostname)){
+ hostname = "127.0.0.1";
+ }
+ int port = 27017;
+ String configurationPort = System.getProperty("MONGODB_PORT");
+ try {
+ if(configurationPort == null || "".equals(configurationPort)){
+ port = Integer.parseInt(configurationPort);
+ }
+ } catch(NumberFormatException e){
+ log.mongoPortIllegalValue(configurationPort);
@Sanne

Sanne Mar 14, 2013

Owner

I think you should throw an exception here

@tristantarrant tristantarrant commented on an outdated diff Mar 14, 2013

...in/resources/schema/mongodb-cachestore-config-5.2.xsd
+ </xs:annotation>
+ </xs:attribute>
+ <xs:attribute name="timeout" type="xs:int" default="2000">
+ <xs:annotation>
+ <xs:documentation>The timeout used by the MongoDB driver at the connection.</xs:documentation>
+ </xs:annotation>
+ </xs:attribute>
+ <xs:attribute name="acknowledgment" type="xs:int" default="1">
+ <xs:annotation>
+ <xs:documentation>The value used to configure the acknowledgment for write operation.</xs:documentation>
+ </xs:annotation>
+ </xs:attribute>
+ </xs:complexType>
+
+ <xs:complexType name="authentication">
+ <xs:attribute name="username" type="xs:string" default="">
@tristantarrant

tristantarrant Mar 14, 2013

Owner

is the default really an empty string ? if this attribute is optional, remove the default="". Otherwise, if the attribute is required, and requires an actual value, add use="required"

@tristantarrant tristantarrant commented on an outdated diff Mar 14, 2013

...in/resources/schema/mongodb-cachestore-config-5.2.xsd
+ </xs:annotation>
+ </xs:attribute>
+ <xs:attribute name="acknowledgment" type="xs:int" default="1">
+ <xs:annotation>
+ <xs:documentation>The value used to configure the acknowledgment for write operation.</xs:documentation>
+ </xs:annotation>
+ </xs:attribute>
+ </xs:complexType>
+
+ <xs:complexType name="authentication">
+ <xs:attribute name="username" type="xs:string" default="">
+ <xs:annotation>
+ <xs:documentation>The username used for the authentication with the MongoDB server.</xs:documentation>
+ </xs:annotation>
+ </xs:attribute>
+ <xs:attribute name="password" type="xs:string" default="">
@tristantarrant

tristantarrant Mar 14, 2013

Owner

same as above

@tristantarrant tristantarrant commented on an outdated diff Mar 14, 2013

...in/resources/schema/mongodb-cachestore-config-5.2.xsd
+
+ <xs:complexType name="authentication">
+ <xs:attribute name="username" type="xs:string" default="">
+ <xs:annotation>
+ <xs:documentation>The username used for the authentication with the MongoDB server.</xs:documentation>
+ </xs:annotation>
+ </xs:attribute>
+ <xs:attribute name="password" type="xs:string" default="">
+ <xs:annotation>
+ <xs:documentation>The password used for the authentication with the MongoDB server.</xs:documentation>
+ </xs:annotation>
+ </xs:attribute>
+ </xs:complexType>
+
+ <xs:complexType name="storage">
+ <xs:attribute name="database" type="xs:string" default="">
@tristantarrant

tristantarrant Mar 14, 2013

Owner

same as above

@tristantarrant tristantarrant commented on an outdated diff Mar 14, 2013

...in/resources/schema/mongodb-cachestore-config-5.2.xsd
+ </xs:annotation>
+ </xs:attribute>
+ <xs:attribute name="password" type="xs:string" default="">
+ <xs:annotation>
+ <xs:documentation>The password used for the authentication with the MongoDB server.</xs:documentation>
+ </xs:annotation>
+ </xs:attribute>
+ </xs:complexType>
+
+ <xs:complexType name="storage">
+ <xs:attribute name="database" type="xs:string" default="">
+ <xs:annotation>
+ <xs:documentation>The database used to store elements.</xs:documentation>
+ </xs:annotation>
+ </xs:attribute>
+ <xs:attribute name="collection" type="xs:string" default="">
@tristantarrant

tristantarrant Mar 14, 2013

Owner

same as above

@Sanne, I would like the MongoDBCacheStoreConfig constructor to return an exception if the port is not properly set (between 1 and 65535) but how to handle it in the caller ?
I can't rethrow it from adapt() so ?

Contributor

mmarkus commented May 21, 2013

thank you Guillaume, this is now integrated!

@mmarkus mmarkus closed this May 21, 2013

Contributor

gscheibel commented May 21, 2013

Great thanks.
If you want me to work on future evolution (for this cachestore) or to work
on others (like couchbase for instance), you know how to contact me :)

Cheers
Guillaume

2013/5/21 Mircea Markus notifications@github.com

thank you Guillaume, this is now integrated!


Reply to this email directly or view it on GitHubhttps://github.com/infinispan/infinispan/pull/1473#issuecomment-18213217
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment