ayushtkn commented on code in PR #5829:
URL: https://github.com/apache/hadoop/pull/5829#discussion_r1381508823
##########
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/StripeReader.java:
##########
@@ -233,41 +236,60 @@ private ByteBufferStrategy[]
getReadStrategies(StripingChunk chunk) {
private int readToBuffer(BlockReader blockReader,
DatanodeInfo currentNode, ByteBufferStrategy strategy,
- ExtendedBlock currentBlock) throws IOException {
+ LocatedBlock currentBlock, int chunkIndex) throws IOException {
final int targetLength = strategy.getTargetLength();
- int length = 0;
- try {
- while (length < targetLength) {
- int ret = strategy.readFromBlock(blockReader);
- if (ret < 0) {
- throw new IOException("Unexpected EOS from the reader");
+ int curAttempts = 0;
+ while (curAttempts < readDNMaxAttempts) {
+ int length = 0;
+ try {
+ while (length < targetLength) {
+ int ret = strategy.readFromBlock(blockReader);
+ if (ret < 0) {
+ throw new IOException("Unexpected EOS from the reader");
+ }
+ length += ret;
}
- length += ret;
+ return length;
+ } catch (ChecksumException ce) {
+ DFSClient.LOG.warn("Found Checksum error for "
+ + currentBlock + " from " + currentNode
+ + " at " + ce.getPos());
+ //Clear buffer to make next decode success
+ strategy.getReadBuffer().clear();
+ // we want to remember which block replicas we have tried
+ corruptedBlocks.addCorruptedBlock(currentBlock.getBlock(),
currentNode);
+ throw ce;
+ } catch (IOException e) {
+ //Clear buffer to make next decode success
+ strategy.getReadBuffer().clear();
+ if (curAttempts < readDNMaxAttempts - 1) {
+ curAttempts++;
+ if (readerInfos[chunkIndex].reader != null) {
+ readerInfos[chunkIndex].reader.close();
+ }
+ if (dfsStripedInputStream.createBlockReader(currentBlock,
+ alignedStripe.getOffsetInBlock(), targetBlocks,
+ readerInfos, chunkIndex, readTo)) {
+ blockReader = readerInfos[chunkIndex].reader;
+ String msg = "Reconnect to " + currentNode.getInfoAddr()
+ + " for block " + currentBlock.getBlock();
+ DFSClient.LOG.warn(msg);
+ continue;
+ }
+ DFSClient.LOG.warn("Exception while reading from "
+ + currentBlock + " of " + dfsStripedInputStream.getSrc() + " from "
+ + currentNode, e);
+ throw e;
}
- return length;
- } catch (ChecksumException ce) {
- DFSClient.LOG.warn("Found Checksum error for "
- + currentBlock + " from " + currentNode
- + " at " + ce.getPos());
- //Clear buffer to make next decode success
- strategy.getReadBuffer().clear();
- // we want to remember which block replicas we have tried
- corruptedBlocks.addCorruptedBlock(currentBlock, currentNode);
- throw ce;
- } catch (IOException e) {
- DFSClient.LOG.warn("Exception while reading from "
- + currentBlock + " of " + dfsStripedInputStream.getSrc() + " from "
- + currentNode, e);
- //Clear buffer to make next decode success
- strategy.getReadBuffer().clear();
- throw e;
}
}
+ return -1;
Review Comment:
I don't think we should return -1, there is logic which uses the return value
```
for (ByteBufferStrategy strategy : strategies) {
int bytesReead = readToBuffer(reader, datanode, strategy,
currentBlock);
ret += bytesReead;
}
```
We should throw exception or a valid value
##########
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/StripeReader.java:
##########
@@ -233,41 +236,60 @@ private ByteBufferStrategy[]
getReadStrategies(StripingChunk chunk) {
private int readToBuffer(BlockReader blockReader,
DatanodeInfo currentNode, ByteBufferStrategy strategy,
- ExtendedBlock currentBlock) throws IOException {
+ LocatedBlock currentBlock, int chunkIndex) throws IOException {
final int targetLength = strategy.getTargetLength();
- int length = 0;
- try {
- while (length < targetLength) {
- int ret = strategy.readFromBlock(blockReader);
- if (ret < 0) {
- throw new IOException("Unexpected EOS from the reader");
+ int curAttempts = 0;
+ while (curAttempts < readDNMaxAttempts) {
+ int length = 0;
+ try {
+ while (length < targetLength) {
+ int ret = strategy.readFromBlock(blockReader);
+ if (ret < 0) {
+ throw new IOException("Unexpected EOS from the reader");
+ }
+ length += ret;
}
- length += ret;
+ return length;
+ } catch (ChecksumException ce) {
+ DFSClient.LOG.warn("Found Checksum error for "
+ + currentBlock + " from " + currentNode
+ + " at " + ce.getPos());
+ //Clear buffer to make next decode success
+ strategy.getReadBuffer().clear();
+ // we want to remember which block replicas we have tried
+ corruptedBlocks.addCorruptedBlock(currentBlock.getBlock(),
currentNode);
+ throw ce;
+ } catch (IOException e) {
+ //Clear buffer to make next decode success
+ strategy.getReadBuffer().clear();
+ if (curAttempts < readDNMaxAttempts - 1) {
Review Comment:
doing this -1 doesn't look very fancy. Can we not initialize curAttempts
with 1 or increment curAttempts inside the while loop?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
