[GitHub] [systemml] Baunsgaard commented on a change in pull request #946: Privacy Runtime Extended

[GitBox <gi...@apache.org>]

Baunsgaard commented on a change in pull request #946:
URL: https://github.com/apache/systemml/pull/946#discussion_r441375740



##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedResponse.java
##########
@@ -94,10 +94,11 @@ public void throwExceptionFromResponse() throws Exception {
 	 * If the map is empty, it means that no privacy constraints were found.
 	 * @param checkedConstraints map of checked constraints from the PrivacyMonitor
 	 */
-	public void setCheckedConstraints(ConcurrentHashMap<PrivacyLevel,LongAdder> checkedConstraints){
+	public void setCheckedConstraints(HashMap<PrivacyLevel,LongAdder> checkedConstraints){
 		if ( checkedConstraints != null && !checkedConstraints.isEmpty() ){
-			this.checkedConstraints = checkedConstraints;
-		}
+			this.checkedConstraints = new HashMap<PrivacyLevel, LongAdder>();
+			this.checkedConstraints.putAll(checkedConstraints);
+		}	

Review comment:
       question.
   Are you leveraging that it is not concurrent? or that the likelihood of two elements having the same hash is low?

##########
File path: src/main/java/org/apache/sysds/runtime/privacy/PrivacyMonitor.java
##########
@@ -30,23 +30,33 @@
 import org.apache.sysds.runtime.privacy.PrivacyConstraint.PrivacyLevel;
 
 public class PrivacyMonitor 
-{
-	//TODO maybe maintain a log of checked constaints for transfers
-	// in order to provide 'privacy explanations' similar to our stats 
-	private static ConcurrentHashMap<PrivacyLevel,LongAdder> checkedConstraints = new ConcurrentHashMap<PrivacyLevel,LongAdder>();
+{ 
+	private static HashMap<PrivacyLevel,LongAdder> checkedConstraints;
+
+	static {
+		checkedConstraints = new HashMap<PrivacyLevel,LongAdder>();
+		for ( PrivacyLevel level : PrivacyLevel.values() ){
+			checkedConstraints.put(level, new LongAdder());
+		}
+	}
+
 	private static boolean checkPrivacy = false;
 
-	public static ConcurrentHashMap<PrivacyLevel,LongAdder> getCheckedConstraints(){
+	public static HashMap<PrivacyLevel,LongAdder> getCheckedConstraints(){
 		return checkedConstraints;
 	}
 
 	private static void incrementCheckedConstraints(PrivacyLevel privacyLevel){
-		if ( checkPrivacy )
-			checkedConstraints.computeIfAbsent(privacyLevel, k -> new LongAdder()).increment();
+		if ( checkPrivacy ){
+			if ( privacyLevel == null )
+				throw new NullPointerException("Cannot increment checked constraints log: Privacy level is null.");
+			checkedConstraints.get(privacyLevel).increment();
+		}
+			
 	}
 
 	public static void clearCheckedConstraints(){
-		checkedConstraints.clear();
+		checkedConstraints.replaceAll((k,v)->new LongAdder());

Review comment:
       once you add more functions, this will become an duplication from lines ~40.
   Maybe make a builderPattern and use that in both cases?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org