rombert commented on code in PR #13:
URL:
https://github.com/apache/sling-org-apache-sling-installer-factory-configuration/pull/13#discussion_r1601372208
##########
src/main/java/org/apache/sling/installer/factories/configuration/impl/ConfigUtil.java:
##########
@@ -336,4 +337,20 @@ public static void removeRedundantProperties(final
Dictionary<String, Object> pr
}
}
}
+
+ public static String getPid(final ConfigurationEvent event) {
+ final String id;
+ final String pid;
+ if (event.getFactoryPid() == null ) {
Review Comment:
I think the code would be clearer if you would check if you need to after
the pid ( branch on line 347 ) and then have a fallback to `id =
event.getPid();`.
##########
src/main/java/org/apache/sling/installer/factories/configuration/impl/ConfigTaskCreator.java:
##########
@@ -131,10 +131,11 @@ public InstallTask createTask(final TaskResourceGroup
group) {
@Override
public void configurationEvent(final ConfigurationEvent event) {
synchronized ( Coordinator.SHARED ) {
+ final String id = ConfigUtil.getPid(event);
Review Comment:
I think we need a more significant name than "id".
##########
src/main/java/org/apache/sling/installer/factories/configuration/impl/ConfigTaskCreator.java:
##########
@@ -131,10 +131,11 @@ public InstallTask createTask(final TaskResourceGroup
group) {
@Override
public void configurationEvent(final ConfigurationEvent event) {
synchronized ( Coordinator.SHARED ) {
+ final String id = ConfigUtil.getPid(event);
if ( event.getType() == ConfigurationEvent.CM_DELETED ) {
final Coordinator.Operation op =
Coordinator.SHARED.get(event.getPid(), event.getFactoryPid(), true);
Review Comment:
It would be good to document somewhere by we pass the original PID and
Factory PID to the coordinator but the recalculated one elsewhere.
##########
src/main/java/org/apache/sling/installer/factories/configuration/impl/ConfigUtil.java:
##########
@@ -336,4 +337,20 @@ public static void removeRedundantProperties(final
Dictionary<String, Object> pr
}
}
}
+
Review Comment:
Please document why this is needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
