Try to improve functionality for #17

pom.xml

@@ -15,7 +15,8 @@
     limitations under the License.
 
 -->
-<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/maven-v4_0_0.xsd">
+<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/maven-v4_0_0.xsd">
 
     <parent>
         <groupId>org.sonatype.oss</groupId>
@@ -62,10 +63,10 @@
         <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
         <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
         <appengine.target.version>1.9.17</appengine.target.version>
-        <objectify.version>5.0.3</objectify.version>
-        <objectify-utils.version>5.0.2</objectify-utils.version>
+        <objectify.version>[5.1.0,5.2.0)</objectify.version>
 
-        <joda.time.version>2.6</joda.time.version>
+        <guava.version>18.0</guava.version>
+        <joda-time.version>2.7</joda-time.version>
         <lombok.version>1.14.8</lombok.version>
 
         <maven.compiler.plugin.version>3.1</maven.compiler.plugin.version>
@@ -145,7 +146,7 @@
         <dependency>
             <groupId>org.mockito</groupId>
             <artifactId>mockito-core</artifactId>
-            <version>1.10.8</version>
+            <version>1.10.19</version>
             <scope>test</scope>
         </dependency>
 
@@ -161,9 +162,15 @@
         <!-- ############## -->
 
         <dependency>
-            <groupId>com.sappenin.objectify</groupId>
-            <artifactId>objectify-utils</artifactId>
-            <version>${objectify-utils.version}</version>
+            <groupId>joda-time</groupId>
+            <artifactId>joda-time</artifactId>
+            <version>${joda-time.version}</version>
+        </dependency>
+
+        <dependency>
+            <groupId>com.google.guava</groupId>
+            <artifactId>guava</artifactId>
+            <version>${guava.version}</version>
         </dependency>
 
         <dependency>

src/main/java/com/theupswell/appengine/counter/service/ShardedCounterServiceImpl.java

@@ -430,13 +430,23 @@ public Counter increment(final String counterName, final long amount, boolean is
 			// "atomicIncrementShardWork" completes. This is because we don't want to increment memcache (below) until
 			// after a real increment in the datastore has taken place.
 			final Long amountIncrementedInTx = ObjectifyService.ofy().transact(atomicIncrementShardWork);
+			this.incrementMemcacheAtomic2(counterName, amountIncrementedInTx.longValue());
 
 			// /////////////////
-			// Increment this counter in memcache atomically, with retry until it succeeds (with some governor). If this
-			// fails, it's ok because memcache is merely a cache of the actual count data, and will eventually become
-			// accurate when the cache is reloaded via a call to getCount.
-			// /////////////////
-			this.incrementMemcacheAtomic2(counterName, amountIncrementedInTx.longValue());
+			// Increment this counter in memcache atomically, but only if we're not inside of an existing transaction.
+			// Otherwise, clear out memcache.
+//			if (isParentTransactionActive())
+//			{
+//				// If we're in a transaction, then don't update memcache. Instead, clear it out since we can't know if the
+//				// parent transaction will abort after our call to memcache.
+//				this.memcacheSafeDelete(counterName);
+//			}
+//			else
+//			{
+//				// If this fails, it's ok because memcache is merely a cache of the actual count data, and will eventually
+//				// become accurate when the cache is reloaded via a call to getCount.
+//				this.incrementMemcacheAtomic2(counterName, amountIncrementedInTx.longValue());
+//			}
 
 			// return #getCount because this will either return the memcache value or get the actual count from the
 			// Datastore, which will do the same thing.
@@ -449,7 +459,7 @@ public void incrementInExistingTX(String counterName, long amount)
 	{
 		// The increment functionality of this forwarding call will work properly. We discard the results of that call,
 		// however, because in certain cases that result cannot be relied upon. See Github issue #17 for more details.
-		this.increment(counterName, amount);
+		this.increment(counterName, amount, false);
 	}
 
 	/**
@@ -950,7 +960,7 @@ protected CounterData getOrCreateCounterData(final String counterName)
 		// stomp on each other. If two threads conflict with each other, one
 		// will win and create the CounterData, and the other thread will retry
 		// and return the loaded CounterData.
-		return ObjectifyService.ofy().transactNew(new Work<CounterData>()
+		return ObjectifyService.ofy().transact(new Work<CounterData>()
 		{
 			@Override
 			public CounterData run()
@@ -1243,4 +1253,17 @@ protected void assertCounterDetailsMutatable(final String counterName, final Cou
 				+ CounterStatus.AVAILABLE + " or " + CounterStatus.READ_ONLY_COUNT + " state!");
 		}
 	}
+
+	/**
+	 * A helper method to determine if Objectify is currently operating in a parent-provided transactional context or
+	 * not.
+	 * 
+	 * @return
+	 */
+	@VisibleForTesting
+	protected boolean isParentTransactionActive()
+	{
+		return ObjectifyService.ofy().getTransaction() == null ? false : ObjectifyService.ofy().getTransaction()
+			.isActive();
+	}
 }

