Skip to content

Suggested changes for compatibility with hazelcast:4.0.1 and spring-boot:2.3.0 on the branch 'v2.3.x' #1645

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
wants to merge 3 commits into from

Conversation

adeniyiodunlami
Copy link

@adeniyiodunlami adeniyiodunlami commented Jun 4, 2020

Suggested changes for compatibility with hazelcast:4.0.1 and spring-boot:2.3.0.
#1644
gh-1584

Adebowale Odunlami and others added 3 commits June 3, 2020 14:07
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 4, 2020
@adeniyiodunlami adeniyiodunlami changed the title 2.3.x Suggested changes for compatibility with hazelcast:4.0.1 and sprint-boot:2.3.0 on the branch 'v2.3.x' Jun 4, 2020
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @adeniyiodunlami!

As discussed in gh-1584, we cannot upgrade to Hazelcast 4.

We would accept a PR that offers support for Hazelcast 4, while still providing full support for Hazelcast 3.
This would be similar to the changes Spring Boot made for spring-projects/spring-boot#21169.

@eleftherias
Copy link
Contributor

After some discussion, we have decided to take a different approach than the Spring Boot team.
Instead of using reflection, we will create a new class, similar to HazelcastIndexedSessionRepository that is compatible with Hazelcast 4.

We will likely leverage some of the work that was done in this PR and will continue to keep you updated as we make progress.

@vpavic
Copy link
Contributor

vpavic commented Jun 23, 2020

we will create a new class, similar to HazelcastIndexedSessionRepository that is compatible with Hazelcast 4.

I'd suggest against this approach, as it will inevitably result in:

  • duplicated code
  • SessionRepository component that's bound to be deprecated once Hazelcast 4 is baseline
  • more difficult configuration

IMO a better way to tackle this would be to extract a strategy that encapsulates the use of Hazelcast API's that introduced binary incompatible changes between Hazelcast 3.x and 4.x - HazelcastIndexedSessionRepository would internally implement this strategy to keep the existing (Hazelcast 3.x compabile) behaviour while additional implementation would be provided to support Hazelcast 4.x.

Once Hazelcast 4.x is the baseline (likely Spring Session 3.0) those implementations simply switch places.

@adeniyiodunlami adeniyiodunlami changed the title Suggested changes for compatibility with hazelcast:4.0.1 and sprint-boot:2.3.0 on the branch 'v2.3.x' Suggested changes for compatibility with hazelcast:4.0.1 and spring-boot:2.3.0 on the branch 'v2.3.x' Oct 12, 2020
@pivotal-issuemaster
Copy link

@adeniyiodunlami Please sign the Contributor License Agreement!

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

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@adeniyiodunlami Thank you for signing the Contributor License Agreement!

@eleftherias
Copy link
Contributor

As discussed in the associated issue gh-1644, changes to support Hazelcast 4 were introduced via 2b6489c.
For that reason I'm closing this issue as a duplicate.

@eleftherias eleftherias added status: duplicate A duplicate of another issue and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 19, 2021
@eleftherias eleftherias self-assigned this Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants