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

Add ActiveMQ scaler #2305

Merged
merged 11 commits into from
Jan 3, 2022
Merged

Add ActiveMQ scaler #2305

merged 11 commits into from
Jan 3, 2022

Conversation

melisatanrverdi
Copy link
Contributor

@melisatanrverdi melisatanrverdi commented Nov 19, 2021 •
edited by JorTurFer
Loading

Signed-off-by: melisatanrverdi melisatanrverdi@gmail.com

Added a new scaler that scales based on ActiveMQ broker. Contacts ActiveMQ's management endpoint to determine the queue's message count and scale accordingly.

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on (repo) (if applicable) Add documentation for ActiveMQ Scaler keda-docs#611
  • Changelog has been updated

Fixes #2120

@melisatanrverdi
add new ActiveMQ scaler with it's unit test
d5d02a3 
Signed-off-by: melisatanrverdi <melisatanrverdi@gmail.com>
@melisatanrverdi
restored changes that build command caused
79b883c 
Signed-off-by: melisatanrverdi <melisatanrverdi@gmail.com>
@melisatanrverdi
Copy link
Contributor Author

Sorry for my late response @arschles , about unifying the Artemis and ActiveMQ scaler codebases question that you asked previous PR, I've thought Artemis and ActiveMQ classic as separately because of they haven't completed that migration: http://activemq.apache.org/activemq-artemis-roadmap.html
My opinion is we can consider unify the codeebases after the migration. What are your thoughts?

@arschles
Copy link
Contributor

My opinion is we can consider unify the codeebases after the migration. What are your thoughts?

@melisatanrverdi that sounds like a fine plan. Thanks for answering that comment from the previous PR!

@melisatanrverdi
Copy link
Contributor Author

I tried to use https://hub.docker.com/r/symptoma/activemq image in my e2e test because this is the only latest version. But it didn’t provide some of my configuration requirements. I tried to use configmap but it didn’t work for one of requirement.
I can patch that docker image according to my requirements or I can use my docker image that I created earlier to test ActiveMQ. I would like to hear your suggestions.

@JorTurFer
Copy link
Member

JorTurFer commented Nov 21, 2021 •
edited
Loading

I tried to use hub.docker.com/r/symptoma/activemq image in my e2e test because this is the only latest version. But it didn’t provide some of my configuration requirements. I tried to use configmap but it didn’t work for one of requirement.
I can patch that docker image according to my requirements or I can use my docker image that I created earlier to test ActiveMQ. I would like to hear your suggestions.

Hi @melisatanrverdi ,
If you need a custom image, you could create a PR here . We usually use custom images, so don't worry if you need to create a new one :)
Personally, I don't have a strong opinion about using and patching one existing image or creating it from scratch.

@tomkerkhove tomkerkhove added this to the v2.6.0 milestone Nov 23, 2021
@tomkerkhove
Copy link
Member

We are planning to ship KEDA v2.5 this week - Do you think you'll have time to complete this PR @melisatanrverdi or not?

No rush, we're just checking what will be in and what not.

@melisatanrverdi
add e2e test for activemq and update changelog file
c94b631 
Signed-off-by: melisatanrverdi <melisatanrverdi@gmail.com>
@melisatanrverdi melisatanrverdi requested a review from a team as a code owner December 2, 2021 16:10
@JorTurFer
Copy link
Member

Hi @melisatanrverdi
Could you update your branch?
Last week we released v2.5 and your branch's CHANGELOG.md is in conflict. Apart from that, you should update it to move your change from v2.5 to v2.6
Thanks for this contribution ❤️

@JorTurFer
Copy link
Member

JorTurFer commented Dec 2, 2021 •
edited by github-actions bot
Loading

/run-e2e activemq.test*
Update: You can check the progres here

@JorTurFer
Copy link
Member

JorTurFer commented Dec 2, 2021 •
edited by github-actions bot
Loading

/run-e2e activemq.test*
Update: You can check the progres here

@JorTurFer
Copy link
Member

hi @melisatanrverdi ,
I have tried the e2e test and it fails in the e2e cluster.
Could you review it? The logs are here. Please, review the cleanup too because after the failure, the resources are not cleaned.

arschles
arschles suggested changes Dec 2, 2021
View reviewed changes
pkg/scalers/activemq_scaler.go Show resolved Hide resolved
pkg/scalers/activemq_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/activemq_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/activemq_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/activemq_scaler_test.go Outdated Show resolved Hide resolved
@melisatanrverdi
Update repo and resolve conflict
f673baf 
Signed-off-by: melisatanrverdi <melisatanrverdi@gmail.com>
@melisatanrverdi
Turn rest API template into a text/template
a72454d 
Signed-off-by: melisatanrverdi <melisatanrverdi@gmail.com>
@melisatanrverdi
Add name field to unit test and configure functions accordingly
fcbb0a7 
Signed-off-by: melisatanrverdi <melisatanrverdi@gmail.com>
arschles
arschles suggested changes Dec 3, 2021
View reviewed changes
Copy link
Contributor