src/test/java/com/theupswell/appengine/counter/service/AbstractShardedCounterServiceTest.java

@@ -1,20 +1,22 @@
 /**
  * Copyright (C) 2014 UpSwell LLC (developers@theupswell.com)
  *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
  *
- *         http://www.apache.org/licenses/LICENSE-2.0
+ * http://www.apache.org/licenses/LICENSE-2.0
  *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
  */
 package com.theupswell.appengine.counter.service;
 
+import static org.junit.Assert.*;
+
+import org.junit.After;
+import org.junit.Before;
+
 import com.google.appengine.api.capabilities.CapabilitiesService;
 import com.google.appengine.api.capabilities.CapabilitiesServiceFactory;
 import com.google.appengine.api.capabilities.Capability;
@@ -27,16 +29,12 @@
 import com.google.appengine.tools.development.testing.LocalMemcacheServiceTestConfig;
 import com.google.appengine.tools.development.testing.LocalServiceTestHelper;
 import com.google.appengine.tools.development.testing.LocalTaskQueueTestConfig;
-import com.googlecode.objectify.ObjectifyFilter;
 import com.googlecode.objectify.ObjectifyService;
-import com.sappenin.objectify.translate.UTCReadableInstantDateTranslatorFactory;
+import com.googlecode.objectify.impl.translate.opt.joda.JodaTimeTranslators;
+import com.googlecode.objectify.util.Closeable;
 import com.theupswell.appengine.counter.Counter;
 import com.theupswell.appengine.counter.data.CounterData;
 import com.theupswell.appengine.counter.data.CounterShardData;
