search

lsc-project / lsc

LSC engine
http://lsc-project.org/wiki/documentation/latest/start
Other
108 stars 41 forks source link

Optimization for multi-valued LDAP attributes such as groups uniqueMember #284

Closed intmgroupe closed 4 months ago

intmgroupe commented 4 months ago

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 add and delete instead of one constantly huge replace when 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.

davidcoutadeur commented 4 months ago

Giving a look to this PR.

davidcoutadeur commented 4 months ago

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)

I have just written the corresponding unit test demonstrating the correct selection of 1 big replace / 1 add + 1 delete.

See this PR: https://github.com/lsc-project/lsc/pull/285

davidcoutadeur commented 4 months ago

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)

I have also written the documentation and upgrade notes in corresponding project, see: https://github.com/lsc-project/documentation/issues/5

davidcoutadeur commented 4 months ago

For me, everything is ready for merging. @rouazana and @soisik, I don't know if you have any more remarks?

davidcoutadeur commented 4 months ago

This PR is replaced by #285