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.

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

Upgrade to jakarta namespace #140

Merged
merged 11 commits into from
Aug 21, 2024
Merged

Conversation

orcunc
Copy link
Contributor

@orcunc orcunc commented Jun 11, 2024

This project is kind of abandoned. It has not been release for long time.
The purpose of this PR is to upgrade spring and jakarta dependencies so that the project can be useful again.

  • Upgrade dependencies to latest version
  • Delete methods removed from spec
  • Change method calls if necessary according to new spec.

@orcunc orcunc requested review from JackPGreen and srknzl June 11, 2024 08:46
@orcunc orcunc self-assigned this Jun 11, 2024
@orcunc orcunc requested a review from frant-hartm June 11, 2024 09:08
Copy link
Member

@srknzl srknzl left a comment

Choose a reason for hiding this comment

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

@orcunc Hold on a second, there's a release in progress for this so we can't merge anything. The customer wanted a release with the old javax namespace, also do you have a jira issue for this change? I'm already working on this project, and as I know there's no jakarta release planned

Copy link
Member

@srknzl srknzl left a comment

Choose a reason for hiding this comment

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

removing request change due to release being done

README.md Show resolved Hide resolved
pom.xml Outdated
Comment on lines 40 to 41
<jsp.api.version>4.0.0-M2</jsp.api.version>
<servlet.api.version>6.1.0-M2</servlet.api.version>
Copy link
Member

Choose a reason for hiding this comment

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

what's M2 in the version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what's M2 in the version?

M2 was the latest version at the time PR is created. Now new versions have been released.

rev : efd7ad2

Copy link
Member

@srknzl srknzl left a comment

Choose a reason for hiding this comment

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

should we update

<module name="AvoidStarImport">
from javax to jakarta as well?

@JamesHazelcast JamesHazelcast removed their request for review July 4, 2024 11:56
@orcunc orcunc requested a review from srknzl August 20, 2024 09:14
@orcunc
Copy link
Contributor Author

orcunc commented Aug 20, 2024

AvoidStarImport

The rule is for javax.swing


- Target application or web server should support Servlet 4.0 or higher spec. Note that this library supports, "javax" namespace, not the new "jakarta" one.
- Target application or web server should support Servlet 6.1 or higher spec. Note that this library supports, "jakarta" namespace.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit - if it exclusively supports jakarta, it should explain it doesn't support javax (and perhaps reference the last version that did).

Copy link
Member

@srknzl srknzl Aug 20, 2024

Choose a reason for hiding this comment

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

Hmm, should not we support tomcat 10 (a currently supported version
) as well? Tomcat 10 supports servlet 6.0 and java 11 and higher.

source: https://tomcat.apache.org/whichversion.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are saying that we support Servlet 6.1. Servlet 6.1 can be implemented by tomcat, jetty or any other servlet container.

@@ -653,7 +657,7 @@
</profile>
<profile>
<!-- SUP-420 case. Tomcat 9.0.z and 5.3.z hazelcast. Using jdk 1.8 because tomcat supports 1.8.
Therefore we have to exclude jetty tests from compiling/running and jetty classes from classpath
Therefore, we have to exclude jetty tests from compiling/running and jetty classes from classpath
because they require jdk 11. -->
<id>tomcat-jdk8</id>
Copy link
Member

Choose a reason for hiding this comment

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

this may not be working anymore, I think we should change a lot of dependency versions in this profile. we can even remove this profile, because tomcat 9 is not supported by this version of hazelcast-wm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can remove it later. This module needs more work. Let's first upgrade to jakarta. Then we can remove these kind of things later

@srknzl
Copy link
Member

srknzl commented Aug 20, 2024

  • now that we'll release 5.1 (I assume), we should change the note in the readme:
diff --git a/README.md b/README.md
index d3e7406..de6fa7c 100644
--- a/README.md
+++ b/README.md
@@ -20,7 +20,7 @@
 <br></br>
 *You can also check [Hazelcast Guides: Session Replication with Spring Boot and Hazelcast](https://guides.hazelcast.org/springboot-webfilter-session-replication/).*
 
-***Note***: *Hazelcast 5.0+ is compatible with Hazelcast-WM 5.0. For older Hazelcast versions, use the latest Hazelcast-WM 3.x.x or 4.x.x releases.*
+***Note***: *Hazelcast 5.0+ is compatible with Hazelcast-WM 5.0+. For older Hazelcast versions, use the latest Hazelcast-WM 3.x.x or 4.x.x releases.*
 <br></br>
 
 Assume that you have more than one web server (A, B, C) with a load balancer in front of it. If server A goes down, your users on that server will be directed to one of the live servers (B or C), but their sessions will be lost.
  • I would remove javax.swing from AvoidStarImport rule to clean the repo from "javax" completely:

AvoidStarImport

The rule is for javax.swing

@orcunc
Copy link
Contributor Author

orcunc commented Aug 20, 2024

verify

@orcunc orcunc merged commit c1b5432 into hazelcast:master Aug 21, 2024
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants