feat: resource monitor sdk v2 #1892
Conversation
…nowflake into resource-monitor-sdk-v2
…nowflake into resource-monitor-sdk-v2
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Integration tests failure for 5af4bbb4f6d945109074a9c4cb23eb13e1418a13 |
|
Integration tests failure for ae7c85ac9a3d5f319d6c3f47a106681cd4d0ebc0 |
|
Integration tests failure for e0a8eb44c8733b19af6ab2194994261600c200c3 |
|
Integration tests failure for ea081ce3fc1e8c196f7b3c1c66a1f4fb553fbe7b |
1 similar comment
|
Integration tests failure for ea081ce3fc1e8c196f7b3c1c66a1f4fb553fbe7b |
|
Integration tests failure for 774b4ed3319bfaddcbda4362d2a00447f0e68281 |
|
Integration tests failure for 5bd0028e7675502b79a4613ca5716db566889838 |
|
Integration tests failure for d0c41146c9eb226393f6159721cce130ec1a1163 |
|
Integration tests failure for 361e3827b5c71a571f31a9ec9c26ff82b2754f28 |
…nowflake into resource-monitor-sdk-v2
|
Integration tests failure for 47724ae0a0b3e1820b1a0d49beda8dd8fb3d961a |
|
Integration tests failure for c2d0525e3380a7169484500ad832e9308f6d1ab7 |
|
Integration tests failure for f6f2f3c00b61f6967fc3978d31c75b3144a8e49e |
|
Integration tests success for e80d0297c334ae1000d88c0c2874525a1b01aaaa |
…nowflake into resource-monitor-sdk-v2
pkg/sdk/resource_monitors.go
Outdated
| Frequency Frequency | ||
| StartTime string | ||
| EndTime string | ||
| SuspendTriggers []TriggerDefinition |
There was a problem hiding this comment.
unlike notify, you cannot have more than one suspend trigger. suspend trigger is at a given percent, and if that threshold is reached the warehouse is suspended. so it doesn't make sense to have two triggers as only the lower value will result in anything. I also verified this in Snowflake just now.
There was a problem hiding this comment.
it may make sense to have this just be an int. i.e. SuspendAt *int
There was a problem hiding this comment.
Ok, I guess I didn't want to change the behaviour. I thought suspend_triggers still might be used under some circumstances.
So what should we do about the suspend_triggers argument then?
I think we can:
- just get rid of it
- ignore it and just take the value from
suspend_trigger - use the trigger with the lowest threshold from
suspendTriggerandsuspend_triggers
I think we should either remove it or at least explain the changed semantics. Otherwise it might be confusing for the users.
There was a problem hiding this comment.
suspend_triggers is a deprecated attribute since it never makes sense to have more than one suspend trigger. We have to continue to support it until the next major provider release (1.0). What i would do is just choose the lowest value in the list and use that
pkg/sdk/resource_monitors.go
Outdated
| StartTime string | ||
| EndTime string | ||
| SuspendTriggers []TriggerDefinition | ||
| SuspendImmediateTriggers []TriggerDefinition |
There was a problem hiding this comment.
same thing as for suspend, you cannot have a list of triggers. perhaps this should be SuspendImmediateAt *int
There was a problem hiding this comment.
same as above, waiting until we agree on the solution
There was a problem hiding this comment.
Yeah just use the lowest value, thats the only thing that makes sense anyways
pkg/sdk/resource_monitors.go
Outdated
| resourceMonitor bool `ddl:"static" sql:"RESOURCE MONITOR"` //lint:ignore U1000 This is used in the ddl tag | ||
| name AccountObjectIdentifier `ddl:"identifier"` | ||
| With *bool `ddl:"keyword" sql:"WITH"` |
There was a problem hiding this comment.
With is a static value, i think. Its not a required or optional parameter, its just there.
There was a problem hiding this comment.
Well, if we create empty resource monitor, then the query won't compile(create resource monitor 'foo' with is not ok)
There was a problem hiding this comment.
In that case would it make sense to have With be a separate type?
for example:
With *ResourceMonitorWith `ddl:"keyword" sql:"WITH"`
and then later have the ResourceMonitorWith type declared which has all the parameters in it?
…nowflake into resource-monitor-sdk-v2
|
Integration tests success for 231ee39a27a20790403d49a918466d96161b486b |
|
Integration tests failure for b97fbe7100848a3ef9f7881be26df3af863de22c |
|
Integration tests failure for 42143e9aebda46d78d39c4dc581ca962f4a07cb2 |
|
Integration tests failure for e0f197f91aa28cee5c540cf8414b4dc12df1a969 |
…nowflake into resource-monitor-sdk-v2
|
Integration tests failure for e13f68899261c1d191e38455cbbd5e9fd82b26d9 |
|
Integration tests failure for 532be5f22f26b9703249f07501a7c31c8a400cc3 |
1 similar comment
|
Integration tests failure for 532be5f22f26b9703249f07501a7c31c8a400cc3 |
…nowflake into resource-monitor-sdk-v2
…nowflake into resource-monitor-sdk-v2
|
Integration tests failure for 9cf8d4533ae60c07cb1f80f9ecf7c77958683ab5 |
|
Integration tests success for 2c66d83d0edec551bbe7fd618f00825061898287 |
Test Plan