Improved Concurrency Support for MemoryJob* classes

[RobertFischer]

Some updates to support running multiple jobs simultaneously: First of all, longs cannot be incremented (++) safely. Second, no reason to use external synchronization when the ConcurrentMap API gives you the functionality we are looking for. Finally, since we often are adding to the end (higher keys), a ConcurrentSkipListMap should be more performant than any other kind of concurrent map.

[dsyer]

Thanks, Robert. Can you do the contributor's agreement (link in README) if you haven't already, please?

[RobertFischer]

Will do.

~~ Robert.
On May 14, 2012 2:33 AM, "Dave Syer" <
reply@reply.github.com>
wrote:

Thanks, Robert. Can you do the contributor's agreement (link in README)
if you haven't already, please?

---

Reply to this email directly or view it on GitHub:
#18 (comment)

[RobertFischer]

I forgot to add the file containing the unit test demonstrating the id generation concurrency bug: it's a hard one to replicate programmatically, so basically I just queue up some huge number of threads and try to saturate the offending method with concurrent executions. The more cores you have, the more likely you are to actually catch this issue, especially since the variable wasn't even marked as volatile.

Also, I signed the ICLA: confirmation number 26720120514094741.

[dsyer]

Thanks. Did you see the ConcurrentMapExecutionContextDaoTests? If you can modify or add to that it would help the general consistency. Let me know if you can do that.

Note that as a rule I have always advised people simply not to use these in memory repositories for real use cases - an in-memory SQL data store is much better. I have a hard time believing we will ever get rid of all the potential corner case bugs in the map-based implementation.

[RobertFischer]

Oh, yeah, I should add to that one, too.

The in-memory stuff is important for "real cases" because there are
use cases where a (meaningful) database simply isn't available.
Suggesting people shift all the way to an in-memory SQL database seems
like it's requiring a lot of work (and dependency JARs) for satisfying
an API requirement with little additional reward. The in-memory stuff
seems like a much simpler way to do that. Ultimately, the API is
pretty simple, so we should be able to get an acceptable
implementation with a bit more care for concurrent execution.

[RobertFischer]

Okay, did some fixing up in ConcurrentMapExecutionContextDao. It was mostly okay by my reading. The only catch was that with the two map implementation, there's simply no good, thread-safe way to implement the "clear" method as it is currently defined. Based on that, I changed to one map with a key type wrapping the ids. While looking at this class, I also noticed a trivial API question (BATCH-1858), so there's an ignored unit test waiting to be completed based on the result of a design decision.

The implementation at bf2c819 really wants Java's switch-on-enums to be more intelligent (more like real case matching). Java won't let me make Type final, and the compiler isn't currently smart enough to realize the Type enum can't be extended. So until Java allows me to make Type final or the compiler gets smart enough to realize that Type's not extended, there's some practically worthless code hanging out in the "default" case of the switch.

[dsyer]

OK, that all sounds good. Can you squash down to 1 commit, please, and rebase if necessary (I doubt there are any commits upstream)? If I were you I would submit pull requests from a topic branch in my fork (it's easier to manage concurrent changes that way). We don't have any strict rules here, but that makes life easier, I think.

[RobertFischer]

Yeah, all a good call. I got a bit carried away with what I thought was
going to be one quick commit.

~~ Robert.
On May 15, 2012 2:55 AM, "Dave Syer" <
reply@reply.github.com>
wrote:

OK, that all sounds good. Can you squash down to 1 commit, please, and
rebase if necessary (I doubt there are any commits upstream)? If I were
you I would submit pull requests from a topic branch in my fork (it's
easier to manage concurrent changes that way). We don't have any strict
rules here, but that makes life easier, I think.

---

Reply to this email directly or view it on GitHub:
#18 (comment)

[RobertFischer]

I cut it down to just the one major commit (addff1c), and then the two tangential commits: a change to .gitignore to ignore vi swap files (629f0cf); README updates based on my working with the library (9d89af6).

[dsyer]

I'm having some issues merging:

$ curl https://github.com/SpringSource/spring-batch/pull/18.patch | git am
...
error: patch failed: spring-batch-core/src/test/java/org/springframework/batc/core/repository/dao/MapExecutionContextDaoTests.java:3
error: spring-batch-core/src/test/java/org/springframework/batch/core/repository/dao/MapExecutionContextDaoTests.java: patch does not apply
Patch failed at 0004 Additional updates to support concurrency in the in-memory execution.

Can you rebase onto master and re-submit? It means forcing a push onto your fork, but if you don't believe anyone is using your fork to branch off that should be safe.

[RobertFischer]

There you go. One commit to rule them all: 6c98560.

[dsyer]

Merged, thanks. Next time, please open a JIRA ticket and use the id in the commit log.

[dsyer]

Sorry, Robert, I had to re-open this and revert the change because it failed the CI build. I think it uses some JDK6 classes and we only do JDK5 still. It's high time we moved on and made a Spring 3.1 JDK6 release, but for now we'll have to wait until 2.1.9 is out.

[RobertFischer]

Oh, lame. Sorry, I assumed you were on JDK6.

Do you know what classes they specifically were? It might be possible
for me to work around them.

~~ Robert.

On Wed, May 16, 2012 at 6:46 AM, Dave Syer
reply@reply.github.com
wrote:

Sorry, Robert, I had to re-open this and revert the change because it failed the CI build.  I think it uses some JDK6 classes and we only do JDK5 still.  It's high time we moved on and made a Spring 3.1 JDK6 release, but for now we'll have to wait until 2.1.9 is out.

---

Reply to this email directly or view it on GitHub:
#18 (comment)

[dsyer]

Do you know what classes they specifically were? It might be possible
for me to work around them.

See the CI report: https://build.springsource.org/browse/BATCH-TRUNK-6286. It's some collection API stuff.

D.

[RobertFischer]

Looks like ConcurrentSkipList and Long.compare(long,long).

Go ahead and close this pull request. I'll create two JIRA tickets and
two distinct pull requests, one that's JDK5 compatible and one that's
JDK6 compatible.

~~ Robert.