FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Support for ActiveMQ-Artemis messaging tracing. by CzyerChen · Pull Request #670 · apache/skywalking-java · GitHub

Support for ActiveMQ-Artemis messaging tracing. - #670

Merged
wu-sheng merged 4 commits into
apache:mainfrom
CzyerChen:feature/activemq-artemis-2.x-plugin
Jan 29, 2024
Merged

Support for ActiveMQ-Artemis messaging tracing.#670
wu-sheng merged 4 commits into
apache:mainfrom
CzyerChen:feature/activemq-artemis-2.x-plugin

Conversation

Copy link
Copy Markdown
Contributor

Add an agent plugin to support ActiveMQ-Artemis

  • Add a test case for the new plugin, refer to the doc

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #11830.

  • Update the CHANGES log.

Comment on lines +18 to +19
2.30.0
2.31.2

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could we support more versions? I saw versions in the docs are 2.30 and 2.31 only

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

for the testcase depends on spring-boot-3.2.0 and artemis-jakarta-client , it can support

  • not 2.19.0, and 2.19.0before not tested
  • not 2.24.0
  • not 2.25.0
  • 2.26.0
  • 2.27.0
  • 2.28.0
  • not 2.29.0
  • 2.30.0
  • 2.31.x

the version not supprt because of the dependency conflicts with SpringBoot, and springboot-stater-artemis officially support 3 versions of Artemis with jakarta-client (2.26.0,2.28.0,2.31.2)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

From you description, I think we should say we support

  • Spring Starter Artemis + ActiveMQ-Artemis a/b/c versions
  • ActiveMQ-Artemis a/b/c/e/f/g versions.

We should have two test scenarios to cover two version lists.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

artemis-jakarta-client has many minor versions, do the support-list.md have to record all the last version number of each minor version.

wu-sheng Jan 29, 2024
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

support-list.md should list all the range. support-version.list should pick the latest patch version of every minor for verification. Such as, we could say, we support 2.19 - 2.31, and test 2.19.1 for 2.19.x, 2.23.1 for 2.23.x, 2.31.2 for 2.31.x ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It can support all artemis-jakarta-client versions from 2.17.0 to 2.31.2 by running the testcase project, but the official docker image apache/activemq-artemis just provide from 2.30.0,. Can I written support 2.17.0 ~ 2.31.2, and test 2.30.0/2.31.2 in support-list.md?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, you can put it that way if you have verified locally or have checked from source codes level.

Suggested change
2.30.0
2.31.2
# You should put comments here about why others are not tested in the plugin test pipeline.
2.30.0
2.31.2

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

One more question, Images you provided are server side versions, could you use various client versions to connect 2.31 server?
What we need to verify is only client version compatibility, I believe the server version doesn't matter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ok, I can fix the server version, to check the client version.

wu-sheng added this to the 9.2.0 milestone Jan 26, 2024

Copy link
Copy Markdown
Member
-activemq-artemis-jakata-client-2.x
+activemq-artemis-jakarta-client-2.x

Please notice the name.

Copy link
Copy Markdown
Member

Please fix the javadoc. Warning is fine, notice the error ones, which should be from your codes.

Copy link
Copy Markdown
Member

Is anything wrong for RocketMQ grpc case?

Copy link
Copy Markdown
Contributor Author

Is anything wrong for RocketMQ grpc case?

I try to check again.

wu-sheng left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

wu-sheng merged commit 5cd5265 into apache:main Jan 29, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL