HDDS-5952. EC: ECBlockReconstructedStripeInputStream should read from blocks in parallel#2899
Conversation
sodonnel
force-pushed
the
ec-HDDS-5952-parallel-read
branch
from
450c1b5 to
bbba145
Compare
umamaheswararao
left a comment
umamaheswararao
left a comment
There was a problem hiding this comment.
Thanks @sodonnel for working on this. I have dropped few comments please check. Thanks
Overall approach looks good to me though.
| "block", getBlockID(), index + 1, ee.getCause()); | ||
| failedDataIndexes.add(index); | ||
| exceptionOccurred = true; | ||
| } catch (InterruptedException ie) { |
There was a problem hiding this comment.
May want to reset this Interrupted exception? SO that we will not suppress it?
| } catch (IOException e) { | ||
| ImmutablePair<Integer, Future<Void>> pair = pendingReads.poll(); | ||
| index = pair.getKey(); | ||
| // Should this future.get() have a timeout? At the end of the call chain |
There was a problem hiding this comment.
I agree. Actual timeouts should be handled by underlying end points.
| failedDataIndexes.add(index); | ||
| exceptionOccurred = true; | ||
| } catch (InterruptedException ie) { | ||
| LOG.error("Interrupted waiting for reads to complete", ie); |
There was a problem hiding this comment.
Looks like we are throwing the IOE on InterruptedException, but we catch the IOE in readStripe and retrying.
So, this interrupted exception would be masked. Probably, we should reset interrupted exception here and handle interruptedException in readStripe as well to throw out?
There was a problem hiding this comment.
Yea, I forgot the IOE would be retried by the higher level. I think we should let the interrupted exception propagate up from this method untouched and then handle it at the higher level.
| decoderInputBuffers = new ByteBuffer[getRepConfig().getRequiredNodes()]; | ||
|
|
||
| ThreadFactory threadFactory = new ThreadFactoryBuilder().setNameFormat( | ||
| "ec-reader-for-" + blockInfo.getBlockID() + "-%d").build(); |
There was a problem hiding this comment.
Can we just explicitly say -TID-%d, just not confuse with block indexes.
There was a problem hiding this comment.
I'm not sure what you mean here?
| exceptionOccurred = true; | ||
| } catch (InterruptedException ie) { | ||
| LOG.error("Interrupted waiting for reads to complete", ie); | ||
| throw new IOException("Interrupted waiting for reads to complete", ie); |
There was a problem hiding this comment.
We may need to shutdown the pool?
2 participants
What changes were proposed in this pull request?
To make reconstruction reads faster, ECBlockReconstructedStripeInputStream could read from the block streams in parallel rather than serially.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5952
How was this patch tested?
Existing tests