kstyrc / embedded-redis

Redis embedded server for Java integration testing
854 stars 371 forks source link

Support JDK 6 #39

Closed rwinch closed 9 years ago

rwinch commented 9 years ago

Previously JDK 8 features were used which limitted the project to those supporting JDK 8. While some of the features being used are convenient, none of the features were necessary.

Now the project supports JDK 6+ for those that are using older JDKs.

Fixes gh-38

rwinch commented 9 years ago

I tried to leave everything as close to as it was before. One of the changes I made was that the PortProvider tests were obtaining the ports in a background thread. This seemed unnecessarily complex since the tests were < 0.000 seconds.

kstyrc commented 9 years ago

Looks pretty good to me.

Just two more notes: 1) remove Concurrently suffix from the XXXPortProviderTest method name, as we don't make it concurrently anymore 2) regarding RedisCluster.start() method, would it add any value to make servers to start in parallel? I believe that it is fast enough, isn't it? Or should we use: https://docs.oracle.com/javase/6/docs/api/java/util/concurrent/CountDownLatch.html

Oh, gotta fix this AppVeyor tests. As someone already reported, redis process does not exit properly on Windows somehow ;<

rwinch commented 9 years ago

remove Concurrently suffix from the XXXPortProviderTest method name, as we don't make it concurrently anymore

Done

regarding RedisCluster.start() method, would it add any value to make servers to start in parallel? I believe that it is fast enough, isn't it?

I gave that some thought myself, but forgot to mention it. I believe it is plenty fast running in a single thread. A few metrics:

Adding threads for something this quick is likely to just slowing things down with the context switching on the Threads.

Oh, gotta fix this AppVeyor tests. As someone already reported, redis process does not exit properly on Windows somehow ;<

Seems like AppVeyor wasn't lying which is good and bad. I may be able to dig up a 64 bit Windows version to look into this if you are unable to do so (please let me know).

kstyrc commented 9 years ago

Pretty good work. Thanks for the PR.

I'll try to release over the weekend. I also wanna publish to Maven Central: https://issues.sonatype.org/browse/OSSRH-14938

If you had time to investigate the bug on Windows, I would be grateful. Will be able to dig into that myself in a week at the earliest. My guess is that redis process on Windows does not exit clearly, so subsequent tests that use the same port will fail. If this is the case, we should do one of the things: 1) investigate how to properly shutdown redis process (maybe wait until port is released?) 2) dig into the process id and kill it somehow (OS specific add-on code)

rwinch commented 9 years ago

If this is the case, we should do one of the things:

You could also randomly select an available port every time. I do this in my builds so that the build can be ran in parallel.

kstyrc commented 9 years ago

Yeah, but even though I would like to leave the env clean after the code that uses embedded-redis will run ;)

rwinch commented 9 years ago

I wanted to add...

Thanks for the quick turnaround merging the PR and getting releases out. It is very much appreciated!

In the event you are interested, we are looking into providing official Spring Embedded Redis support using your project. See https://github.com/spring-projects/spring-session/issues/121 and the linked issues.

For an example of how I am using it in Spring Session samples (and a baseline for what it might look like):

kstyrc commented 9 years ago

I've released 0.6 via OSS Sonatype: https://oss.sonatype.org/content/groups/public/com/github/kstyrc/embedded-redis/0.6/

Maven dependency will now be:

<dependency>
  <groupId>com.github.kstyrc</groupId>
  <artifactId>embedded-redis</artifactId>
  <version>0.6</version>
</dependency>

Do you know any way to migrate previous artifacts? I guess I need to make it manually..

I'm still waiting for a sync with Maven Central:

JIRA: https://issues.sonatype.org/browse/OSSRH-14938 Maven Central: http://repo.maven.apache.org/maven2/com/github/kstyrc/embedded-redis/

kstyrc commented 9 years ago

Got synced with Maven Central. Adjusting README.md dependency section.