Update Infinispan dependencies to 9.0.1.Final #9417

galderz · 2017-06-06T12:21:48Z

Update Infinispan dependencies and fix sample cache example so that it works with Infinispan 9.

pivotal-issuemaster · 2017-06-06T12:26:50Z

@galderz Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

snicoll

Can you please explain if the sample is broken with Infinispan 9? If so, a link that explains the change would be appreciated.

If the sample is broken now, I think we should fix it regardless of the update.

Are you aware that infinispan provides a starter of their own?

snicoll · 2017-06-06T12:27:21Z

spring-boot-actuator/pom.xml

 		</dependency>
+		<dependency>
+			<groupId>org.infinispan</groupId>
+			<artifactId>infinispan-core</artifactId>


Why is that extra dependency necessary. Has something changed in 9.x that the core isn't provided with the embedded module?

Indeed. Starting with Infinispan 9 infinispan-jcache module has infinispan-core as provided dependency. So if an end-user application depends on infinispan-jcache, it must also depend on infinispan-core. More info can be found here.

Sorry but that link doesn't give me any extra info. Can you please clarify? What justify the fact we need to add two dependencies now? Having a infinispan-jcache module IMO should be enough.

Good point, I'm trying to find out more on this.

snicoll · 2017-06-06T12:28:08Z

spring-boot-samples/spring-boot-sample-cache/README.adoc

-`infinispan.xml` configuration instead.
+file so if you don't specify anything it will bootstrap on a hardcoded default. However,
+for the sample application to work, `spring.cache.infinispan.config` needs to be set to
+`infinispan.xml` so that necessary caches can be found.


And infinispan.xml is no longer used as a default?

infinispan.xml has never been a default for us. We've never had a default configuration file name.

Oh I get it now. That's because previously you'd have created a "default cache". Yeah I remember that now. Perhaps I should fix that part of the sample differently then. I'll give it some thoughts.

Do we need to provide both core and the embedded dependencies? I thought that was JCache only...

I guess by embedded you mean infinispan-embedded artifact id. If so, infinispan-embedded should never be a dependency in a Maven project. embedded is really an uber jar designed for those who still use Ant. Having it as a Maven dependency can cause troubles.

I'm not aware of any infinispan-embedded dependencies inside spring-boot repo.

I mean the dependency in the doc and the code, infinispan-spring4-embedded

Ah, let me check that out...

galderz · 2017-06-06T12:51:54Z

Hi @snicoll

Can you please explain if the sample is broken with Infinispan 9? If so, a link that explains the change would be appreciated.

Yeah, before Infinispan 9, you could just retrieve non-default any cache even if no configuration file or no default cache configuration had been defined. However, starting with Infinispan 9 this is no longer possible. So, if you want to access cache countries, you need to either specifically define a default cache for the cache manager, or define the cache per se.

So, the simplest fix is to simply define the countries cache and force infinispan.xml to be used.

The Infinispan 9 changes in this are are explained here.

If the sample is broken now, I think we should fix it regardless of the update.

The sample is only broken when using Infinispan 9+.

Are you aware that infinispan provides a starter of their own?

I do indeed, I'm a colleague of @slaskawi. We still want to make sure any Infinispan examples within Spring Boot repo are up to date.

pivotal-issuemaster · 2017-06-06T12:54:49Z

@galderz Thank you for signing the Contributor License Agreement!

snicoll · 2017-06-06T16:01:52Z

spring-boot-samples/spring-boot-sample-cache/src/main/resources/infinispan.xml

 <infinispan xmlns="urn:infinispan:config:7.2">

-	<cache-container default-cache="default">
+	<cache-container default-cache="countries">


Sorry I don't understand. Isn't the local-cache below creating a cache named countries? countries is just one cache used in the application, it's not the default cache.

Yeah, but you also need to provide a default cache definition. That file, as it was, would not work because there was no cache named default in the file. There were two ways to solve it:

a. Add default cache definition, e.g.

<local-cache name="default"/>

b. Set countries as default cache.

I opted for b. since it was less config. It means countries cache could be retrieved via getCache() or getCache("countries") call.

I don't think having countries as the default cache is a good idea for the sample. None of the other samples do that. I've already polished that part in fc38c1b based on your feedback so we should be good already.

Oh ok, thx for fixing that! That looks good. With my changes, I'd only need to update the schema version and that'd be fine.

Clarify the cache sample and in particular that Infinispan does not bootstrap with a default configuration file. Hence the custom `infinispan.xml` configuration is enabled by default if Infinispan is available on the classpath. See gh-9417

snicoll · 2017-06-09T06:02:29Z

@galderz any news about that new required dependency? IMO that's pretty bad for users upgrading if that's mandatory all the sudden.

galderz · 2017-06-14T12:54:44Z

@galderz any news about that new required dependency? IMO that's pretty bad for users upgrading if that's mandatory all the sudden.

Sorry for delay (travel). We're in the middle of a dev discussion that I need to catch up on. I'll update as soon as I have something. The current latest version would require the dependencies to be like this though. I'm trying to get that changed in the next point release.

snicoll · 2017-06-14T12:56:06Z

@galderz thanks. We'll wait for that then.

galderz · 2017-07-04T15:55:56Z

@snicoll I'm trying to build master but does not seem to work? Trying to build with JDK 8

$ ./mvnw clean install -DskipTests
...
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.1:testCompile (default-testCompile) on project spring-boot-autoconfigure: Compilation failure: Compilation failure:
[ERROR] /Users/g/0/spring/spring-boot/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/mongo/MongoDataAutoConfigurationTests.java:[170,53] incompatible types: org.springframework.data.mongodb.core.mapping.BasicMongoPersistentEntity<capture#1 of ?> cannot be converted to java.util.Optional<org.springframework.data.mongodb.core.mapping.BasicMongoPersistentEntity<?>>
[ERROR] /Users/g/0/spring/spring-boot/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/mongo/MongoDataAutoConfigurationTests.java:[173,55] incompatible types: org.springframework.data.mongodb.core.mapping.MongoPersistentProperty cannot be converted to java.util.Optional<org.springframework.data.mongodb.core.mapping.MongoPersistentProperty>

Ring a bell?

snicoll · 2017-07-04T16:28:26Z

No but I just found out that Spring Data changed some of their APIs and we're consuming their snapshot. @wilkinsona is looking into it right now.

wilkinsona · 2017-07-04T16:49:38Z

@galderz I believe I've fixed it in b11053a.

* infinispan-core dependency needed since this is now provided for infinispan-cache. * To access "countries" cache, it's necessary to define it in advance now, so a configuration file needs to be passed in. * Use "countries" as default cache that needs to be predefined.

galderz · 2017-07-06T06:43:33Z

Thanks @wilkinsona and @snicoll, master is indeed compiling now.

@snicoll Let's close this PR. Infinispan 9.0.3.Final has been released which does not require the extra infinispan-core dependency. I'll open a PR in a few minutes.

galderz · 2017-07-06T06:44:05Z

New PR is #9688

snicoll · 2017-07-06T09:34:04Z

@galderz for the record there is no need to open a new PR. If you push force on the branch you used to create the PR it will be updated.

galderz · 2017-07-09T03:20:30Z

@galderz for the record there is no need to open a new PR. If you push force on the branch you used to create the PR it will be updated.

I'm aware. I regularly do that, but in this case I wanted to keep around the other branch just in case, and the update was sufficiently different, and much more simplified, to deserve a separate PR :)

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 6, 2017

snicoll reviewed Jun 6, 2017

View reviewed changes

snicoll self-assigned this Jun 6, 2017

snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 6, 2017

snicoll added this to the 2.0.0.M2 milestone Jun 6, 2017

snicoll requested changes Jun 6, 2017

View reviewed changes

snicoll removed this from the 2.0.0.M2 milestone Jun 7, 2017

snicoll added the status: on-hold We can't start working on this issue yet label Jun 14, 2017

snicoll removed their assignment Jun 27, 2017

galderz force-pushed the t_infinispan_9x branch from a6d0950 to c3b1554 Compare July 4, 2017 15:17

galderz added 2 commits July 5, 2017 09:00

Update to Infinispan 9.0.1.Final

3cd8ecd

galderz force-pushed the t_infinispan_9x branch from c3b1554 to fd147a5 Compare July 5, 2017 07:00

snicoll mentioned this pull request Jul 5, 2017

Upgrade Infinispan to 9.1.0.CR1 #9681

Closed

galderz closed this Jul 6, 2017

snicoll added the status: duplicate A duplicate of another issue label Jul 6, 2017

snicoll removed status: on-hold We can't start working on this issue yet type: enhancement A general enhancement labels Jul 6, 2017

Update Infinispan dependencies to 9.0.1.Final #9417

Update Infinispan dependencies to 9.0.1.Final #9417

Uh oh!

Conversation

galderz commented Jun 6, 2017

Uh oh!

pivotal-issuemaster commented Jun 6, 2017

Uh oh!

snicoll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galderz commented Jun 6, 2017

Uh oh!

pivotal-issuemaster commented Jun 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snicoll commented Jun 9, 2017

Uh oh!

galderz commented Jun 14, 2017

Uh oh!

snicoll commented Jun 14, 2017

Uh oh!

galderz commented Jul 4, 2017

Uh oh!

snicoll commented Jul 4, 2017

Uh oh!

wilkinsona commented Jul 4, 2017

Uh oh!

galderz commented Jul 6, 2017

Uh oh!

galderz commented Jul 6, 2017

Uh oh!

snicoll commented Jul 6, 2017

Uh oh!

galderz commented Jul 9, 2017

Uh oh!

Uh oh!