leonfin commented on PR #7953:
URL: https://github.com/apache/geode/pull/7953#issuecomment-3615128615
1. Critical: Inconsistent Attribute Name in XML Configuration
Location:
geode-docs/tools_modules/http_session_mgmt/quick_start.html.md.erb:66
- <Listener
className="org.apache.geode.modules.session.catalina.PeerToPeerCacheLifecycleListener"
- locator="localhost[10334]" />
+ <Listener
className="org.apache.geode.modules.session.catalina.PeerToPeerCacheLifecycleListener"
+ locators="localhost[10334]" />
Issue: The attribute name changed from locator to locators. This needs
verification:
- Is this a breaking API change?
- Does the actual implementation class support both attributes for
backward compatibility?
- Should there be migration notes about this change?
2. Potentially Confusing Example Path
Location:
geode-docs/tools_modules/http_session_mgmt/tomcat_installing_the_module.html.md.erb:56
- CATALINA_HOME=/usr/bin/apache-tomcat-9.0.62
+ CATALINA_HOME=/opt/apache-tomcat-10.1.49
Issue: The path shows .49 but earlier in the file it shows .30. While
these are just examples, consistency would be better for readability.
3. Java Version Format Inconsistency in Output Examples
Location:
geode-docs/tools_modules/http_session_mgmt/tomcat_setting_up_the_module.html.md.erb:123
-15-Jul-2021 10:25:11.483 INFO...
+15-Jul-2025 10:25:11.483 INFO...
Issue: The year was updated to 2025, but this is output from a running
system. The commit message says Java 17 is required, and the output should
reflect that version number format. However, I see:
Java Version: <%=vars.min_java_version%>.0.<%=vars.min_java_update%>
This will render as 17.0.16 which is correct for Java 17 format (not
1.17.0_16).
4. Missing Period in Changelog/Deprecation Message
Location:
geode-docs/tools_modules/http_session_mgmt/quick_start.html.md.erb:55
**Support for Pivotal tc Server has been removed. Users should migrate to
Tomcat 10.1 or later.**
This line appears standalone and could be integrated better with the
"Note:" style used elsewhere.
Positive Aspects
1. ✅ Comprehensive Version Updates: All Java version strings correctly
changed from 1.8.0_121 format to 17.0.16 format
2. ✅ Consistent Class Name Updates: All manager class names updated from
Tomcat9DeltaSessionManager to Tomcat10DeltaSessionManager
3. ✅ Clear Deprecation Notices: Multiple warnings added about tc Server
and old Tomcat version discontinuation
4. ✅ Good Troubleshooting Section: New troubleshooting section added to
help users debug common issues
5. ✅ Jakarta Namespace Migration: Correctly updated from
javax.transaction-api to jakarta.transaction-api
6. ✅ Navigation Cleanup: Removed tc Server entries from navigation (subnav
file)
7. ✅ Updated Tomcat Homepage Image: Binary image file updated (likely
showing Jakarta EE compatible Tomcat version)
---
Recommendations
1. High Priority: Verify the locator → locators attribute name change is
correct and intentional
2. Medium Priority: Clarify whether backward compatibility exists for the
attribute name change
3. Low Priority: Fix minor formatting inconsistencies (periods, example
version numbers)
4. Documentation: Add a migration guide section explaining breaking
changes for users upgrading from 1.x to 2.0
---
Correctness Assessment
Overall: Good with minor issues ✅
The commit correctly updates the documentation for the major version
migration. The changes are internally consistent and properly reflect:
- Java 8 → Java 17 migration
- Java EE → Jakarta EE 10 migration
- Tomcat 10.1+ requirement
- tc Server deprecation
The main concern is the locator → locators attribute change which could
break existing configurations and needs verification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
