[ 
https://issues.apache.org/jira/browse/HDFS-9494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15117849#comment-15117849
 ] 

Jing Zhao commented on HDFS-9494:
---------------------------------

Thanks for updating the patch, [~demongaorui]! The latest patch looks good to 
me. Some minors:
# We do not need to create a thread pool instance for each 
{{flushAllInternals}} call. It will be better if we can reuse.
# In {{handleCurrentStreamerFailure}}, let's still set {{currentPacket}} to 
null.
# should be "new ConcurrentHashMap<>()"
{code}
+    final Map<StripedDataStreamer, Exception> streamersExceptionMap = new
+        ConcurrentHashMap();
{code}
# We do not need to remove the entry from the temporary map here.
{code}
+      iterator.remove();
+    }
+    executor.shutdownNow();
{code}
# The exception thrown by {{waitForAckedSeqno}} can be caught as 
{{ExecutionException}} while calling {{Future#get}}. Thus I think we do not 
need to use a concurrent hashmap to catch the exceptions. Instead, we can 
create a map to track the mapping between streamers and corresponding futures. 
In this way we can remove both {{healthyStreamerCount}} and 
{{streamersExceptionMap}}. Please see {{DFSStripedInputStream#readStripe}} as 
an example.

bq. After checking the related codes, it seems that we haven't set a timeout 
for waitForAckedSeqno(). Maybe we could consider to set a timeout for it in 
another new Jira. 

It will be very hard to set this timeout value. Let's still depend on the 
read/write timeout set on the connection instead of adding a timeout for 
{{waitForAckedSeqno}}.

> Parallel optimization of DFSStripedOutputStream#flushAllInternals( )
> --------------------------------------------------------------------
>
>                 Key: HDFS-9494
>                 URL: https://issues.apache.org/jira/browse/HDFS-9494
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>            Reporter: GAO Rui
>            Assignee: GAO Rui
>            Priority: Minor
>         Attachments: HDFS-9494-origin-trunk.00.patch, 
> HDFS-9494-origin-trunk.01.patch, HDFS-9494-origin-trunk.02.patch, 
> HDFS-9494-origin-trunk.03.patch, HDFS-9494-origin-trunk.04.patch, 
> HDFS-9494-origin-trunk.05.patch
>
>
> Currently, in DFSStripedOutputStream#flushAllInternals( ), we trigger and 
> wait for flushInternal( ) in sequence. So the runtime flow is like:
> {code}
> Streamer0#flushInternal( )
> Streamer0#waitForAckedSeqno( )
> Streamer1#flushInternal( )
> Streamer1#waitForAckedSeqno( )
> …
> Streamer8#flushInternal( )
> Streamer8#waitForAckedSeqno( )
> {code}
> It could be better to trigger all the streamers to flushInternal( ) and
> wait for all of them to return from waitForAckedSeqno( ),  and then 
> flushAllInternals( ) returns.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to