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

Adding Jakarta Messaging 3.0 support #674

Merged
merged 6 commits into from
Jul 2, 2024
Merged

Adding Jakarta Messaging 3.0 support #674

merged 6 commits into from
Jul 2, 2024

Conversation

samueleresca
Copy link
Member

@samueleresca samueleresca commented May 27, 2024 •
edited
Loading

See: #483

Few notes regarding the PR after spending some time on this:

  • Jakarta JMS 3.1 requires Java 11+, see Jakarta Messaging 3.1.0 Release Review. This PR is upgrading to Jakarta JMS 3.0 which is the latest release supporting Java 8.
  • ActiveMQ Classic is used in the jms connector to perform the integration tests. ActiveMQ classic cannot be upgraded to a version later than 5.16.7 because the later versions don't support Java 8.
  • ActiveMQ Classic 5.16.7 only supports Javax JMS 1.1.
  • I replaced ActiveMQ Classic with ActiveMQ Artemis, which supports both the new versions of JMS and also Java 8. I wasn't able to port the following 2 tests using ActiveMQ Artemis:
    • JmsProducer should retry sending on network failures
    • The JMS Transactional Connectors ensure no message loss or starvation when exceptions occur in a stream missing commits

@samueleresca samueleresca marked this pull request as draft May 27, 2024 17:19
@samueleresca samueleresca changed the title [DRAFT] Verify Jakarta support Verify Jakarta support May 27, 2024
@samueleresca samueleresca force-pushed the adding-jakarta-support branch 3 times, most recently from 838b8b8 to e620c47 Compare June 2, 2024 19:40
samueleresca
samueleresca commented Jun 2, 2024
View reviewed changes
...ams/src/test/scala/org/apache/pekko/stream/connectors/jakartams/EmbeddedActiveMQServer.scala
@@ -0,0 +1,34 @@
/*
Copy link
Member Author

Choose a reason for hiding this comment

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

Using the Apache header as this is has been written by me.

@samueleresca samueleresca changed the title Verify Jakarta support Adding Jakarta 3.0 support Jun 2, 2024
@samueleresca samueleresca marked this pull request as ready for review June 3, 2024 20:48
@samueleresca samueleresca mentioned this pull request Jun 3, 2024
Open
pjfanning
pjfanning reviewed Jun 6, 2024
View reviewed changes
.github/workflows/check-build-test.yml Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
...ms/src/main/scala/org/apache/pekko/stream/connectors/jakartams/ConnectionRetrySettings.scala Show resolved Hide resolved
project/Dependencies.scala Outdated Show resolved Hide resolved
project/Dependencies.scala Outdated Show resolved Hide resolved
...st/scala/org/apache/pekko/stream/connectors/jakartams/scaladsl/CachedConnectionFactory.scala Outdated Show resolved Hide resolved
laglangyue
laglangyue reviewed Jun 7, 2024
View reviewed changes
...ms/src/main/scala/org/apache/pekko/stream/connectors/jakartams/ConnectionRetrySettings.scala Outdated
val maxRetries: Int) {

/** Time allowed to establish and start a connection. */
def withConnectTimeout(value: scala.concurrent.duration.FiniteDuration): ConnectionRetrySettings =
Copy link
Contributor

Choose a reason for hiding this comment

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

This API is intended for users to use, and usually user level parameters should be fully named instead of using value. The same goes for other places, such as c for config

Copy link
Member Author

Choose a reason for hiding this comment

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

@laglangyue Thanks for spotting this. I have updated please let me know if my understanding is right.

The same goes for other places, such as c for config

I'm not sure I follow. What's c for config are you referring to?

@pjfanning pjfanning changed the title Adding Jakarta 3.0 support Adding Jakarta Messaging 3.0 support Jun 21, 2024
@pjfanning
Copy link
Contributor

@samueleresca is this PR complete? I'm hoping to start a discussion about a Pekko Connectors 1.1.0-M1 release. This can be delayed until after that release if that suits you better.

@samueleresca
Copy link
Member Author

@samueleresca is this PR complete? I'm hoping to start a discussion about a Pekko Connectors 1.1.0-M1 release. This can be delayed until after that release if that suits you better.

There is a comment @laglangyue that I need to address, apart from that it is ready. It would be great to merge this for Pekko Connectors 1.1.0-M1.

@pjfanning pjfanning added this to the 1.1.0-M1 milestone Jun 24, 2024
@pjfanning
Copy link
Contributor

@samueleresca This is the last PR blocking the release. Again, I'm happy to move on with the M1 release without this and to have it merged prior to the full 1.1.0 release.

@samueleresca
Copy link
Member Author

@samueleresca This is the last PR blocking the release. Again, I'm happy to move on with the M1 release without this and to have it merged prior to the full 1.1.0 release.

@pjfanning The PR is ready apart few minor comments that I think shouldn't block the merge. So I'm happy to get the approval and get this merged

pjfanning
pjfanning approved these changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm

@samueleresca samueleresca merged commit deccaf7 into apache:main Jul 2, 2024
52 checks passed
@pjfanning pjfanning mentioned this pull request Jul 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laglangyue laglangyue laglangyue left review comments

@pjfanning pjfanning pjfanning approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.1.0-M1
Development

Successfully merging this pull request may close these issues.
3 participants
@samueleresca @pjfanning @laglangyue