Skip to content

Add starter and sample for Apache Geode. #5445

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

Closed

Conversation

jxblum
Copy link
Contributor

@jxblum jxblum commented Mar 18, 2016

Closes gh-5438.

@jxblum
Copy link
Contributor Author

jxblum commented Mar 18, 2016

DO NOT MERGE (yet)! This PR is a preview for feedback only.

So, it turns out the spring-boot-starters/spring-boot-starter-data-geode (and sample) I created does not work as expected. Meaning, if you run the SampleDataGeodeApplicationTest, it picks up Pivotal GemFire 8.2.0 dependency and not Apache Geode 1.0.0-incubating.M1 as it should.

As further verification, my independent, example/test application declaring the spring-boot-starter-data-geode dependency also resolves to Pivotal GemFire and not Apache Geode. See attached screenshot as proof.

spring-boot-starter-data-geode

I believe this is related to the inclusion of the Spring Data Maven BOM file directly based on the Spring Data version currently used by Spring Boot.

The problem is 1) the published Spring Data Maven BOM file has a hard version on the spring-data-gemfire dependency in the dependency declarations, rather than a property placeholder (e.g. ${spring-data-gemfire.version}) and a corresponding property definition (that could be overridden in an inheriting POM)...

<properties>
  <spring-data-gemfire.version>1.8.0.RC1</spring-data-gemfire.version>
<properties>

Thus as I have done in the spring-boot/spring-boot-dependencies.pom file, properties section, and therefore, making the Spring Data GemFire version included by Spring Boot overridable, if even possible.

I am thinking at this point, if there is no special Maven magic to work around this issue, then we might have to go with an actual, separate Spring Data GemFire dependency, as in spring-data-geode, making "Spring Data GemFire with Apache Geode support" a top-level module (i.e. Spring Data Geode) in the Spring Data portfolio.

Thoughts?

@@ -135,6 +135,7 @@
<spring-amqp.version>1.6.0.M1</spring-amqp.version>
<spring-cloud-connectors.version>1.2.1.RELEASE</spring-cloud-connectors.version>
<spring-batch.version>3.0.6.RELEASE</spring-batch.version>
<spring-data-geode.version>1.7.0.APACHE-GEODE-EA-M1</spring-data-geode.version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated but that's a weird version naming. Any reason why it has to be that long?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I explained here and here, this is not a usual version number.

Essentially, 2 separate releases of Spring Data GemFire are occurring off 1 codebase (in GitHub) right now. Though, the Spring Data GemFire Support for Apache Geode is technically occurring off a branch (apache-geode).

Based on the issues I had with the starter, I think now, even more than ever/before, it is time to consider Spring Data GemFire with Apache Geode Support as a separate project (SD module) having a separately releasable artifact (i.e. spring-data-geode) and intuitive version number (i.e. 1.0.0.M1).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 21, 2016
@jxblum jxblum force-pushed the starter-data-geode-and-sample branch from 5fb6b64 to 3026557 Compare March 22, 2016 01:58
* @since 1.4.0
*/
@SpringBootApplication
@ConfigurationProperties
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's wrong. This should be@EnableConfigurationProperties(SampleDataGeodeProperties.class) and you should define that class with the list of keys that the sample needs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

@snicoll snicoll added status: on-hold We can't start working on this issue yet and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 22, 2016
@jxblum jxblum force-pushed the starter-data-geode-and-sample branch from 3026557 to 94af046 Compare March 23, 2016 02:30
@jxblum
Copy link
Contributor Author

jxblum commented Mar 23, 2016

Note, I cleaned up both #5444 and this PR by...

  1. Removing all uses of @Value annotations.
  2. Along with removing all <exclusions> in the (sample) POMs.
  3. I also moved the spring-shell.version property from the GemFire/Geode sample POMs to spring-boot-dependencies/pom.xml. The GemFire/Geode starter POMs do not include Spring Shell as a dependency because it is not required at runtime to use GemFire/Geode (it just avoids the nasty, benign stack trace in the samples).

#5444 (polishing the GemFire starter/sample) is complete.

This PR still needs to sort out the spring-data-gemfire artifact. Again, I think the only appropriate solution is to create a new SD module, Spring Data Geode.

@olivergierke what say you?

@jxblum
Copy link
Contributor Author

jxblum commented Mar 23, 2016

Note, out of curiosity, I also observed that including the following in the spring-boot-starter-data-geode POM file...

<dependencyManagement>
    <dependencies>
        <dependency>
            <groupId>org.springframework.data</groupId>
            <artifactId>spring-data-gemfire</artifactId>
            <version>${spring-data-geode.version}</version>
        </dependency>
    </dependencies>
</dependencyManagement>

Rather than...

<dependency>
    <groupId>org.springframework.data</groupId>
    <artifactId>spring-data-gemfire</artifactId>
    <version>${spring-data-geode.version}</version>

With the explicit <version> is no different. I.e. it has the same effect.

Pivotal GemFire is still used over Apache Geode in the spring-boot-sample-data-geode application, that just declares the spring-boot-starter-data-geode dependency. This means the SD release train dependency declaration is ever lasting/overriding and takes absolute precedence.

The only workaround is to put the above <dependencyManagement> section in the Geode sample, which means the user would need to declare the same <dependencyManagement> section in their application POM, which defeats the purpose of the spring-data-starter-data-geode Starter since the user could just use the spring-boot-starter-data-gemfire Starter and include the same <dependencyManagement> section in her application POM to get Apache Geode support.

Therefore, everything rides on creating a separate artifact (i.e. Spring Data Geode), or simply not having a Spring Boot Starter for Apache Geode. However, I do think it would be nice and convenient for our users.

DISCLAIMER: I know that we don't want to include the <dependencyManagement> section in either the Geode starter or sample POMs, as it is the same thing as overriding the version, which we DON'T want to do, I know; I was simply experimenting out of curiosity, ;-)

@jxblum jxblum force-pushed the starter-data-geode-and-sample branch 4 times, most recently from c11ef56 to 0081a38 Compare April 30, 2016 22:55
@jxblum
Copy link
Contributor Author

jxblum commented Apr 30, 2016

@SpringBootTeam - Alright, after all checks pass, this PR is ready to go (review and merge after your approval).

If there are any questions or concerns, please let me know.

Thanks!
-John

@jxblum jxblum self-assigned this May 1, 2016
@wilkinsona
Copy link
Member

thanks, @jxblum. What's the plan for a final release of both Geode and the accompanying Spring Data module? We can't use a milestone dependency in a final release of Spring Boot so this may have to wait till after 1.4.0.

@jxblum
Copy link
Contributor Author

jxblum commented May 1, 2016

Hi @wilkinsona -

There has only been recent mention of an official Apache Geode 1.0.0-incubating GA release. However, there is no current plan or schedule in place yet.

Still, both Apache Geode 1.0.0-incubating.M2 and Spring Data Geode 1.0.0.APACHE-GEODE-INCUBATING-M2 are available in Maven Central. Additionally, even after Apache Geode reaches GA status, it will still be "incubating" at ASF and won't be a TLP yet.

Would it be possible to include Geode support in Spring Boot 1.4.0.RELEASE as a sort of Early Access (EA) preview, citing that Apache Geode has not reached final GA yet?

Technically, the only reason why I have the newly minted Spring Data Geode module tagged as a milestone (i.e. APACHE-GEODE-INCUBATING-M2) is because Geode is still releasing milestones. However, SD Geode is really based on the latest Spring Data GemFire codebase (i.e. 1.8.1.RELEASE) and is, therefore, quite stable.

Cheers,
John

@wilkinsona
Copy link
Member

Thanks. We'll wait for Geode to reach GA.

@jxblum jxblum force-pushed the starter-data-geode-and-sample branch from 0081a38 to 403687f Compare September 19, 2016 20:26
@jxblum jxblum force-pushed the starter-data-geode-and-sample branch from 403687f to 10b59c1 Compare November 3, 2016 07:27
@jxblum
Copy link
Contributor Author

jxblum commented Nov 3, 2016

@wilkinsona, @snicoll & Spring Boot team-

This PR to add support for Apache Geode has been updated with the latest release of Spring Data Geode 1.0.0.INCUBATING-RELEASE (here), which is now based on Apache Geode 1.0.0-incubating GA release (here; more details here).

If you have questions, let me know.

Thanks!

@jxblum jxblum force-pushed the starter-data-geode-and-sample branch from 10b59c1 to 5366e0f Compare November 4, 2016 05:36
@philwebb
Copy link
Member

philwebb commented Nov 4, 2016

As with #6952 we should wait for a non incubating release I think.

@wilkinsona
Copy link
Member

As discussed, this code is going to be delivered in a separate module outside of Spring Boot.

@wilkinsona wilkinsona closed this Nov 22, 2016
@jxblum
Copy link
Contributor Author

jxblum commented Nov 22, 2016

@philwebb, @wilkinsona - I heard the message loud and clear on the SD Geode Caching and Repository support, but why does the Spring Boot "starter" for Spring Data Geode need to live outside of Spring Boot? If that is the case, then why do we include the spring-boot-starter-data-gemfire in Spring Boot? What's the difference?

@wilkinsona
Copy link
Member

The rule that we apply is that we only provide starters and samples for something if we also have auto-configuration for it. We've rejected other requests for starters and samples based on this rule and we need to apply it consistently. The GemFire starter is an exception to that rule for reasons that escape me. If we were doing things again, we wouldn't have that starter either.

@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed status: on-hold We can't start working on this issue yet labels Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants