Skip to content

Polish Pivotal GemFire starter and sample. #5444

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

jxblum
Copy link
Contributor

@jxblum jxblum commented Mar 18, 2016

Closes gh-5439.

@jxblum jxblum force-pushed the polish-data-gemfire-starter-and-sample branch 3 times, most recently from fbfef77 to 350506f Compare March 23, 2016 02:02
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 29, 2016
@jxblum jxblum force-pushed the polish-data-gemfire-starter-and-sample branch from 350506f to 1c17227 Compare March 30, 2016 18:52
@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 5, 2016
@snicoll snicoll added this to the 1.4.0.M2 milestone Apr 5, 2016
@snicoll snicoll self-assigned this Apr 5, 2016
@snicoll snicoll closed this in d4a1365 Apr 5, 2016
snicoll added a commit that referenced this pull request Apr 5, 2016
…sample

* pr/5444:
  Polish contribution
  Polish Pivotal GemFire starter and sample
@snicoll
Copy link
Member

snicoll commented Apr 5, 2016

Merged with a polish commit. Thanks!

@wilkinsona
Copy link
Member

Reopening as we need to get rid of the dependency management for Spring Shell

@wilkinsona wilkinsona reopened this Apr 5, 2016
@wilkinsona wilkinsona added the type: blocker An issue that is blocking us from releasing label Apr 5, 2016
@wilkinsona wilkinsona closed this in e0f9da5 Apr 5, 2016
@wilkinsona
Copy link
Member

Reopening once again. The dependency management for Gemfire needs to be reinstated. We shouldn't rely on the transitive dependency pulling in the right version as it's then possible that another dependency would down or upgrade the version to one that isn't supported.

@wilkinsona wilkinsona reopened this Apr 5, 2016
@wilkinsona wilkinsona assigned wilkinsona and unassigned snicoll Apr 5, 2016
@wilkinsona wilkinsona closed this in b4ea90c Apr 5, 2016
@jxblum
Copy link
Contributor Author

jxblum commented Apr 5, 2016

@wilkinsona

We shouldn't rely on the transitive dependency pulling in the right version as it's then possible that another dependency would down or upgrade the version to one that isn't supported.

Understood; that makes sense. However, I fail to see any reason why an explicit dependency declaration is needed in the spring-boot-starter-data-gemfire/pom.xml at all if...

  1. GemFire is appropriately managed by Spring Boot in the spring-boot-dependencies/pom.xml, as it was here and here.
  2. And, regardless if Spring Data GemFire (using the starter), or another Spring Boot Starter (e.g. integration) or even say, an application dependency, pulls in GemFire transitively, as long as the starter or the application POM, (indirectly) inherits from the spring-boot-dependencies/pom.xml appropriately, such as through the spring-boot-parent (for the spring-boot-starter-data-gemfire, the hierarchy is spring-boot-starters -> spring-boot-parent -> spring-boot-dependencies), then the explicit GemFire dependency declaration should not be needed, yet the right version of GemFire for Spring Boot, and specifically SDG, or any other module (e.g. integration) requiring GemFire, will be used.

I would also caution that Spring Data GemFire has a very specific requirement on the version of GemFire used. It was both a mistake and naive that SDG supported multiple simultaneous versions of GemFire at once in the past, something I have corrected since then.

So, it is absolutely paramount that the gemfire.version managed by Spring Boot is kept in-sync with the version that Spring Data GemFire expects, which is determined by the Spring Data Release Train version pulled in by Spring Boot (e.g. for Hopper, SDG 1.8 uses GemFire 8.2). Of course, this is manageable, but will require a bit of extra care and attention.

@wilkinsona
Copy link
Member

However, I fail to see any reason why an explicit dependency declaration is needed in the spring-boot-starter-data-gemfire/pom.xml at all

You've linked to the starter from M1. As things stand, there's no such dependency in the starter.

I would also caution that Spring Data GemFire has a very specific requirement on the version of GemFire used.

Thanks. We understand that. It's no different to several other dependencies for which we also provide dependency management.

@jxblum
Copy link
Contributor Author

jxblum commented Apr 5, 2016

As things stand, there's no such dependency in the starter.

I saw that, actually (I only referenced M1 assuming we wanted tor revert back to how it was).

So is there anything that needs to be done with this PR, or that I should redo? I suppose not since it is now closed.

@wilkinsona
Copy link
Member

Nothing more needs to be done. I took care of the last two niggles in the two commits that are referenced above (e0f9da5 and b4ea90c)

@jxblum
Copy link
Contributor Author

jxblum commented Apr 5, 2016

Thanks @wilkinsona!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: blocker An issue that is blocking us from releasing type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants