Skip to content

Conversation

vpavic
Copy link
Contributor

@vpavic vpavic commented Jul 15, 2019

This PR adds support for configuring Spring Session's newly introduced SaveMode for applicable SessionRepository implementations.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 15, 2019
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

LGTM. Perhaps "Sessions save mode" is a bit raw and we can extend it to what the javadoc of the enum states?

@snicoll snicoll self-assigned this Jul 15, 2019
@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 15, 2019
@snicoll snicoll added this to the 2.2.0.M5 milestone Jul 15, 2019
@vpavic vpavic force-pushed the session-save-mode branch from 7a2dddf to bf36365 Compare July 15, 2019 17:25
@vpavic
Copy link
Contributor Author

vpavic commented Jul 15, 2019

I've updated the PR with improved description of saveMode property and suggested change to SessionAutoConfigurationRedisTests.

@snicoll
Copy link
Member

snicoll commented Jul 16, 2019

Thanks for the update @vpavic. I've been back and forth with the change as I wanted to make sure somewhat related properties are grouped together rather than simply being added at the end. This made me revert part of the polish I suggested, sorry about that.

@vpavic vpavic deleted the session-save-mode branch July 16, 2019 09:42
@vpavic
Copy link
Contributor Author

vpavic commented Jul 16, 2019

NP @snicoll - I've also contemplated that but in the end went for the simplest solution (add to end).

However, we'll probably take a closer look at this on Spring Session side before 2.2.0.RELEASE so I planned to polish up and harmonize things on the Boot side depending on the outcome of that effort. This could probably be done as a part of #17397.

@snicoll
Copy link
Member

snicoll commented Jul 16, 2019

Roger. I've just flagged that one for team attention so that we discuss the various namespace options at our next team call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants