Target Group Quota introduction - #3337
Conversation
Signed-off-by: strailov <Stanislav.Trailov@bosch.io>
| @Transactional | ||
| @Retryable(includes = ConcurrencyFailureException.class, maxRetriesString = Constants.RETRY_MAX, delayString = Constants.RETRY_DELAY) | ||
| public JpaTarget create(final TargetManagement.Create create) { | ||
| assertTargetGroupQuota(Collections.singletonList(create.getGroup())); |
There was a problem hiding this comment.
why here we don't check for group == null, but on some places we do check>?
same on update
There was a problem hiding this comment.
We actually do on later point - on wanted.isEmpty() in the assertTargetGroupQuota. It is possible to directly check in the create method, but I have decided the code would become more complex to read idk ...
There was a problem hiding this comment.
So I guess the two guards in assign methods are redundant here ... Will remove them in order to comply with all the others.
| } | ||
|
|
||
| private void assertTargetGroupQuota(final Collection<String> requested) { | ||
| final long limit = quotaManagement.getMaxTargetGroups(); |
There was a problem hiding this comment.
if wanted is empty - no need to getMaxTargetGroups, which at some point could become db managed
There was a problem hiding this comment.
Fair point, will switch the order
|
|
||
| final SortedSet<String> wanted = requested.stream() | ||
| .filter(Objects::nonNull) | ||
| .collect(Collectors.toCollection(() -> new TreeSet<>(String.CASE_INSENSITIVE_ORDER))); |
There was a problem hiding this comment.
db could be case sensitive?
There was a problem hiding this comment.
Ah, agreed - I'll let the db collation do the job without specifying in java for it
| if (wanted.size() == 1 && jpaRepository.existsByGroup(wanted.first())) { | ||
| return; | ||
| } | ||
| final List<String> existing = jpaRepository.findDistinctGroups(AccessContext.tenant()); |
There was a problem hiding this comment.
it could be the case that updating a group other is removed, e.g.
update group x -> y, if device is the only in x - then x is removed y is created - no need to check.
I wonder if it will be better if we do a "relaxed" check - i.e. if there are already 100 groups - we don't allow not existing, though - this also could be wrong if we "remove" groups
There was a problem hiding this comment.
Didn't quite get the second one - don't we actually currently do that ? We don't allow not existing groups (currently) when the limit is reached .
Fair point for the first case with x and y, however not quite sure currently how could we handle it painlessly.
Most of those cases some devices are left with group x which could lead to hitting the limit, however if exactly those devices that has x goes to y this is like more renaming and we won't work in that case, yes. Not sure if we should mitigate this - workaround is present, just unassign X and assign Y.
Signed-off-by: strailov <Stanislav.Trailov@bosch.io>
Signed-off-by: strailov <Stanislav.Trailov@bosch.io>
2 participants
Introduction
Limits the number of distinct target groups a tenant may create.
Quota check on each write case for target group - create/update (+bulk case) targets if groups is present and assigning group to target(s).
Default quota set to 100 with options to be increased/decreased.
Behaviour
nullgroup never consumes quota — unassignment always allowed<= 0means unlimitedso introducing a limit never breaks existing tenants
n + kis checked ratherthan
n + 1repeatedlyAssignmentQuotaExceededExceptionupdate, so a bad id stillreports
EntityNotFoundException