Optimization for multi-valued LDAP attributes such as groups uniqueMember #284
Conversation
Use smaller ADD and DELETE modifications instead of a huge REPLACE, at worst equivalent
|
Giving a look to this PR. |
There was a problem hiding this comment.
I like this PR, it is quite elegant in my opinion.
I have only 2 general comments:
- it would be really nice to have a corresponding test. Maybe we could grab the LSC log showing that we are in the condition:
(missingValues.size() + extraValues.size()) >= toSetAttrValues.size()? - as it is a behavior change for everyone, I think we must document this (in main documentation + in release notes)
| // check if there are any extra values to be removed | ||
| Set<Object> extraValues = SetUtils.findMissingNeedles(toSetAttrValues, dstAttrValues); | ||
|
|
||
| if((missingValues.size() + extraValues.size()) >= toSetAttrValues.size()) { |
There was a problem hiding this comment.
I am not sure at which point one replace is more efficient than 1 add + 1 delete.
Maybe it can be a configuration parameter? The condition would be:
(missingValues.size() + extraValues.size()) >= max_value_modifications
It would make the LSC modifications more predictable.
However I understand that depending on the entry size has also its benefits. For example, with the previous condition, if we had an entry with 1000 values, and max_value_modifications = 100, then 50 adds and 51 adds would lead to a big replace, which may be less efficient than adding 50 values + removing 51 values.
Without further efficiency analysis, I think we can keep the initial proposition in this PR.
I have just written the corresponding unit test demonstrating the correct selection of 1 big replace / 1 add + 1 delete. See this PR: #285 |
I have also written the documentation and upgrade notes in corresponding project, see: lsc-project/documentation#5 |
|
This PR is replaced by #285 |
See #255.
This pull request replaces #259 as per the discussion in its thread.
The goal is to make FORCE operations on large datasets faster by doing a few
addanddeleteinstead of one constantly hugereplacewhen it is beneficial to do so.The testing on this is nowhere near substantial, nor are any test cases updated to reflect the change in behaviour.
These changes were developed by INTM on behalf of EDF, who noticed the slowdown in real use, and the fix is succesfully working for them.
Please let us know if something is wrong or doesn't fit the guidelines adopted by the LSC project, so we can make any required modification before this can be merged.