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:
us...@infra.apache.org
