-
Notifications
You must be signed in to change notification settings - Fork 95
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
Added targetMetric field in CRD #168
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajanth97 thanks for this! two requests
go.mod
Outdated
@@ -3,8 +3,14 @@ module github.com/kedacore/http-add-on | |||
go 1.16 | |||
|
|||
require ( | |||
github.com/armon/consul-api v0.0.0-20180202201655-eb2c6b5be1b6 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajanth97 I'm not seeing why all these new dependencies need to be added. could you try running go mod tidy
? it should get rid of these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that fixed it
scaler/handlers.go
Outdated
@@ -60,15 +63,30 @@ func (e *impl) StreamIsActive( | |||
} | |||
} | |||
|
|||
func getTargetMetric() int64 { | |||
targetMetricStr, err := env.Get("KEDA_HTTP_SCALER_TARGET_METRIC") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajanth97 can you read this environment variable in the main
function near where all the other configs are read, and then pass the value into this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made new changes, please check it
Don't forget to update the Helm chart as well please on kedacore/charts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajanth97 can you remove the mage
binary from this PR? As a sidenote, you might want to consider putting it somewhere in your PATH
so you can use it anywhere
go.mod
Outdated
@@ -17,6 +17,7 @@ require ( | |||
google.golang.org/grpc v1.33.2 | |||
google.golang.org/protobuf v1.26.0 | |||
k8s.io/api v0.20.4 | |||
k8s.io/apiextensions-apiserver v0.20.2 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I removed the mage binary and added it to my PATH. And yes the go.mod file still shows those depenedencies
operator/config/rbac/role.yaml
Outdated
@@ -4,7 +4,7 @@ apiVersion: rbac.authorization.k8s.io/v1 | |||
kind: ClusterRole | |||
metadata: | |||
creationTimestamp: null | |||
name: keda-http-addon-manager-role | |||
name: keda-http-manager-role |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajanth97 did the mage generate
command change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used mage dockerBuildAll, I am not really sure what caused this change
This requires kedacore/http-add-on#168 Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>
Corresponding chart change in kedacore/charts#153 |
This requires kedacore/http-add-on#168 Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>
@ajanth97 this looks good! I want to hold on this until the 0.1.0 release, which should be very soon. stand by! |
feel free to resolve conflicts in the meantime though |
Sure |
@ajanth97 this is ready to merge. can you resolve conflicts? |
Thanks, Sure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work. I just got confused on the name targetMetric
, maybe this will generate some confusion as it is a bit generic.
Maybe scaleRequestAmount
(I'm terrible at naming)
@@ -85,6 +93,10 @@ spec: | |||
- port | |||
- service | |||
type: object | |||
targetMetric: | |||
description: (optional) Target metric value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a bit more explanation here? I think people will get lost on what this is for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, Yeah sure I will add some explanation. As for the naming yeah lets change it if its causing confusion. However I feel scaleRequestAmount
is a bit counterintuitive since the lower this value the faster it scales. Let me know your thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about queueMinScaleSize
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes more sense but is a bit too long, How about targetQueueSize
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing about the target
is that it seems that it'll only scale once it hits this exact number. What about scaleReqCount
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frankly, if you do not know how it works using queue
is very confusing as you are using HTTP traffic and not a queue.
When scaling HTTP, they don't care about the inner workings - They just want it to scale to meet the demand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if @zroubalik likes the name as well then that's fine for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomkerkhove how about targetPendingRequests
then? I agree with you RE the queue
nomenclature, but I think if a user wants to tweak this, then they will be interested enough in inner workings to learn about how the Addon treats pending requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomkerkhove how about
targetPendingRequests
then? I agree with you RE thequeue
nomenclature, but I think if a user wants to tweak this, then they will be interested enough in inner workings to learn about how the Addon treats pending requests.
I think you might be surprised about how many people will (want) to define their own number, without fully understanding/caring how it works internally.
Name is fine for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might be surprised about how many people will (want) to define their own number, without fully understanding/caring how it works internally.
if they do want to do so, and don't understand/care how it works internally, we'll have good documentation (which I'll submit in a follow-up to this PR, see #230) to explain it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good with me, but I'd like to hear from you @tomkerkhove about the name. this PR currently is using targetPendingRequests
Head branch was pushed to by a user without write access
ddb3f4b
to
dc3fe95
Compare
RELEASE-PROCESS 2.md
Outdated
@@ -0,0 +1,43 @@ | |||
# Release Process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajanth97 can you remove this document? it appears to be a duplicate of the release process document
docs/developing 2.md
Outdated
@@ -0,0 +1,163 @@ | |||
# Developing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajanth97 same here regarding the duplicate document
Head branch was pushed to by a user without write access
@@ -115,6 +115,8 @@ type HTTPScaledObjectSpec struct { | |||
// (optional) Replica information | |||
//+optional | |||
Replicas ReplicaStruct `json:"replicas,omitempty"` | |||
//(optional) Target metric value | |||
TargetMetric int32 `json:"targetMetric,omitempty" description:"The target metric value for the HPA (Default 100)"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajanth97 you'll need to change this to TargetPendingRequests
as well, and then re-generate the CRD
Signed-off-by: Ajanth <ajanth1997@gmail.com>
KEDA was accepted as CNCF Incubation project as per cncf/toc#622 Signed-off-by: Tom Kerkhove <kerkhove.tom@gmail.com> Signed-off-by: Ajanth <ajanth1997@gmail.com>
Signed-off-by: Ajanth <ajanth1997@gmail.com>
Signed-off-by: Ajanth <ajanth1997@gmail.com>
Signed-off-by: Ajanth <ajanth1997@gmail.com>
Signed-off-by: Ajanth <ajanth1997@gmail.com>
Signed-off-by: Ajanth <ajanth1997@gmail.com>
Signed-off-by: Ajanth <ajanth1997@gmail.com>
2df285b
to
bb65cb7
Compare
Thank you @arschles for helping out with this PR |
* Forgot to sign last commit Signed-off-by: Ajanth <ajanth1997@gmail.com>
Provide a description of what has been changed
I added the target metric field in the CRD.
Checklist
README.md
docs/
Fixes #