[GitHub] [systemds] sebwrede commented on a change in pull request #1054: [MINOR] Remove Exceptions From FederatedResponse

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

sebwrede commented on a change in pull request #1054:
URL: https://github.com/apache/systemds/pull/1054#discussion_r487786099



##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedWorkerHandler.java
##########
@@ -155,7 +155,7 @@ private FederatedResponse executeCommand(FederatedRequest request) {
 		catch (Exception ex) {
 			return new FederatedResponse(ResponseType.ERROR,
 				new FederatedWorkerHandlerException("Exception of type "
-				+ ex.getClass() + " thrown when processing request", ex));
+				+ ex.getClass() + " thrown when processing request"));

Review comment:
       We need to send the DMLPrivacyExceptions since we want the coordinator to understand why a FederatedRequest was rejected by the worker. In that way, it would later be possible for the coordinator to mitigate the problem by sending a different request which does not violate the privacy constraints.
   The DMLPrivacyException class is only used when checking privacy constraints, hence we control which information is put in the instances of the class. This means that we can safely return the exception to the coordinator without worrying about exposing private data. The same idea applies to the FederatedWorkerHandlerException: we create instances of this class and we are therefore able to control the information in the exception instances. 
   This is different from the other exceptions because they could be thrown from anywhere, be of any exception type, and contain any kind of information. This means that it is safer to catch such exceptions and return our own exception type with a message we control without including the original exception in the federated response. 
   We could add the catch clause for DMLPrivacyExceptions in other place, but generally I try to throw exceptions early and catch them late. If I catch DMLPrivacyExceptions in other places it should be to rethrow them and only catch them in executeCommand so that we only create the FederatedResponse once instead of having that logic in several different places. Also, the DMLPrivacyException is only thrown in some places which means that it should never be caught in for instance the "execClear" method. However, we can still add it so that the exception is caught if the code is changed to throw the privacy exception in the future. 
   I have changed the code now so that it has the catch clause for DMLPrivacyException and FederatedWorkerHandlerException before the Exception catch clauses. 




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