Skip to content

adding ReactiveFindByIndexNameSessionRepository #1205

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

ayudovin
Copy link

A reactive equivalent to FindByIndexNameSessionRepository. This enhancement

@vpavic vpavic self-assigned this Sep 22, 2018
@vpavic vpavic added this to the General Backlog milestone Sep 22, 2018
@vpavic vpavic removed their assignment Sep 26, 2018
@ayudovin
Copy link
Author

ayudovin commented Nov 2, 2018

@vpavic, Do i have chance thtat It'll be merged ?

@vpavic
Copy link
Contributor

vpavic commented Nov 2, 2018

Thanks for the PR @ayudovin - we'll give this a closer look when we start working on 2.2.

@vpavic vpavic modified the milestones: General Backlog, 2.2.x Nov 2, 2018
@vpavic vpavic self-requested a review November 2, 2018 11:22
@vpavic vpavic self-assigned this Jun 18, 2019
Copy link
Contributor

@vpavic vpavic 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 @ayudovin, I've left some comments as it isn't obvious how the implementation works. You might also want to take a look at RedisOperationsSessionRepository for inspiration (see class javadoc).

I also think that in light of #1408 and further developments around it we might want to keep the existing ReactiveRedisOperationsSessionRepository and if we end up supporting ReactiveFindByIndexNameSessionRepository we might do it in a separate implementation as support for indexing will likely require more complexity.

@@ -192,6 +198,24 @@ private String getSessionKey(String sessionId) {
return this.namespace + "sessions:" + sessionId;
}

private String getPrincipalKey(String principalName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not mistaken, the index key constructed here is only used for reading - what does actually write the index?

}
String principalKey = getPrincipalKey(indexValue);
return this.sessionRedisOperations.opsForSet()
.scan(principalKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid this isn't optimal, as you would be scanning for a match each time, rather having an index that's updated on write and have reads simple.

@vpavic vpavic modified the milestones: 2.2.x, General Backlog Jun 18, 2019
@vpavic vpavic added the status: waiting-for-feedback We need additional information before we can continue label Jun 23, 2019
@vpavic
Copy link
Contributor

vpavic commented Sep 9, 2019

Closing due to lack of feedback. Please comment back if you can provide more details and we can re-open the issue.

@vpavic vpavic closed this Sep 9, 2019
@vpavic vpavic removed in: redis status: waiting-for-feedback We need additional information before we can continue labels Sep 9, 2019
@vpavic vpavic added status: declined A suggestion or change that we don't feel we should currently apply and removed type: enhancement A general enhancement labels Sep 9, 2019
@vpavic vpavic removed this from the General Backlog milestone Sep 9, 2019
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.

3 participants