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

Service bus queue #101

Merged
merged 12 commits into from
Apr 23, 2019
Merged

Service bus queue #101

merged 12 commits into from
Apr 23, 2019

Conversation

lee0c
Copy link
Contributor

@lee0c lee0c commented Apr 20, 2019

Implements #15

Additionally:

  • Fixes some spelling errors
  • Removes calls to getConnectionString & getQueueName in azure_queue_scaler that were not removed when the functions in question were
  • Fixes an error w/ a function call in the kafka_scaler test file
  • switches scalers from TargetAverageValue to TargetValue metrics (Avg value is for averages across pods & doesn't make sense for a message queue) Read docs wrong, this is reverted
  • Adds typeMeta to the HPA spec so that external metrics can be used

@Aarthisk
Copy link
Contributor

I believe TargetAverageValue is correct here- the idea being we try to estimate the number of messages a function pod can process- in the PAAS offering for functions for example it is 1000 messages per worker before we scale and add another worker.

@lee0c
Copy link
Contributor Author

lee0c commented Apr 22, 2019

@Aarthisk you're entirely right, I was reading that wrong the other day. Let me revert that change

@@ -467,6 +469,8 @@ func (h *ScaleHandler) getScaler(trigger kore_v1alpha1.ScaleTriggers, resolvedEn
switch trigger.Type {
case "azure-queue":
return scalers.NewAzureQueueScaler(resolvedEnv, trigger.Metadata)
case "serviceBusTrigger": // TODO in func core tools probably: define type for this
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather this be azure-servicebus to be consistent with other triggers here. I think rabbitMQ should be rabbitmq as well, but that can go in a separate PR.

I don't understand the TODO though. The core-tools will have its own mapping of azure functions trigger name <=> keda trigger name.

Azure Functions trigger name Keda trigger name
queueTrigger azure-queue
blobTrigger azure-blob
serviceBusTrigger azure-servicebus
kafkaTrigger kafka
httpTrigger N/A

Copy link
Contributor Author

Choose a reason for hiding this comment

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

theTODO was in relation to making sure this got cleaned up on the core tools side - right now it still goes through as serviceBusTrigger afaik. If core tools is updated I can switch this to azure-servicebus - though will SB Queues & SB Topics/Subscriptions have different triggers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahmelsayed poking through func cli looks like Queues & Topics have the same trigger name but different metadata - I think @priyaananthasankar & I will be looking at implementing Topics scaler later so we can sort out how to differentiate between the two then.

Regardless, will push the change for serviceBusTrigger -> azure-servicebus

@ahmelsayed ahmelsayed merged commit aa61d2b into master Apr 23, 2019
@ahmelsayed ahmelsayed deleted the service_bus_queue branch April 24, 2019 22:49
patnaikshekhar pushed a commit to patnaikshekhar/keda that referenced this pull request Jul 17, 2019
Service bus queue

Former-commit-id: aa61d2b
Aarthisk pushed a commit that referenced this pull request Aug 12, 2019
Service bus queue

Former-commit-id: 1275c68
preflightsiren pushed a commit to preflightsiren/keda that referenced this pull request Nov 7, 2021
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.

3 participants