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

[v2] Consolidate apiHost & host for RabbitMQ scaler #1115

Merged
merged 5 commits into from
Sep 11, 2020

Conversation

turbaszek
Copy link
Contributor

@turbaszek turbaszek commented Sep 8, 2020

Signed-off-by: Tomek Urbaszek tomasz.urbaszek@polidea.com

Consolidate apiHost & host for RabbitMQ scaler

Checklist

Relates to #711

@turbaszek turbaszek marked this pull request as ready for review September 9, 2020 16:22
@turbaszek
Copy link
Contributor Author

turbaszek commented Sep 9, 2020

@tomkerkhove @zroubalik can you please take a look? Eventually, do we have a RabbitMQ expert?

@ahmelsayed ahmelsayed changed the title Consolidate apiHost & host for RabbitMQ scaler [v2] Consolidate apiHost & host for RabbitMQ scaler Sep 9, 2020
@ahmelsayed
Copy link
Contributor

since this is a breaking change for v2, can you also update the CHANGELOG

@tomkerkhove
Copy link
Member

Good point! And our migration guide in the docs as well please.

@tomkerkhove
Copy link
Member

Looks like we only need a PR for it to https://github.com/kedacore/keda-docs

@zroubalik
Copy link
Member

@holyketzer @tbickford could you please take a look on this?

@turbaszek
Copy link
Contributor Author

Looks like we only need a PR for it to https://github.com/kedacore/keda-docs

Yeah, I will do in the evening

@turbaszek
Copy link
Contributor Author

@tomkerkhove do you think we need protocol specification? I think we may just resolve it using the host?

@turbaszek
Copy link
Contributor Author

turbaszek commented Sep 10, 2020

Docs update: kedacore/keda-docs#253

@@ -167,7 +158,7 @@ func (s *rabbitMQScaler) IsActive(ctx context.Context) (bool, error) {
}

func (s *rabbitMQScaler) getQueueMessages() (int, error) {
if s.metadata.includeUnacked {
if s.metadata.protocol == httpProtocol {
Copy link
Member

Choose a reason for hiding this comment

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

With this change, if http protocol is used, unacked messages are always included and it is no longer optional as it was before. Am I missing anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I think this name was confusing. From what I understand the HTTP protocol (apiHost) was used only when includeUnacked==True otherwise this parameter was unused (amqp case):

includeUnacked bool // if true uses HTTP API and requires apiHost, if false uses AMQP and requires host

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done it, I think it's much more userfriendly to just provide host than provide host and partial information from it

closes kedacore#711

Signed-off-by: Tomek Urbaszek <tomasz.urbaszek@polidea.com>
Signed-off-by: Tomek Urbaszek <tomasz.urbaszek@polidea.com>
Signed-off-by: Tomek Urbaszek <tomasz.urbaszek@polidea.com>
Signed-off-by: Tomek Urbaszek <tomasz.urbaszek@polidea.com>
Signed-off-by: Tomek Urbaszek <tomasz.urbaszek@polidea.com>
@tomkerkhove
Copy link
Member

@tomkerkhove do you think we need protocol specification? I think we may just resolve it using the host?

I think protocolis a great addition because maybe today you can figure it out based on the host but:

  1. It's a lot easier for people to understand what is being used if they are not a KEDA expert
  2. We are ready in case we want to add new ones later on

@turbaszek
Copy link
Contributor Author

  1. It's a lot easier for people to understand what is being used if they are not a KEDA expert

Then using s single uri is the simplest thing I think.

  1. We are ready in case we want to add new ones later on

The protocol which is defined by a schema of the host can be determined automatically. And as long as the schema is provided (probably always) we should be able to figure it out for users.

If we agree to have protocol param then I will revert the last commit

@tomkerkhove
Copy link
Member

I would prefer to keep it because if you don't use a literal value in the scaler metadata you have to go check somewhere else. From a ops/management team I prefer not to have to do that.

@turbaszek
Copy link
Contributor Author

I would prefer to keep it because if you don't use a literal value in the scaler metadata you have to go check somewhere else.

That's a good point

@turbaszek
Copy link
Contributor Author

Removed the last commit, please take a look at the code as well as the related docs PR: kedacore/keda-docs#253

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.

Thanks!

@zroubalik zroubalik merged commit 2b10aca into kedacore:v2 Sep 11, 2020
SpiritZhou pushed a commit to SpiritZhou/keda that referenced this pull request Jul 18, 2023
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.

4 participants