http issue 216: DataStore based on String key and Serializable value

http issue 216: DataStore based on String key and Serializable value

https://code.google.com/p/google-http-java-client/issues/detail?id=216

Nick: careful review of everything
Eyal: design review

[ngmiceli, 2013-05-31 19:42:10]

Note that there are some comments that are written on only one implementation of
a datastore but apply to others as well.

The abstract testing mechanism used for the different datastore implementations
is very cool!

https://codereview.appspot.com/9029044/diff/11002/checkstyle-suppressions.xml
File checkstyle-suppressions.xml (right):

https://codereview.appspot.com/9029044/diff/11002/checkstyle-suppressions.xml...
checkstyle-suppressions.xml:18: <suppress checks="JavadocPackage"
files="google-http-client-test"/>
Please add why we shouldn't check this to the comment.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
File
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:48:
* Data Store API.
Add to documentation that it also uses the memcache API (enabled by default)

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:60:
public <V extends Serializable> TypedDataStore<V> of(String id) throws
IOException {
This class should keep a cache of all Typed datastores it's created so far, just
like the FileDataStore does. Otherwise, this is not typesafe as you could have
two instances of the typed datastore which both reference the same database. 
Same for all other datastores.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:90:
private final String kind;
Okay, this is neither a helpful name nor description :)
I'd either call it a "namespace", as the memcacheService calls it, or an "id"
like its called in TypedDataStore. And please expand the description slightly.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:130:
PreparedQuery queryResult = service.prepare(new Query(kind));
So long as we're getting every key+value pair, let's update the memcache as well
as a side-effect. We should document this side-effect in the javadoc.  We could
consider doing this as well in the keySet, but aren't convinced its a good idea.
Thoughts?

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:156:
if (result == null) {
This is tricky. If the user calls set(key, null) then we should delete key. But
what does it mean if the query has a null result? I'm not confident it means we
shouldn't cache the new result of null. Or no, because we said you cannot have a
value of null?

Just because our application specified you can't have null values in the
database doesn't mean someone wont use our library to access a database
populated by other means, where a null value is legal.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:159:
memcache.put(key, result);
By default this sets no expiration time. Should we perhaps expose that option to
the user through the Builder?

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:164:
return null;
Darn, its a shame we have to keep to the superclass' interface, as
EntityNotFoundException has notably more meaning as a result than null. Any
other way around this? Perhaps have our own wrapped Exception that the get
method can throw?
(Eyal) We should log it as well

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:206:
PreparedQuery queryResult = service.prepare(new Query(kind).setKeysOnly());
Use the keySet() method to get the keys instead of duplicating the query here.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:229:
/** Whether to disable the memcache (which is enabled by default). */
Our standard javadoc format is "The default value is {@code true}". Similarly
above for disableMemcache and in other files.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:232:
/** Returns whether to disable the memcache (which is enabled by default). */
No need to put the default value on the getter, just the setter (following what
we've done in other classes)

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
File
google-http-client-appengine/src/test/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreTest.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/test/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreTest.java:30:
public class AppEngineDataStoreTest extends AbstractDataStoreTest {
Is there a way we could modify the Abstract test to support testing both with
memcache on and off in the same file, as two separate test cases? I'ts a little
weird to have them separate.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/.pro...
File google-http-client-jdo/.project (right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/.pro...
google-http-client-jdo/.project:10: <arguments>
Remove <arguments>. Similarly below

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/pom.xml
File google-http-client-jdo/pom.xml (right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/pom....
google-http-client-jdo/pom.xml:62: <execution>
Eclipse gives the following error
Plugin execution not covered by lifecycle configuration:
org.datanucleus:maven-datanucleus-plugin:
 2.2.1:enhance (execution: default, phase: process-classes)
Note that it still builds fine using maven.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/pom....
google-http-client-jdo/pom.xml:116: <groupId>com.google.guava</groupId>
Remove dependency on guava.  If this was copied from another pom, check that one
as well.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
File
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:56:
JdoDataStore(PersistenceManagerFactory persistenceManagerFactory) {
public? 
Builder pattern?
Let's try to keep consistent

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:61:
public <V extends Serializable> TypedDataStore<V> of(String id) throws
IOException {
Check to ensure id isn't null or empty.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:66:
static class JdoTypedDataStore<V extends Serializable> extends TypedDataStore<V>
{
We feel awkward using one table but pretending it's multiple by using the "kind"
as an additional primary key. Better perhaps just to use separate tables for
each "kind"?

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:91:
Set<String> result = Sets.newTreeSet();
Do we need a TreeSet as opposed to a regular Set? What advantage are we giving
the user by sorting the output?

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:144:
if (key != null) {
Flip if. 

if (key == null) {
  return this;
}
// Rest of code...

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:218:
query.setRange(0, 1);
Do we need this? We're specifying both primary keys, so we should only ever get
one or zero result(s).

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:228:
static class JdoValue {
Class level comment please

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:240:
/** Byte array of value or {@code null} for none. */
I thought we didn't allow null values.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:244:
@SuppressWarnings("unused")
Comment why its needed if unused

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:271:
static class HideAsPackagePrivate {
First, while what we're doing is hiding as package private, couldn't we have a
better class name? Perhaps JdoDataStoreUtils or something?

Second, add the reason why we have to do this to the javadoc. Something
regarding:

Class com.google.api.client.extensions.jdo.JdoDataStore$JdoValue has been
specified with an object-id class
com.google.api.client.extensions.jdo.JdoDataStore$ComposedIdKey which is not
public. The ObjectId class MUST be public.

Optionally, lets split on the table, have only a single primary key, and get rid
of this class altogether.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
File
google-http-client-jdo/src/test/java/com/google/api/client/extensions/jdo/JdoDataStoreTest.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/test/java/com/google/api/client/extensions/jdo/JdoDataStoreTest.java:41:
create database mytest
semicolon at end of line

https://codereview.appspot.com/9029044/diff/11002/google-http-client-test/src...
File
google-http-client-test/src/main/java/com/google/api/client/test/util/AbstractDataStoreTest.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client-test/src...
google-http-client-test/src/main/java/com/google/api/client/test/util/AbstractDataStoreTest.java:171:
int size = 100;
[optional] Add a zero! We want to test LARGE, not medium :)
From testing on my machine, changing this from 100 -> 1000 adds 5 seconds
without the JdoDataStoreTest, and a good 30-45 seconds with it.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/.classpath
File google-http-client/.classpath (right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client/.classpa...
google-http-client/.classpath:6: <classpathentry kind="con"
path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/jdk6-google-v4-64"/>
Revert this change

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
File google-http-client/src/main/java/com/google/api/client/util/IOUtils.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:153: *
Serializes the given byte array into to a newly allocated object.
Serializes -> Deserializes. Same below

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:202: }
catch (ClassNotFoundException e) {
e -> exception for consistency

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:204: }
catch (IllegalArgumentException exception) {
Rather than all these empty catch statements, just catch IO and rethrow, then
catch Exception with //handled below for readability:

} catch (IOException e) {
  throw e;
} catch (Exception e) {
  // handled below
}

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java:42:
public abstract class DataStore {
We feel that the TypedDataStore is really the DataStore, and that this class
should be renamed to something like (Eyal) "DataStoreFactory" or (Nick)
"DataStoreAccessor"

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java:56:
public abstract <V extends Serializable> TypedDataStore<V> of(String id) throws
IOException;
I think both the name and javadoc description of this method seem to imply that
calling "of" creates a new TypedDataStore for a given type, which makes the need
of the ID confusing. Perhaps a more straightforward (though less aesthetically
pleasing) name such as "getTypedDataStore" or "fetchTypedDataStore" would be
more clear? Also, mention explicitly that the implementation should return a
datastore with a matching ID if it exists, or else it will return a new instance
(or whatever the exact expected behavior is). It's good to regulate and mandate
this general behavior from the interface level (abstract class) anyway.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStore.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStore.java:55:
/** Store of memory persisted credentials, indexed by userId. */
This comment doesn't make sense in this scope. Please correct. Same below on the
other store object which has the exact same comment.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStore.java:110:
private final SortedMap<String, byte[]> store;
Let's not call this "store" as well; that's just confusing. Why not "cache"?

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStore.java:190:
public FileTypedDataStore<V> clear() throws IOException {
Don't we want to save after clear as well? I'm not sure which more sense, an
empty file or deleting the file, but it should definitely do one or the other.
Test this as well.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/util/store/MemoryDataStore.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/MemoryDataStore.java:48:
private final Map<String, MemoryTypedDataStore<?>> store = Maps.newHashMap();
ditto comment ("credentials"?)
Should be renamed to avoid duplicate naming

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/util/store/TypedDataStore.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/TypedDataStore.java:34:
* @param <V> value type
Can we elaborate? This doesn't tell us anything we couldn't know just from
looking at <V>

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/TypedDataStore.java:40:
public abstract class TypedDataStore<V extends Serializable> {
Eyal thinks we should implement the Map<String, V> interface rather than
duplicating essentially the same API.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/TypedDataStore.java:43:
private final DataStore dataStore;
What is the value of having a reference back to the dataStore?

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/TypedDataStore.java:85:
public abstract TypedDataStore<V> set(String key, V value) throws IOException;
We noticed ReentrantLock is used in most if not all implementations. Perhaps we
could just enforce the lock usage by using it like so:

public TypedDataStore<V> set(String key, V value) throws IOException {
  lock.lock();
  try {
      return setAction(key, value); // This is the real abstract class to be
implemented
  } finally {
    lock.unlock();
  }
}

public abstract TypedDataStore<V> setAction(String key, V value) throws
IOException;

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/TypedDataStore.java:123:
* also the default implementation.
Document what happens if the key doesn't exist. Does it just return itself,
unmodified? Does it throw an exception?

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/util/store/package-info.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/package-info.java:16:
* Utilities to store serializable data.
This is a new package, so let's introduce it as @Beta

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/test...
File
google-http-client/src/test/java/com/google/api/client/util/IOUtilsTest.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/util/IOUtilsTest.java:40:
public void testIsSymbolicLink() throws IOException {
Please test the assertTrue case as well

https://codereview.appspot.com/9029044/diff/11002/jdiff.xml
File jdiff.xml (right):

https://codereview.appspot.com/9029044/diff/11002/jdiff.xml#newcode1
jdiff.xml:1: <?xml version="1.0" encoding="UTF-8"?>
hg rm this file. I automatically generate jdiff as part of the release process.
I thought I removed them all but I must have missed http in the default branch.

[yanivi, 2013-06-11 20:46:18]

I responded to everything except the separate database tables issue for JDO
which I listed as "TODO(yanivi)".  Let's meet to discuss these comments in
detail tomorrow morning.

https://codereview.appspot.com/9029044/diff/11002/checkstyle-suppressions.xml
File checkstyle-suppressions.xml (right):

https://codereview.appspot.com/9029044/diff/11002/checkstyle-suppressions.xml...
checkstyle-suppressions.xml:18: <suppress checks="JavadocPackage"
files="google-http-client-test"/>
On 2013/05/31 19:42:10, ngmiceli wrote:
> Please add why we shouldn't check this to the comment.

Reverted.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
File
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:48:
* Data Store API.
On 2013/05/31 19:42:10, ngmiceli wrote:
> Add to documentation that it also uses the memcache API (enabled by default)

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:60:
public <V extends Serializable> TypedDataStore<V> of(String id) throws
IOException {
On 2013/05/31 19:42:10, ngmiceli wrote:
> This class should keep a cache of all Typed datastores it's created so far,
just
> like the FileDataStore does. Otherwise, this is not typesafe as you could have
> two instances of the typed datastore which both reference the same database. 
> Same for all other datastores. 

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:90:
private final String kind;
On 2013/05/31 19:42:10, ngmiceli wrote:
> Okay, this is neither a helpful name nor description :)
> I'd either call it a "namespace", as the memcacheService calls it, or an "id"
> like its called in TypedDataStore. And please expand the description slightly.


Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:130:
PreparedQuery queryResult = service.prepare(new Query(kind));
On 2013/05/31 19:42:10, ngmiceli wrote:
> So long as we're getting every key+value pair, let's update the memcache as
well
> as a side-effect. We should document this side-effect in the javadoc.  We
could
> consider doing this as well in the keySet, but aren't convinced its a good
idea.
> Thoughts?

Done, except not for keySet because that's keyOnly()

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:156:
if (result == null) {
On 2013/05/31 19:42:10, ngmiceli wrote:
> This is tricky. If the user calls set(key, null) then we should delete key.
But
> what does it mean if the query has a null result? I'm not confident it means
we
> shouldn't cache the new result of null. Or no, because we said you cannot have
a
> value of null?

Right, because you cannot have a null value.

> 
> Just because our application specified you can't have null values in the
> database doesn't mean someone wont use our library to access a database
> populated by other means, where a null value is legal.

Null value is not legal.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:159:
memcache.put(key, result);
On 2013/05/31 19:42:10, ngmiceli wrote:
> By default this sets no expiration time. Should we perhaps expose that option
to
> the user through the Builder?

TODO added

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:164:
return null;
On 2013/05/31 19:42:10, ngmiceli wrote:
> Darn, its a shame we have to keep to the superclass' interface, as
> EntityNotFoundException has notably more meaning as a result than null. Any
> other way around this? Perhaps have our own wrapped Exception that the get
> method can throw?
> (Eyal) We should log it as well

null means that there is no key-value pair, just like it does with the get
method in Map.  I don't see how this is vague, nor do I agree that throwing an
exception is the right behavior for what is a normal codepath.  Exceptions
should really be used for exceptional cases.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:206:
PreparedQuery queryResult = service.prepare(new Query(kind).setKeysOnly());
On 2013/05/31 19:42:10, ngmiceli wrote:
> Use the keySet() method to get the keys instead of duplicating the query here.

I shared the little bit of code that was duplicated in a new query(boolean)
function.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:229:
/** Whether to disable the memcache (which is enabled by default). */
On 2013/05/31 19:42:10, ngmiceli wrote:
> Our standard javadoc format is "The default value is {@code true}". Similarly
> above for disableMemcache and in other files.

I think specifying the default value in the first sentence is useful.  Cleaned
it up a bit to match more of our style.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStore.java:232:
/** Returns whether to disable the memcache (which is enabled by default). */
On 2013/05/31 19:42:10, ngmiceli wrote:
> No need to put the default value on the getter, just the setter (following
what
> we've done in other classes)

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
File
google-http-client-appengine/src/test/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreTest.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client-appengin...
google-http-client-appengine/src/test/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreTest.java:30:
public class AppEngineDataStoreTest extends AbstractDataStoreTest {
On 2013/05/31 19:42:10, ngmiceli wrote:
> Is there a way we could modify the Abstract test to support testing both with
> memcache on and off in the same file, as two separate test cases? I'ts a
little
> weird to have them separate.

I cannot think of any useful way to do that, but let me know if you think of
anything.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/.pro...
File google-http-client-jdo/.project (right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/.pro...
google-http-client-jdo/.project:10: <arguments>
On 2013/05/31 19:42:10, ngmiceli wrote:
> Remove <arguments>. Similarly below

That's the default that Eclipse generates, so I'd prefer not to change it. 
Anyway, if we did, we should do it for all projects, because they are all using
it this way.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/pom.xml
File google-http-client-jdo/pom.xml (right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/pom....
google-http-client-jdo/pom.xml:62: <execution>
On 2013/05/31 19:42:10, ngmiceli wrote:
> Eclipse gives the following error
> Plugin execution not covered by lifecycle configuration:
> org.datanucleus:maven-datanucleus-plugin:
>  2.2.1:enhance (execution: default, phase: process-classes)
> Note that it still builds fine using maven.

For some reason I don't see that.  Sometimes I see these warnings, and sometimes
not.  Strange.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/pom....
google-http-client-jdo/pom.xml:116: <groupId>com.google.guava</groupId>
On 2013/05/31 19:42:10, ngmiceli wrote:
> Remove dependency on guava.  If this was copied from another pom, check that
one
> as well.

This is intentional because it helps with testing in Eclipse to avoid the
ClassNotFoundException.  The upside is that it has <scope>test</scope> so it's
fine anyway.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
File
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:56:
JdoDataStore(PersistenceManagerFactory persistenceManagerFactory) {
On 2013/05/31 19:42:10, ngmiceli wrote:
> public? 

Done.

> Builder pattern?
> Let's try to keep consistent

The style is to not use a Builder pattern if there are no optional parameters.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:61:
public <V extends Serializable> TypedDataStore<V> of(String id) throws
IOException {
On 2013/05/31 19:42:10, ngmiceli wrote:
> Check to ensure id isn't null or empty.

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:66:
static class JdoTypedDataStore<V extends Serializable> extends TypedDataStore<V>
{
On 2013/05/31 19:42:10, ngmiceli wrote:
> We feel awkward using one table but pretending it's multiple by using the
"kind"
> as an additional primary key. Better perhaps just to use separate tables for
> each "kind"?

TODO(yanivi)

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:91:
Set<String> result = Sets.newTreeSet();
On 2013/05/31 19:42:10, ngmiceli wrote:
> Do we need a TreeSet as opposed to a regular Set? What advantage are we giving
> the user by sorting the output?

A predictable order of output is sometimes important, e.g. if you are outputting
something as a result of that information, or if you are checking the result
against an expected output such as in unit tests.  I don't know if sorting it is
necessarily the right thing to do, but that's what I've implemented.  I changed
the JavaDoc of keySet() accordingly.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:144:
if (key != null) {
On 2013/05/31 19:42:10, ngmiceli wrote:
> Flip if. 
> 
> if (key == null) {
>   return this;
> }
> // Rest of code...

I think this is getting kind of nitpicky.  It's just a different style
preference.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:218:
query.setRange(0, 1);
On 2013/05/31 19:42:10, ngmiceli wrote:
> Do we need this? We're specifying both primary keys, so we should only ever
get
> one or zero result(s).

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:228:
static class JdoValue {
On 2013/05/31 19:42:10, ngmiceli wrote:
> Class level comment please

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:240:
/** Byte array of value or {@code null} for none. */
On 2013/05/31 19:42:10, ngmiceli wrote:
> I thought we didn't allow null values.

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:244:
@SuppressWarnings("unused")
On 2013/05/31 19:42:10, ngmiceli wrote:
> Comment why its needed if unused

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStore.java:271:
static class HideAsPackagePrivate {
On 2013/05/31 19:42:10, ngmiceli wrote:
> First, while what we're doing is hiding as package private, couldn't we have a
> better class name? Perhaps JdoDataStoreUtils or something?

Renamed to PrivateUtils.  Better?

> 
> Second, add the reason why we have to do this to the javadoc. Something
> regarding:
> 
> Class com.google.api.client.extensions.jdo.JdoDataStore$JdoValue has been
> specified with an object-id class
> com.google.api.client.extensions.jdo.JdoDataStore$ComposedIdKey which is not
> public. The ObjectId class MUST be public.

Done.

> 
> Optionally, lets split on the table, have only a single primary key, and get
rid
> of this class altogether.

TODO(yanivi)

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
File
google-http-client-jdo/src/test/java/com/google/api/client/extensions/jdo/JdoDataStoreTest.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client-jdo/src/...
google-http-client-jdo/src/test/java/com/google/api/client/extensions/jdo/JdoDataStoreTest.java:41:
create database mytest
On 2013/05/31 19:42:10, ngmiceli wrote:
> semicolon at end of line

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client-test/src...
File
google-http-client-test/src/main/java/com/google/api/client/test/util/AbstractDataStoreTest.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client-test/src...
google-http-client-test/src/main/java/com/google/api/client/test/util/AbstractDataStoreTest.java:171:
int size = 100;
On 2013/05/31 19:42:10, ngmiceli wrote:
> [optional] Add a zero! We want to test LARGE, not medium :)
> From testing on my machine, changing this from 100 -> 1000 adds 5 seconds
> without the JdoDataStoreTest, and a good 30-45 seconds with it.

Yeah, originally this was much bigger, but I removed the zeros because of
JdoDataStoreTest.  Added a TODO for now.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/.classpath
File google-http-client/.classpath (right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client/.classpa...
google-http-client/.classpath:6: <classpathentry kind="con"
path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/jdk6-google-v4-64"/>
On 2013/05/31 19:42:10, ngmiceli wrote:
> Revert this change

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
File google-http-client/src/main/java/com/google/api/client/util/IOUtils.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:153: *
Serializes the given byte array into to a newly allocated object.
On 2013/05/31 19:42:10, ngmiceli wrote:
> Serializes -> Deserializes. Same below

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:202: }
catch (ClassNotFoundException e) {
On 2013/05/31 19:42:10, ngmiceli wrote:
> e -> exception for consistency

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/IOUtils.java:204: }
catch (IllegalArgumentException exception) {
On 2013/05/31 19:42:10, ngmiceli wrote:
> Rather than all these empty catch statements, just catch IO and rethrow, then
> catch Exception with //handled below for readability:
> 
> } catch (IOException e) {
>   throw e;
> } catch (Exception e) {
>   // handled below
> }

Catching runtime exceptions is bad practice.  Note that RuntimeException extends
Exception.  So I think the way I wrote the code is probably the simplest way to
deal with that reality.

I did notice a bug in the way I handled InvocationTargetException, so I fixed
that.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java:42:
public abstract class DataStore {
On 2013/05/31 19:42:10, ngmiceli wrote:
> We feel that the TypedDataStore is really the DataStore, and that this class
> should be renamed to something like (Eyal) "DataStoreFactory" or (Nick)
> "DataStoreAccessor" 

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java:56:
public abstract <V extends Serializable> TypedDataStore<V> of(String id) throws
IOException;
On 2013/05/31 19:42:10, ngmiceli wrote:
> I think both the name and javadoc description of this method seem to imply
that
> calling "of" creates a new TypedDataStore for a given type, which makes the
need
> of the ID confusing. Perhaps a more straightforward (though less aesthetically
> pleasing) name such as "getTypedDataStore" or "fetchTypedDataStore" would be
> more clear? Also, mention explicitly that the implementation should return a
> datastore with a matching ID if it exists, or else it will return a new
instance
> (or whatever the exact expected behavior is). It's good to regulate and
mandate
> this general behavior from the interface level (abstract class) anyway.

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStore.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStore.java:55:
/** Store of memory persisted credentials, indexed by userId. */
On 2013/05/31 19:42:10, ngmiceli wrote:
> This comment doesn't make sense in this scope. Please correct. Same below on
the
> other store object which has the exact same comment.

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStore.java:110:
private final SortedMap<String, byte[]> store;
On 2013/05/31 19:42:10, ngmiceli wrote:
> Let's not call this "store" as well; that's just confusing. Why not "cache"?

How about keyValueMap?

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStore.java:190:
public FileTypedDataStore<V> clear() throws IOException {
On 2013/05/31 19:42:10, ngmiceli wrote:
> Don't we want to save after clear as well? I'm not sure which more sense, an
> empty file or deleting the file, but it should definitely do one or the other.
> Test this as well.

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/util/store/MemoryDataStore.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/MemoryDataStore.java:48:
private final Map<String, MemoryTypedDataStore<?>> store = Maps.newHashMap();
On 2013/05/31 19:42:10, ngmiceli wrote:
> ditto comment ("credentials"?)
> Should be renamed to avoid duplicate naming

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/util/store/TypedDataStore.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/TypedDataStore.java:34:
* @param <V> value type
On 2013/05/31 19:42:10, ngmiceli wrote:
> Can we elaborate? This doesn't tell us anything we couldn't know just from
> looking at <V>

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/TypedDataStore.java:40:
public abstract class TypedDataStore<V extends Serializable> {
On 2013/05/31 19:42:10, ngmiceli wrote:
> Eyal thinks we should implement the Map<String, V> interface rather than
> duplicating essentially the same API.

Let's discuss this.  The Map interface is quite complex, including EntrySet. 
None of its method throws IOException, so we would have to wrap that in a
RuntimeException which would suck.  There also some subtle differences, such as
the way to treat null value is meaning "delete" instead of assigning the key a
null value.  One interesting question is whether we should implement entrySet()
which can be useful to users, but would take some work to implement properly.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/TypedDataStore.java:43:
private final DataStore dataStore;
On 2013/05/31 19:42:10, ngmiceli wrote:
> What is the value of having a reference back to the dataStore?

I think in general it is convenient to have a reference to factory via a
getDataStore() method.  It's just like the way JsonParser has a getFactory()
method, which is occasionally convenient.  But if you really feel strongly
against it, I can remove it.  Is there any real cost to having the
getDataStore() method?

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/TypedDataStore.java:85:
public abstract TypedDataStore<V> set(String key, V value) throws IOException;
On 2013/05/31 19:42:10, ngmiceli wrote:
> We noticed ReentrantLock is used in most if not all implementations. Perhaps
we
> could just enforce the lock usage by using it like so:
> 
> public TypedDataStore<V> set(String key, V value) throws IOException {
>   lock.lock();
>   try {
>       return setAction(key, value); // This is the real abstract class to be
> implemented
>   } finally {
>     lock.unlock();
>   }
> }
> 
> public abstract TypedDataStore<V> setAction(String key, V value) throws
> IOException;

I think it is dangerous to implement the locking mechanism inside the abstract
superclass.  You are left with either two options: share locking mechanism with
your subclass with potentially negative consequences (see Effective Java), or
hiding the locking mechanism from the subclass which means the subclass cannot
access the lock to access its own internal implementation details.  I'd rather
live with minor code duplication.

I agree that it feels non-ideal.  Maybe we'll come up with a better solution in
the future.

I did do one optimization in that MemoryDataStore and FileDataStore share a lot
of logic, so I created AbstractMemoryDataStore for them to share.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/TypedDataStore.java:123:
* also the default implementation.
On 2013/05/31 19:42:10, ngmiceli wrote:
> Document what happens if the key doesn't exist. Does it just return itself,
> unmodified? Does it throw an exception?

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/util/store/package-info.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/store/package-info.java:16:
* Utilities to store serializable data.
On 2013/05/31 19:42:10, ngmiceli wrote:
> This is a new package, so let's introduce it as @Beta

Done.

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/test...
File
google-http-client/src/test/java/com/google/api/client/util/IOUtilsTest.java
(right):

https://codereview.appspot.com/9029044/diff/11002/google-http-client/src/test...
google-http-client/src/test/java/com/google/api/client/util/IOUtilsTest.java:40:
public void testIsSymbolicLink() throws IOException {
On 2013/05/31 19:42:10, ngmiceli wrote:
> Please test the assertTrue case as well

Done.

https://codereview.appspot.com/9029044/diff/11002/jdiff.xml
File jdiff.xml (right):

https://codereview.appspot.com/9029044/diff/11002/jdiff.xml#newcode1
jdiff.xml:1: <?xml version="1.0" encoding="UTF-8"?>
On 2013/05/31 19:42:10, ngmiceli wrote:
> hg rm this file. I automatically generate jdiff as part of the release
process.
> I thought I removed them all but I must have missed http in the default
branch.

Done.

[yanivi, 2013-06-12 19:12:19]

I looked into the possibility of a pure JDBC implementation implementation of a
DataStore and it turned out to be just too difficult for me to figure out.  I
think we should leave that out for a future feature request.  So for now I think
we should just live with the JDO implementation which we know works.  It's
possibly not as efficient because it has to use a single database table, but it
should be good enough.

[peleyal, 2013-06-13 15:51:17]

Great job!
I have a lot of comments, but almost all of them are regarding to better
documentation.

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
File
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:112:
AppEngineDataStore(AppEngineDataStoreFactory dataStore, String id) {
change the name of the parameter. dataStoreFactory

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:131:
result.add(entity.getKey().getName());
here and in other places, I didn't test it - but do you get the right keys here?
key in app engine is define by id and key (so here you should get only the key
part) - I'll explain it better when we will sit

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:145:
}
[optional but worth document this]
what happen if the memache already contain all the keys-values and lets say that
their size 1000. It's very inefficient to clear and add all. can we have a delta
and only add \ remove the delta?

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:178:
V result = deserialize(service.get(KeyFactory.createKey(getId(), key)));
[optional] here and in other places, you declare the service variable to use it
only once (here for the get), so maybe it will be better just to use
DatastoreServiceFactory.getDataStoreService inline... (and not declaring a
variable for it). I'm not sure it's the right way to do it, just something to
think about.

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:183:
} catch (EntityNotFoundException exception) {
[optional]
can you minimize the try-catch scope here, as I understand the
EntityNotFoundException can be thrown from servier.get, right? so I think the
following code is more readable (in a first look I didn't understand from where
the EntityNotFoundException can be thrown)

          object obj = null;
          try {
            obj = service.get(KeyFactory.createKey(getId(), key);
          } catch (EntityNotFoundException exception) {
            if (memcache != null) {
              memcache.delete(key);
            }
            return null;
          }
          V result = deserialize(obj));
          ...

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:238:
List<Key> keys = Lists.newArrayList();
worth adding a comment that we can't delete all keys in the app engine
datastore, so we query all keys and then delete all of them

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:323:
* @return this instance
[optional]
in most places you omit this return statement (for returning this, should we
omit it here as well?)

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
File
google-http-client-appengine/src/test/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactoryTest.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/test/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactoryTest.java:34:
public void setUp() throws Exception {
why did you change IOException here to Exception?

https://codereview.appspot.com/9029044/diff/107002/google-http-client-jdo/src...
File
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStoreFactory.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client-jdo/src...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStoreFactory.java:215:
* Returns a new id query.
in most places I saw that you wrote ID... please fix here and in other places

https://codereview.appspot.com/9029044/diff/107002/google-http-client-jdo/src...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStoreFactory.java:246:
Query query = persistenceManager.newQuery(JdoValue.class);
what about closing the query. I saw that you are doing that in other places, why
not here as well?

https://codereview.appspot.com/9029044/diff/107002/google-http-client-jdo/src...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStoreFactory.java:329:
StringTokenizer token = new StringTokenizer(value, "::");
can you add explanation what we are trying to achieve here?

https://codereview.appspot.com/9029044/diff/107002/google-http-client-jdo/src...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStoreFactory.java:344:
return key.equals(c.key) && id.equals(c.id);
I think Objects.equals is better here... what if id or key is null. is it
possible that they will be null?

https://codereview.appspot.com/9029044/diff/107002/google-http-client-test/sr...
File
google-http-client-test/src/main/java/com/google/api/client/test/util/store/AbstractDataStoreFactoryTest.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client-test/sr...
google-http-client-test/src/main/java/com/google/api/client/test/util/store/AbstractDataStoreFactoryTest.java:45:
public void setUp() throws Exception {
IOExpcetion?

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/AbstractDataStore.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/AbstractDataStore.java:49:
this.dataStoreFactory = Preconditions.checkNotNull(dataStoreFactory);
[optional] change the order first parameter is dataStoreFactory so keep the
first statement dataStoreFactory as well

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/AbstractDataStoreFactory.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/AbstractDataStoreFactory.java:44:
private static final Pattern ID_PATTERN = Pattern.compile("\\w{1,30}");
please add a comment and explain here also why we need it (I know you had a
comment on DataStoreFactoy that Id can't be more than 30 chars)

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java:56:
boolean containsValue(V value) throws IOException;
do we need that one?
It looks weird and I didn't see something close to it in general Map interface

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java:59:
* Returns the unmodifiable sorted set of all stored keys.
I think we agreed that it won't be sorted

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java:62:
* Keys must be sorted in ascending order.
ditto

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java:73:
* @param key key or {@code null} for {@code null} result
you don't allowed nulls any more, right?

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java:97:
* @param key key or {@code null} to ignore
ditto

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/DataStoreFactory.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStoreFactory.java:28:
* Users should keep a single globally shared instance of the data store factory.
[optional] should we explain that otherwise things may not work (like memcache
on AppEngine, or just two memory data store, with different memory...)

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStoreFactory.java:48:
* If a data store by that ID does not already exist, it should be created now
and stored for
created now and stored for later access. Otherwise
--->
created now, stored for later access and be returned. Otherwise...

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/tes...
File
google-http-client/src/test/java/com/google/api/client/util/IOUtilsTest.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/tes...
google-http-client/src/test/java/com/google/api/client/util/IOUtilsTest.java:48:
File file2 = new File(file.getCanonicalPath() + "2");
you should use file.deleteOnExit here as well.
for both files, right?

https://codereview.appspot.com/9029044/diff/107002/pom.xml
File pom.xml (right):

https://codereview.appspot.com/9029044/diff/107002/pom.xml#newcode215
pom.xml:215: <dependency>
why are you adding all those dependencies?
do we need them?

[yanivi, 2013-06-13 16:35:08]

In addition, I added getDefaultInstance() to AppEngineDataStoreFactory,
MemoryDataStoreFactory, GsonFactory, and JacksonFactory.  I need those for the
API project.  Please review that also.

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
File
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:112:
AppEngineDataStore(AppEngineDataStoreFactory dataStore, String id) {
On 2013/06/13 15:51:17, peleyal wrote:
> change the name of the parameter. dataStoreFactory

Done.

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:131:
result.add(entity.getKey().getName());
On 2013/06/13 15:51:17, peleyal wrote:
> here and in other places, I didn't test it - but do you get the right keys
here?
> key in app engine is define by id and key (so here you should get only the key
> part) - I'll explain it better when we will sit

Yes, it works because the name is the key.

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:145:
}
On 2013/06/13 15:51:17, peleyal wrote:
> [optional but worth document this]
> what happen if the memache already contain all the keys-values and lets say
that
> their size 1000. It's very inefficient to clear and add all. can we have a
delta
> and only add \ remove the delta?

No, we cannot.  Added a comment to explain.

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:178:
V result = deserialize(service.get(KeyFactory.createKey(getId(), key)));
On 2013/06/13 15:51:17, peleyal wrote:
> [optional] here and in other places, you declare the service variable to use
it
> only once (here for the get), so maybe it will be better just to use
> DatastoreServiceFactory.getDataStoreService inline... (and not declaring a
> variable for it). I'm not sure it's the right way to do it, just something to
> think about.

I made it a final variable just to make this simpler.

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:183:
} catch (EntityNotFoundException exception) {
On 2013/06/13 15:51:17, peleyal wrote:
> [optional]
> can you minimize the try-catch scope here, as I understand the
> EntityNotFoundException can be thrown from servier.get, right? so I think the
> following code is more readable (in a first look I didn't understand from
where
> the EntityNotFoundException can be thrown)
> 
>           object obj = null;
>           try {
>             obj = service.get(KeyFactory.createKey(getId(), key);
>           } catch (EntityNotFoundException exception) {
>             if (memcache != null) {
>               memcache.delete(key);
>             }
>             return null;
>           }
>           V result = deserialize(obj));
>           ...

Done.

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:238:
List<Key> keys = Lists.newArrayList();
On 2013/06/13 15:51:17, peleyal wrote:
> worth adding a comment that we can't delete all keys in the app engine
> datastore, so we query all keys and then delete all of them

Done.

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:323:
* @return this instance
On 2013/06/13 15:51:17, peleyal wrote:
> [optional]
> in most places you omit this return statement (for returning this, should we
> omit it here as well?)

I think it's nice.  What do you think?

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
File
google-http-client-appengine/src/test/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactoryTest.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/test/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactoryTest.java:34:
public void setUp() throws Exception {
On 2013/06/13 15:51:17, peleyal wrote:
> why did you change IOException here to Exception?

because I needed to do it for AbstractDataStoreFactoryTest.setUp() because the
SqlDataStoreFactory I was playing around with can throw SqlException in
SqlDataStoreFactory() constructor, which isn't an IOException.

https://codereview.appspot.com/9029044/diff/107002/google-http-client-jdo/src...
File
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStoreFactory.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client-jdo/src...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStoreFactory.java:215:
* Returns a new id query.
On 2013/06/13 15:51:17, peleyal wrote:
> in most places I saw that you wrote ID... please fix here and in other places

Done.

https://codereview.appspot.com/9029044/diff/107002/google-http-client-jdo/src...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStoreFactory.java:246:
Query query = persistenceManager.newQuery(JdoValue.class);
On 2013/06/13 15:51:17, peleyal wrote:
> what about closing the query. I saw that you are doing that in other places,
why
> not here as well?

Done.

https://codereview.appspot.com/9029044/diff/107002/google-http-client-jdo/src...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStoreFactory.java:329:
StringTokenizer token = new StringTokenizer(value, "::");
On 2013/06/13 15:51:17, peleyal wrote:
> can you add explanation what we are trying to achieve here?

See the comment at the class level.  It is just copied from that reference.

https://codereview.appspot.com/9029044/diff/107002/google-http-client-jdo/src...
google-http-client-jdo/src/main/java/com/google/api/client/extensions/jdo/JdoDataStoreFactory.java:344:
return key.equals(c.key) && id.equals(c.id);
On 2013/06/13 15:51:17, peleyal wrote:
> I think Objects.equals is better here... what if id or key is null. is it
> possible that they will be null?

No, null is not possible.

https://codereview.appspot.com/9029044/diff/107002/google-http-client-test/sr...
File
google-http-client-test/src/main/java/com/google/api/client/test/util/store/AbstractDataStoreFactoryTest.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client-test/sr...
google-http-client-test/src/main/java/com/google/api/client/test/util/store/AbstractDataStoreFactoryTest.java:45:
public void setUp() throws Exception {
On 2013/06/13 15:51:17, peleyal wrote:
> IOExpcetion?

See my comment on the App Engine one.

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/AbstractDataStore.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/AbstractDataStore.java:49:
this.dataStoreFactory = Preconditions.checkNotNull(dataStoreFactory);
On 2013/06/13 15:51:17, peleyal wrote:
> [optional] change the order first parameter is dataStoreFactory so keep the
> first statement dataStoreFactory as well

Done.

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/AbstractDataStoreFactory.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/AbstractDataStoreFactory.java:44:
private static final Pattern ID_PATTERN = Pattern.compile("\\w{1,30}");
On 2013/06/13 15:51:17, peleyal wrote:
> please add a comment and explain here also why we need it (I know you had a
> comment on DataStoreFactoy that Id can't be more than 30 chars)

Done.

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java:56:
boolean containsValue(V value) throws IOException;
On 2013/06/13 15:51:17, peleyal wrote:
> do we need that one?
> It looks weird and I didn't see something close to it in general Map interface

Yes, it matches the Map interface.

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java:59:
* Returns the unmodifiable sorted set of all stored keys.
On 2013/06/13 15:51:17, peleyal wrote:
> I think we agreed that it won't be sorted

Done.

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java:62:
* Keys must be sorted in ascending order.
On 2013/06/13 15:51:17, peleyal wrote:
> ditto

Done.

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java:73:
* @param key key or {@code null} for {@code null} result
On 2013/06/13 15:51:17, peleyal wrote:
> you don't allowed nulls any more, right?

Right, but I only changed the set method to not allow nulls to be set, but if
you get on null we might as well match Map behavior and say null value isn't
there.  I don't have a strong opinion here, just that it is convenient to not
have to do a null check everywhere.

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java:97:
* @param key key or {@code null} to ignore
On 2013/06/13 15:51:17, peleyal wrote:
> ditto

ditto.

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/DataStoreFactory.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStoreFactory.java:28:
* Users should keep a single globally shared instance of the data store factory.
On 2013/06/13 15:51:17, peleyal wrote:
> [optional] should we explain that otherwise things may not work (like memcache
> on AppEngine, or just two memory data store, with different memory...)

Done.

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStoreFactory.java:48:
* If a data store by that ID does not already exist, it should be created now
and stored for
On 2013/06/13 15:51:17, peleyal wrote:
> created now and stored for later access. Otherwise
> --->
> created now, stored for later access and be returned. Otherwise...

Done.

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/tes...
File
google-http-client/src/test/java/com/google/api/client/util/IOUtilsTest.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client/src/tes...
google-http-client/src/test/java/com/google/api/client/util/IOUtilsTest.java:48:
File file2 = new File(file.getCanonicalPath() + "2");
On 2013/06/13 15:51:17, peleyal wrote:
> you should use file.deleteOnExit here as well.
> for both files, right?

Done.

https://codereview.appspot.com/9029044/diff/107002/pom.xml
File pom.xml (right):

https://codereview.appspot.com/9029044/diff/107002/pom.xml#newcode215
pom.xml:215: <dependency>
On 2013/06/13 15:51:17, peleyal wrote:
> why are you adding all those dependencies?
> do we need them?

Yes, for google-http-client-jdo.

[peleyal, 2013-06-13 19:32:23]

LGTM with comments

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
File
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:323:
* @return this instance
In my opinion most of the places don't have it so we should remove it here as
well
On 2013/06/13 16:35:09, yanivi wrote:
> On 2013/06/13 15:51:17, peleyal wrote:
> > [optional]
> > in most places you omit this return statement (for returning this, should we
> > omit it here as well?)
> 
> I think it's nice.  What do you think?

https://codereview.appspot.com/9029044/diff/106005/google-http-client-appengi...
File
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java
(right):

https://codereview.appspot.com/9029044/diff/106005/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:105:
/** Holder for the result of {@link #getDefaultInstance()}. */
[optional] Like we discuss I prefer static field on AppEngineDataSroreFactory
which is less efficient but it increase readability.

https://codereview.appspot.com/9029044/diff/106005/google-http-client-gson/sr...
File
google-http-client-gson/src/main/java/com/google/api/client/json/gson/GsonFactory.java
(right):

https://codereview.appspot.com/9029044/diff/106005/google-http-client-gson/sr...
google-http-client-gson/src/main/java/com/google/api/client/json/gson/GsonFactory.java:58:
/** Holder for the result of {@link #getDefaultInstance()}. */
add @Beta

https://codereview.appspot.com/9029044/diff/106005/google-http-client-jackson...
File
google-http-client-jackson2/src/main/java/com/google/api/client/json/jackson2/JacksonFactory.java
(right):

https://codereview.appspot.com/9029044/diff/106005/google-http-client-jackson...
google-http-client-jackson2/src/main/java/com/google/api/client/json/jackson2/JacksonFactory.java:66:
static class InstanceHolder {
@Beta

https://codereview.appspot.com/9029044/diff/106005/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java
(right):

https://codereview.appspot.com/9029044/diff/106005/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java:62:
* Order of the keys is not specified.
[optional] I think you can just delete that paragraph

[yanivi, 2013-06-13 19:47:57]

I made some small fixes, and added DataStoreUtil.toString() to help with
debugging.  Please take another quick look.

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
File
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java
(right):

https://codereview.appspot.com/9029044/diff/107002/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:323:
* @return this instance
On 2013/06/13 19:32:23, peleyal wrote:
> In my opinion most of the places don't have it so we should remove it here as
> well
> On 2013/06/13 16:35:09, yanivi wrote:
> > On 2013/06/13 15:51:17, peleyal wrote:
> > > [optional]
> > > in most places you omit this return statement (for returning this, should
we
> > > omit it here as well?)
> > 
> > I think it's nice.  What do you think?
> 

Done.

https://codereview.appspot.com/9029044/diff/106005/google-http-client-appengi...
File
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java
(right):

https://codereview.appspot.com/9029044/diff/106005/google-http-client-appengi...
google-http-client-appengine/src/main/java/com/google/api/client/extensions/appengine/datastore/AppEngineDataStoreFactory.java:105:
/** Holder for the result of {@link #getDefaultInstance()}. */
On 2013/06/13 19:32:23, peleyal wrote:
> [optional] Like we discuss I prefer static field on AppEngineDataSroreFactory
> which is less efficient but it increase readability.

Disagree.  This is standard technique from Effective Java Item 71.

https://codereview.appspot.com/9029044/diff/106005/google-http-client-gson/sr...
File
google-http-client-gson/src/main/java/com/google/api/client/json/gson/GsonFactory.java
(right):

https://codereview.appspot.com/9029044/diff/106005/google-http-client-gson/sr...
google-http-client-gson/src/main/java/com/google/api/client/json/gson/GsonFactory.java:58:
/** Holder for the result of {@link #getDefaultInstance()}. */
On 2013/06/13 19:32:23, peleyal wrote:
> add @Beta

Why?

https://codereview.appspot.com/9029044/diff/106005/google-http-client-jackson...
File
google-http-client-jackson2/src/main/java/com/google/api/client/json/jackson2/JacksonFactory.java
(right):

https://codereview.appspot.com/9029044/diff/106005/google-http-client-jackson...
google-http-client-jackson2/src/main/java/com/google/api/client/json/jackson2/JacksonFactory.java:66:
static class InstanceHolder {
On 2013/06/13 19:32:23, peleyal wrote:
> @Beta

Why?

https://codereview.appspot.com/9029044/diff/106005/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java
(right):

https://codereview.appspot.com/9029044/diff/106005/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStore.java:62:
* Order of the keys is not specified.
On 2013/06/13 19:32:23, peleyal wrote:
> [optional] I think you can just delete that paragraph 

Disagree.

[yanivi, 2013-06-13 20:11:15]

Added the @Beta annotation

[peleyal, 2013-06-14 13:21:27]

LGTM

https://codereview.appspot.com/9029044/diff/115012/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/DataStoreUtils.java
(right):

https://codereview.appspot.com/9029044/diff/115012/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStoreUtils.java:33:
* {@link Object#toString()}.
[optional] please add that this method calls to keySet and get(key)

https://codereview.appspot.com/9029044/diff/115012/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java
(right):

https://codereview.appspot.com/9029044/diff/115012/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/FileDataStoreFactory.java:101:
keyValueMap = Maps.newHashMap();
good catch

[yanivi, 2013-06-14 14:15:07]

submitting now...

https://codereview.appspot.com/9029044/diff/115012/google-http-client/src/mai...
File
google-http-client/src/main/java/com/google/api/client/util/store/DataStoreUtils.java
(right):

https://codereview.appspot.com/9029044/diff/115012/google-http-client/src/mai...
google-http-client/src/main/java/com/google/api/client/util/store/DataStoreUtils.java:33:
* {@link Object#toString()}.
On 2013/06/14 13:21:27, peleyal wrote:
> [optional] please add that this method calls to keySet and get(key)

Done.