-import org.junit.After;
-import org.junit.Before;
-
-import static org.junit.Assert.*;
 
 /**
  * An abstract base class for testing {@link com.theupswell.appengine.counter.service.ShardedCounterServiceImpl}
@@ -56,10 +54,10 @@ public abstract class AbstractShardedCounterServiceTest
 	protected LocalTaskQueueTestConfig.TaskCountDownLatch countdownLatch;
 
 	protected LocalServiceTestHelper helper = new LocalServiceTestHelper(
-			// Our tests assume strong consistency, but a bug in the appengine test
-			// harness forces us to set this value to at least 1.
-			new LocalDatastoreServiceTestConfig().setDefaultHighRepJobPolicyUnappliedJobPercentage(0.01f),
-			new LocalMemcacheServiceTestConfig(), new LocalTaskQueueTestConfig());
+		// Our tests assume strong consistency, but a bug in the appengine test
+		// harness forces us to set this value to at least 1.
+		new LocalDatastoreServiceTestConfig().setDefaultHighRepJobPolicyUnappliedJobPercentage(0.01f),
+		new LocalMemcacheServiceTestConfig(), new LocalTaskQueueTestConfig());
 
 	protected MemcacheService memcache;
 
@@ -99,34 +97,38 @@ public void setUp() throws Exception
 		// below
 		// http://stackoverflow.com/questions/11197058/testing-non-default-app-engine-task-queues
 		final LocalTaskQueueTestConfig localTaskQueueConfig = new LocalTaskQueueTestConfig()
-				.setDisableAutoTaskExecution(false).setQueueXmlPath("src/test/resources/queue.xml")
-				.setTaskExecutionLatch(countdownLatch).setCallbackClass(DeleteShardedCounterDeferredCallback.class);
+			.setDisableAutoTaskExecution(false).setQueueXmlPath("src/test/resources/queue.xml")
+			.setTaskExecutionLatch(countdownLatch).setCallbackClass(DeleteShardedCounterDeferredCallback.class);
 
 		// Use a different queue.xml for testing purposes
 		helper = new LocalServiceTestHelper(
-				new LocalDatastoreServiceTestConfig().setDefaultHighRepJobPolicyUnappliedJobPercentage(0.01f),
-				new LocalMemcacheServiceTestConfig(), new LocalCapabilitiesServiceTestConfig(), localTaskQueueConfig);
+			new LocalDatastoreServiceTestConfig().setDefaultHighRepJobPolicyUnappliedJobPercentage(0.01f),
+			new LocalMemcacheServiceTestConfig(), new LocalCapabilitiesServiceTestConfig(), localTaskQueueConfig);
 		helper.setUp();
 
 		memcache = MemcacheServiceFactory.getMemcacheService();
 		capabilitiesService = CapabilitiesServiceFactory.getCapabilitiesService();
 
-		ObjectifyService.ofy().clear();
-		// Must be added before registering entities...
-		ObjectifyService.factory().getTranslators().add(new UTCReadableInstantDateTranslatorFactory());
+		// New Objectify 5.1 Way. See https://groups.google.com/forum/#!topic/objectify-appengine/O4FHC_i7EGk
+		this.session = ObjectifyService.begin();
+
+		// Enable Joda Translators
+		JodaTimeTranslators.add(ObjectifyService.factory());
 
 		ObjectifyService.factory().register(CounterData.class);
 		ObjectifyService.factory().register(CounterShardData.class);
 
 		shardedCounterService = new ShardedCounterServiceImpl();
 	}
 
+	// New Objectify 5.1 Way. See https://groups.google.com/forum/#!topic/objectify-appengine/O4FHC_i7EGk
+	protected Closeable session;
+
 	@After
 	public void tearDown()
 	{
-		// This is normally done in ObjectifyFilter but that doesn't exist for
-		// tests
-		ObjectifyFilter.complete();
+		// New Objectify 5.1 Way. See https://groups.google.com/forum/#!topic/objectify-appengine/O4FHC_i7EGk
+		this.session.close();
 
 		this.helper.tearDown();
 	}
@@ -158,10 +160,10 @@ protected void assertCounter(Counter counter, String expectedCounterName, long e
 	protected ShardedCounterService initialShardedCounterService(int numInitialShards)
 	{
 		ShardedCounterServiceConfiguration config = new ShardedCounterServiceConfiguration.Builder()
-				.withNumInitialShards(numInitialShards).build();
+			.withNumInitialShards(numInitialShards).build();
 
 		ShardedCounterService service = new ShardedCounterServiceImpl(MemcacheServiceFactory.getMemcacheService(),
-				CapabilitiesServiceFactory.getCapabilitiesService(), config);
+			CapabilitiesServiceFactory.getCapabilitiesService(), config);
 		return service;
 	}
 
@@ -182,19 +184,19 @@ protected void disableMemcache()
 		// below
 		// http://stackoverflow.com/questions/11197058/testing-non-default-app-engine-task-queues
 		final LocalTaskQueueTestConfig localTaskQueueConfig = new LocalTaskQueueTestConfig()
-				.setDisableAutoTaskExecution(false).setQueueXmlPath("src/test/resources/queue.xml")
-				.setTaskExecutionLatch(countdownLatch).setCallbackClass(DeleteShardedCounterDeferredCallback.class);
+			.setDisableAutoTaskExecution(false).setQueueXmlPath("src/test/resources/queue.xml")
+			.setTaskExecutionLatch(countdownLatch).setCallbackClass(DeleteShardedCounterDeferredCallback.class);
 
 		Capability testOne = new Capability("memcache");
 		CapabilityStatus testStatus = CapabilityStatus.DISABLED;
 		// Initialize
 		LocalCapabilitiesServiceTestConfig capabilityStatusConfig = new LocalCapabilitiesServiceTestConfig()
-				.setCapabilityStatus(testOne, testStatus);
+			.setCapabilityStatus(testOne, testStatus);
 
 		// Use a different queue.xml for testing purposes
 		helper = new LocalServiceTestHelper(
-				new LocalDatastoreServiceTestConfig().setDefaultHighRepJobPolicyUnappliedJobPercentage(0.01f),
-				new LocalMemcacheServiceTestConfig(), localTaskQueueConfig, capabilityStatusConfig);
+			new LocalDatastoreServiceTestConfig().setDefaultHighRepJobPolicyUnappliedJobPercentage(0.01f),
+			new LocalMemcacheServiceTestConfig(), localTaskQueueConfig, capabilityStatusConfig);
 		helper.setUp();
 	}
 

src/test/java/com/theupswell/appengine/counter/service/ShardedCounterServiceImplTest.java

@@ -14,6 +14,7 @@
 import com.google.common.base.Optional;
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.ObjectifyService;
+import com.googlecode.objectify.VoidWork;
 import com.theupswell.appengine.counter.Counter;
 import com.theupswell.appengine.counter.data.CounterData;
 import com.theupswell.appengine.counter.data.CounterData.CounterStatus;
@@ -559,4 +560,25 @@ public void testDecrementShardWork_PreExistingCounter() throws Exception
 		assertThat(actual, is(0L));
 		assertThat(impl.getCounter(TEST_COUNTER1).getCount(), is(1L));
 	}
+
+	// ///////////////////////////////
+	// test isParentTransactionActive
+	// ///////////////////////////////
+
+	@Test
+	public void testIsParentTransactionActive()
+	{
+		assertThat(impl.isParentTransactionActive(), is(false));
+
+		ObjectifyService.ofy().transactNew(new VoidWork()
+		{
+			@Override
+			public void vrun()
+			{
+				assertThat(impl.isParentTransactionActive(), is(true));
+			}
+		});
+
+		assertThat(impl.isParentTransactionActive(), is(false));
+	}
 }