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

ActiveMQ Artemis: Parameters provided in restApiTemplate are again needed to be set in metadata. #2097

Closed
Ritikaa96 opened this issue Sep 9, 2021 · 7 comments · Fixed by #2104
Labels
bug Something isn't working

Comments

@Ritikaa96
Copy link
Contributor

Report

While testing ActiveMQ Artemis Scaler, it was found that even if I provide restAPITemplate, I still would need to provide the parameters : brokerName, brokerAddress, managementEndpoint,queueName.
The restAPITemplate can be generated by these parameters as it's as follows:

"http://<<managementEndpoint>>/console/jolokia/read/org.apache.activemq.artemis:broker=\"<<brokerName>>\",component=addresses,address=\"<<brokerAddress>>\",subcomponent=queues,routing-type=\"anycast\",queue=\"<<queueName>>\"/MessageCount"

On further investigation I stumble upon this function to generate the restAPITemplate here
which again is confusing by the name.

Are we doing this knowingly?

Expected Behavior

  • Either restAPITemplate should be generated solely by the given parameters as given in the function.
    or
  • the parameters : brokerName, brokerAddress, managementEndpoint,queueName; should not be required when restAPITemplate is given.

Actual Behavior

All Parameters: brokerName, brokerAddress, managementEndpoint,queueName, are needed even if restAPITemplate is given.

Steps to Reproduce the Problem

  1. Deploy Activemq Artemis.
  2. Use the SO without parameters: brokerName, brokerAddress, managementEndpoint,queueName.
  3. Deploy the scaledObject.

Logs from KEDA operator

No response

KEDA Version

2.4.0

Kubernetes Version

1.22

Platform

Any

Scaler Details

ActiveMQ Artemis

Anything else?

No response

@Ritikaa96 Ritikaa96 added the bug Something isn't working label Sep 9, 2021
@zroubalik
Copy link
Member

I think you are right, IMHO if a parameter is provided in restApiTemplate it shouldn't be needed to be set again in the metadata.

@balchua @spoplavskiy @bonky42 WDYT?

@balchua
Copy link
Contributor

balchua commented Sep 10, 2021

I agree its either the restApiTemplate or the broker config.

@bonky42
Copy link
Contributor

bonky42 commented Sep 10, 2021

I agree, make sense to not set both

@Ritikaa96
Copy link
Contributor Author

Ritikaa96 commented Sep 10, 2021

@zroubalik @tomkerkhove @spoplavskiy @balchua there is already a function to provide restAPITemplate from the parameters, the only thing we need is to make the four params optional and a parser to get these parameters from the template URL
I'd be glad to work on this if everyone agrees.

@zroubalik
Copy link
Member

@Ritikaa96 that would be nice!

@balchua
Copy link
Contributor

balchua commented Sep 10, 2021

Thanks @Ritikaa96

@Ritikaa96
Copy link
Contributor Author

Ritikaa96 commented Sep 20, 2021

Hi @zroubalik , @tomkerkhove , @balchua I've added the functionality to parse out the parameters from restApiTemplate. PTAL

Now there are two options.

  1. we add a check to throw an error to users not to provide both restApiTemplate & brokerConfig in metadata.
  2. we just mention this information in our scaler's document because when we provide restAPITemplate the code doesn't try to search values of the parameters from config.

what do you suggest?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants