[GitHub] [hadoop] cnauroth commented on a diff in pull request #4248: MAPREDUCE-7370. Parallelize MultipleOutputs#close call

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

cnauroth commented on code in PR #4248:
URL: https://github.com/apache/hadoop/pull/4248#discussion_r904363794


##########
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/lib/output/MultipleOutputs.java:
##########
@@ -570,8 +570,14 @@ public void setStatus(String status) {
    */
   @SuppressWarnings("unchecked")
   public void close() throws IOException, InterruptedException {
-    for (RecordWriter writer : recordWriters.values()) {
-      writer.close(context);
-    }
+    recordWriters.values().parallelStream().forEach(writer -> {

Review Comment:
   I'm concerned that this could have unintended side effects for callers, because it changes the error contract. Errors during `close()` that were formerly visible as a checked `IOException` or `InterruptedException` now become an unchecked `RuntimeException`. In the case of thread interruption, the interrupt now occurs on the background thread with no propagation of interrupted status back up to the coordinating thread.
   
   Unfortunately, `parallelStream()` with a lambda puts us down this path. It would be more code, but directly managing a `ThreadPoolExecutor` would give you the chance to preserve the contract by unwrapping checked exceptions from the `Future` and propagating.



-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org