Feature/issue 158 provisioning alerts when consumed capacity exceeds throughput alarm threshold #174
Conversation
| @@ -61,17 +63,19 @@ | |||
| 'sns_message_types': [] | |||
| }, | |||
| 'gsi': { | |||
| 'reads-alarm-threshold': 90, | |||
| 'writes-alarm-threshold': 90, | |||
There was a problem hiding this comment.
Note to self:
- Look at whether this should be defaulted to
0or not
There was a problem hiding this comment.
I guess the default can be 0, if it's 0, the alarm will never get triggered.
| @@ -44,6 +44,8 @@ Important note: The table name is treated as a regular expression. That means th | |||
| ========================================== ==== ========================================== | |||
| Option Type Comment | |||
| ========================================== ==== ========================================== | |||
| reads-alarm-threshold int How many percent of the reads capacity should be used before trigging the throughput alarm? | |||
| writes-alarm-threshold int How many percent of the writes capacity should be used before trigging the throughput alarm? | |||
There was a problem hiding this comment.
I think that we should implement those as:
reads-upper-alarm-thresholdreads-lower-alarm-thresholdwrites-upper-alarm-thresholdwrites-lower-alarm-threshold
There was a problem hiding this comment.
Why lower-alarm-threshold? I would only be interested to get alert when the throughput exceed my threshold, like the basic throughput alarm.
|
Thank you @Jofen for a very well written pull request! It's a pleasure to see it :). I have a few comments on the request that I would like to get sorted before merging and releasing:
|
|
Hello, Agree with #2 and #3. For #1, what exactly is the purpose of a lower alarm threshold? My reason for adding this feature is as follows:
Jiaofen |
|
Thanks for the additional commit, it looks good. My reasoning behind 1) is about being coherent to your 1) reason for the feature. Dynamic DynamoDB has a lower and a higher threshold and should scale if the current provisioning is outside those boundaries, thus having an alarm for the higher case, but not for the lower seems confusing. Although I do agree that the use case for the high alarm is more valuable. |
|
Make sense, I will change to include both upper-alarm-threshold and lower-alarm-threshold. :) |
|
Hello, Is this going to be merged and released soon? Thanks, Jiaofen |
|
Sorry, I had missed that you had completed the last things from the code review. Will take a look at that within the next few days and merge. I just got my second child, so my time is a bit limited at the time. |
|
👍 No kidding! Congratulations!! :) |
|
This is now merged to the |
|
This has now been released in 1.16.0. Thank you very much for the pull request @Jofen! |
As per discussed here: #158.