Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Punch List : Redis Session Manager #24990

Closed
10 tasks done
wezell opened this issue May 19, 2023 · 4 comments · Fixed by dotCMS/tomcat#30, dotCMS/tomcat-redis-session-manager#1, #25179 or #25313
Closed
10 tasks done

Punch List : Redis Session Manager #24990

wezell opened this issue May 19, 2023 · 4 comments · Fixed by dotCMS/tomcat#30, dotCMS/tomcat-redis-session-manager#1, #25179 or #25313
Assignees
@jcastro-dotcms
Labels
OKR : Core Features Owned by Will Priority : 3 Average QA : Approved Release : 23.01.7 Included in LTS patch release 23.01.7 Release : 23.08.16 Team : Lunik Type : New Functionality

Comments

@wezell
Copy link
Contributor

wezell commented May 19, 2023 •
edited by jcastro-dotcms

Parent Issue

#24294

User Story

  • Make connection pool size configrable
  • If the environmental variable DOT_DOTCMS_CLUSTER_ID is set, use it to prefix all gets/sets/keys/ in REDIS. See our current redis cache implementation. Doing this will allow multiple clusters to share the same session redis store.
  • rename master branch to trunk and make it the default in github.
  • change java packaging to com.dotcms
  • Test getting a single jedis connection when we initializeRedisConnection. When we start now, it looks like we are configured correctly but then errors happen later once sessions start getting created.
  • don't dump connection stack traces into the logs - make those debug. Think in about a production environment and a redis server becoming unavailable. All those session logs would swamp a server/logging very quickly. Just log a 1 line warn that contains the reason we cannot connect to redis.
  • Verify resiliency - take the redis server down while using the dotCMS and then bring it back up and make sure that we reconnect automatically.
  • create a github action that automatically jars and publishes the artifact to our repo at https://repo.dotcms.com when a merge is made to the trunk branch.
  • Many web requests are single requests - done by bots or by session-less CDNs whatever. In most cases, we should not persist an anonymous session. We need to make sure that a single call to a front end asset or page is not enough to persist that. Maybe we have a switch that enables it on the front end and if that is not set then we only persist backend sessions TOMCAT_REDIS_ENABLED_FOR_ANON_TRAFFIC
  • Unify redis config - we now have two ways to configure redis, one in the RedisClientFactory which uses a class called MasterReplicaLettuceClient and one in the RedisSessionManager. In the MasterReplicaLettuceClient, we can fall back and use the the environmental variables used by the RedisSessionManager if the ones for the MasterReplicaLettuceClient are not passed in. We can reuse as many properties from the RedisSessionManager as possible, including min/maxConnections, SSL, etc...

Acceptance Criteria

Finish the punch list.

Proposed Objective

Cloud Engineering

Proposed Priority

Priority 2 - Important

External Links... Slack Conversations, Support Tickets, Figma Designs, etc.

No response

Assumptions & Initiation Needs

No response

Quality Assurance Notes & Workarounds

No response

Sub-Tasks & Estimates

No response
@jcastro-dotcms jcastro-dotcms self-assigned this May 19, 2023
jdotcms added a commit that referenced this issue Jun 7, 2023
@jdotcms
#24990 adding composite context to avoid store the response non-seria…
918a466 
…lized on the simple context instead of the session context
This was linked to pull requests Jun 12, 2023
#24990 : [Punch list] Updating class packaging and related JAR files. dotCMS/tomcat#30
Merged
updated packages, redis client dotCMS/tomcat-redis-session-manager#1
Merged
Adding improvements to the Redis Session Manager feature #25179
Merged
fmontes pushed a commit that referenced this issue Jun 21, 2023
@jcastro-dotcms @jdotcms
Fix #24990 Adding improvements to the Redis Session Manager feature (#…
50cb7f6 
…25179)

* Adding/Updating default config values. Using the `SessionMonitor` class to add a Session parameter that indicates the Sessions that must be persisted.

* #24990 adding composite context to avoid store the response non-serialized on the simple context instead of the session context

* Updating Javadoc and more feedback from Sanchez.

* Renaming Session variable, as per feedback form Will.

* Reverting incorrect change

* Making the `WorkflowSearcher` class implement `Serializable`

---------

Co-authored-by: jdotcms <jonathan.sanchez@dotcms.com>
@jcastro-dotcms jcastro-dotcms linked a pull request Jun 22, 2023 that will close this issue
Unifying Redis config in the MasterReplicaLettuceClient class to use the Tomcat Redis Session Manager config as a fallback, if exists. #25313
Merged
fmontes pushed a commit that referenced this issue Jul 3, 2023
@jcastro-dotcms
Fix #24990 : Unifying Redis config in the `MasterReplicaLettuceClient…
2119fc5 
…` class to use the Tomcat Redis Session Manager config as a fallback, if exists. (#25313)
@fmontes
Copy link
Member

fmontes commented Jul 3, 2023

@jcastro-dotcms @bryanboza Not sure how to test this, maybe we send this directly to QA?

@jcastro-dotcms
Copy link
Contributor

@fmontes I agree. I can pass @bryanboza some instructions on how these improvements can be tested.

@jcastro-dotcms jcastro-dotcms reopened this Jul 3, 2023
manuelrojas pushed a commit that referenced this issue Jul 5, 2023
@jcastro-dotcms @manuelrojas
Fix #24990 : Unifying Redis config in the `MasterReplicaLettuceClient…
2a48a3a 
…` class to use the Tomcat Redis Session Manager config as a fallback, if exists. (#25313)
@wezell
Copy link
Contributor Author

wezell commented Jul 18, 2023

We need to test that we do not put an entry in Redis for single requests. To test clear keys in redis FLUSHAL and hit different front end URLs - pages, images, /dA urls, APIs - and including those that that switch languages (the costa rican page). Also hit the "/member" login page (without logging in).

After doing all of this (without logging in) you should be able to run KEYS in redis and there should still be 0 keys.

Try logging into the /member page and run KEYS again. You should see 1 key in redis. Run FLUSHALL and then login to the back end. Check redis KEYS and there should be 1 new key for the backend session.

@jcastro-dotcms jcastro-dotcms mentioned this issue Jul 31, 2023
Closed
4 tasks
@bryanboza
Copy link
Member

Fixed, tested on master with the last redis example and now this is working as expected. Some of the tested scenarios

  • Able to start with saving the session with redis and the regular cache
  • Able to all the parameters are configurable and you are able to see reflected those changes
  • Able to start dotCMS with redis cache provider

Image

erickgonzalez added a commit that referenced this issue Sep 8, 2023
@erickgonzalez
#24990 include in 23.01.7
700491b
@erickgonzalez erickgonzalez added Release : 23.01.7 Included in LTS patch release 23.01.7 and removed Next LTS Release labels Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jcastro-dotcms jcastro-dotcms

Labels
OKR : Core Features Owned by Will Priority : 3 Average QA : Approved Release : 23.01.7 Included in LTS patch release 23.01.7 Release : 23.08.16 Team : Lunik Type : New Functionality
Projects
dotCMS - Product Planning
Archived in project
Milestone
No milestone
5 participants
@fmontes @wezell @bryanboza @erickgonzalez @jcastro-dotcms