Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8b3bfc9
Optimize FORCE policyType for REPLACE_VALUES operation
LiquidFenrir Dec 18, 2023
ca52c16
Optimize more: allow replace when better to do so
LiquidFenrir Dec 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions src/main/java/org/lsc/beans/BeanComparator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ private static LscModifications getUpdatedObject(
LscDatasetModificationType operationType = getRequiredOperationForAttribute(toSetAttrValues, dstAttrValues);

// Build the modification
List<LscDatasetModification> multiMi = null;
LscDatasetModification mi = null;
switch (operationType) {
case DELETE_VALUES:
Expand Down Expand Up @@ -320,10 +321,32 @@ private static LscModifications getUpdatedObject(

case REPLACE_VALUES:
if (attrStatus == PolicyType.FORCE) {
if (isModified(dstBean, dstAttrValues, toSetAttrValues)) {
// check if there are any extra values to be added
Set<Object> missingValues = SetUtils.findMissingNeedles(dstAttrValues, toSetAttrValues);
// check if there are any extra values to be removed
Set<Object> extraValues = SetUtils.findMissingNeedles(toSetAttrValues, dstAttrValues);

if((missingValues.size() + extraValues.size()) >= toSetAttrValues.size()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// More things to add and delete than remaining in the final set
// so, replace with the final set directly.
LOGGER.debug("{} Replacing attribute \"{}\": source values are {}, old values were {}, new values are {}",
new Object[]{logPrefix, attrName, srcAttrValues, dstAttrValues, toSetAttrValues});
mi = new LscDatasetModification(operationType, dstAttr.getID(), toSetAttrValues);
} else {
// Adding and deleting the values is less expensive than replacing everything
multiMi = new ArrayList<LscDatasetModification>(2);

if (missingValues.size() > 0) {
LOGGER.debug("{} Adding values to attribute \"{}\": new values are {}",
new Object[]{logPrefix, attrName, missingValues});
multiMi.add(new LscDatasetModification(LscDatasetModificationType.ADD_VALUES, dstAttr.getID(), missingValues));
}

if (extraValues.size() > 0) {
LOGGER.debug("{} Removing values from attribute \"{}\": old values are {}",
new Object[]{logPrefix, attrName, extraValues});
multiMi.add(new LscDatasetModification(LscDatasetModificationType.DELETE_VALUES, dstAttr.getID(), extraValues));
}
}
} else if (attrStatus == PolicyType.MERGE) {
// check if there are any extra values to be added
Expand All @@ -339,9 +362,11 @@ private static LscModifications getUpdatedObject(
break;
}

if (mi == null) {
if (mi == null && multiMi == null) {
LOGGER.debug("{} Attribute \"{}\" will not be written to the destination", logPrefix, attrName);
} else {
} else if (multiMi != null) {
modificationItems.addAll(multiMi);
} else if (mi != null) {
modificationItems.add(mi);
}

Expand Down
Loading