Fix setTag logic to prevent high cpu load on rds #572
Conversation
stevie9868
force-pushed
the
yingjianw/fixSetTagLatency
branch
2 times, most recently
from
fcad1b4 to
048cd19
Compare
…d of loading from memory + test
stevie9868
force-pushed
the
yingjianw/fixSetTagLatency
branch
from
048cd19 to
c2f395d
Compare
| } | ||
|
|
||
| @Unroll | ||
| def "test includeValues"() { |
There was a problem hiding this comment.
Can you add some additional tests featuring duplicates and multiple call iterations
There was a problem hiding this comment.
Sorry, could you elaborate more?
I think I have covered them also in the functional test.
There was a problem hiding this comment.
ok looks like the functional test can cover the iterations part, could you add another unit test featuring if a caller tries to add duplicate tag in one call like pass in the same tag multiple times in the input list
| "delete from lookup_values where lookup_id=? and values_string in (%s)"; | ||
| private static final String SQL_INSERT_LOOKUP_VALUE_IF_NOT_EXIST = | ||
| "INSERT INTO lookup_values(lookup_id, values_string) VALUES (?,?) " | ||
| + "ON DUPLICATE KEY UPDATE lookup_id=lookup_id, values_string=values_string"; |
There was a problem hiding this comment.
This is functionally correct though for high usage tags it might lead to continuously doing useless updates, just checking do you have any way to do this with IGNORE instead.
| } | ||
|
|
||
| @Unroll | ||
| def "test includeValues"() { |
There was a problem hiding this comment.
ok looks like the functional test can cover the iterations part, could you add another unit test featuring if a caller tries to add duplicate tag in one call like pass in the same tag multiple times in the input list
| } | ||
| if (!inserts.isEmpty()) { | ||
| insertLookupValues(lookup.getId(), inserts); | ||
| if (includeValues) { |
There was a problem hiding this comment.
the previous code didn't even add back the new tags to the return result, but based on usage it does not make a difference either way
There was a problem hiding this comment.
yep, you are right but I want to make it consistent.
For now, whenever we want to set a tag on a resource, it will always fetch all tags from the dbs into memory, and then do a diff to figure out what tags never exist in the db. Loading all tags at once creates a lot of load on the rds if the number of tags is huge when number of setTag request is high.
For this PR, I also remove some unused code.
Instead of fetching all tags into memory, we will instead do
Even though, there is no unit test for tag component, there are lots of existing tests in the functional test, and I also add one more functional test to cover more scenarios.
Since the logic I change should have no impact on the core logic, we should expect the functionalTest to have the same result before and after this change.
Finally, in order to make this insert query fast, we will need to build a composite index on (lookup_id, values_string)