@arschles arschles left a comment

Choose a reason for hiding this comment

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

@melisatanrverdi I hope my comments helped answer your questions. I'm happy to provide more detail if you'd like. Just let me know.

pkg/scalers/activemq_scaler.go Show resolved Hide resolved
pkg/scalers/activemq_scaler.go Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

JorTurFer commented Dec 4, 2021 •
edited by github-actions bot
Loading

/run-e2e activemq.test*
Update: You can check the progres here

@melisatanrverdi
Fix e2e test fail, add suggested changes to scaler
4ec0558 
Signed-off-by: melisatanrverdi <melisatanrverdi@gmail.com>
@melisatanrverdi
Copy link
Contributor Author

hi @melisatanrverdi , I have tried the e2e test and it fails in the e2e cluster. Could you review it? The logs are here. Please, review the cleanup too because after the failure, the resources are not cleaned.

I was accessing health status endpoint of ActiveMQ via IP address. I changed this by sending from inside the pod. I will update CHANGELOG file after ensure that everthing works properly.

@JorTurFer
Copy link
Member

JorTurFer commented Dec 7, 2021 •
edited by github-actions bot
Loading

/run-e2e activemq.test*
Update: You can check the progres here

@melisatanrverdi
Merge branch 'main' into activeMQScaler
5cd0fac
@melisatanrverdi
Update CHANGELOG.md file
47edaaf 
Signed-off-by: melisatanrverdi <melisatanrverdi@gmail.com>
JorTurFer
JorTurFer requested changes Dec 7, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

LGTM! Only the small nit about CHANGELOG.md
@arschles PTAL

@melisatanrverdi
Replace TODO line with new ActiveMQ line
77df77d 
Signed-off-by: melisatanrverdi <melisatanrverdi@gmail.com>
zroubalik
zroubalik reviewed Dec 9, 2021
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Looking good, just one Q whether we want to expose metricName to the users.

Could you pleaes open documentation PR, thanks!

pkg/scalers/activemq_scaler.go Show resolved Hide resolved
pkg/scalers/activemq_scaler.go Outdated Show resolved Hide resolved
@melisatanrverdi melisatanrverdi mentioned this pull request Dec 10, 2021
Merged
1 task
@melisatanrverdi
Remove user-defined option of metricName parameter
509baf5 
Signed-off-by: melisatanrverdi <melisatanrverdi@gmail.com>
@melisatanrverdi
Copy link
Contributor Author

Is everything all right? Is there something I'm missing that is preventing the merge?

@JorTurFer
Copy link
Member

JorTurFer commented Dec 20, 2021 •
edited
Loading

Is everything all right? Is there something I'm missing that is preventing the merge?

Hi @melisatanrverdi ,
your PR looks good, but we'd like till another review and unfortunately we are totally busy till vacations. After x-mas we will take care about this PR, sorry for the inconvenience

zroubalik
zroubalik approved these changes Jan 3, 2022
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM, tahnks @melisatanrverdi for the contribution, this is great! And sorry for the delay :)

@zroubalik zroubalik merged commit eeefb10 into kedacore:main Jan 3, 2022
zroubalik pushed a commit to zroubalik/keda that referenced this pull request Jan 4, 2022
@melisatanrverdi
Add ActiveMQ scaler (kedacore#2305)
eb8bd0f 
Signed-off-by: melisatanrverdi <melisatanrverdi@gmail.com>
zroubalik pushed a commit that referenced this pull request Jan 4, 2022
@melisatanrverdi @zroubalik
Add ActiveMQ scaler (#2305)
5728c2f 
Signed-off-by: melisatanrverdi <melisatanrverdi@gmail.com>
alex60217101990 pushed a commit to dysnix/keda that referenced this pull request Jan 10, 2022
@melisatanrverdi @alex60217101990
Add ActiveMQ scaler (kedacore#2305)
29c4a02 
Signed-off-by: melisatanrverdi <melisatanrverdi@gmail.com>
Signed-off-by: alex60217101990 <alex6021710@gmail.com>
alex60217101990 pushed a commit to dysnix/keda that referenced this pull request Jan 10, 2022
@melisatanrverdi @alex60217101990
Add ActiveMQ scaler (kedacore#2305)
83d397b 
Signed-off-by: melisatanrverdi <melisatanrverdi@gmail.com>
Signed-off-by: alex60217101990 <alex6021710@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arschles arschles arschles requested changes

@zroubalik zroubalik zroubalik approved these changes

@JorTurFer JorTurFer JorTurFer approved these changes

@ahmelsayed ahmelsayed Awaiting requested review from ahmelsayed

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.6.0
Development

Successfully merging this pull request may close these issues.

Provide scaler for ActiveMQ Classic
5 participants
@melisatanrverdi @arschles @JorTurFer @tomkerkhove @zroubalik