You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "sodonnel (via GitHub)" <gi...@apache.org> on 2023/02/27 12:54:03 UTC
[GitHub] [ozone] sodonnel opened a new pull request, #4320: HDDS-8037. Improve logging in EC Reconstruction putBlock precondition check
sodonnel opened a new pull request, #4320:
URL: https://github.com/apache/ozone/pull/4320
## What changes were proposed in this pull request?
Add more details to the logging around the put block precondition check to help with debugging [HDDS-7836](https://issues.apache.org/jira/browse/HDDS-7836).
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8037
## How was this patch tested?
No tests impacted - logging only change
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a diff in pull request #4320: HDDS-8037. Improve logging in EC Reconstruction putBlock precondition check
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #4320:
URL: https://github.com/apache/ozone/pull/4320#discussion_r1118969714
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ec/reconstruction/ECReconstructionCoordinator.java:
##########
@@ -324,9 +324,11 @@ private void checkFailures(ECBlockOutputStream targetBlockStream,
// Even after retries if it failed, we should declare the
// reconstruction as failed.
// For now, let's throw the exception.
- throw new IOException(
- "Chunk write failed at the new target node: " + targetBlockStream
- .getDatanodeDetails() + ". Aborting the reconstruction process.");
+ String message = "Chunk write failed at the new target node: " +
Review Comment:
OK, I remove the extra log.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime merged pull request #4320: HDDS-8037. Improve logging in EC Reconstruction putBlock precondition check
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime merged PR #4320:
URL: https://github.com/apache/ozone/pull/4320
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on pull request #4320: HDDS-8037. Improve logging in EC Reconstruction putBlock precondition check
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on PR #4320:
URL: https://github.com/apache/ozone/pull/4320#issuecomment-1446703683
On a side note, Can we add more logging on where this additional chunk is being added once the issue is identified? This can be a part of [HDDS-7836](https://issues.apache.org/jira/browse/HDDS-7836)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #4320: HDDS-8037. Improve logging in EC Reconstruction putBlock precondition check
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4320:
URL: https://github.com/apache/ozone/pull/4320#discussion_r1118959061
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ec/reconstruction/ECReconstructionCoordinator.java:
##########
@@ -324,9 +324,11 @@ private void checkFailures(ECBlockOutputStream targetBlockStream,
// Even after retries if it failed, we should declare the
// reconstruction as failed.
// For now, let's throw the exception.
- throw new IOException(
- "Chunk write failed at the new target node: " + targetBlockStream
- .getDatanodeDetails() + ". Aborting the reconstruction process.");
+ String message = "Chunk write failed at the new target node: " +
Review Comment:
Nit: This would create redundant logs. As far as I understand this is already being logged in the class ECReconstructionCoordinatorTask.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ayushtkn commented on a diff in pull request #4320: HDDS-8037. Improve logging in EC Reconstruction putBlock precondition check
Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on code in PR #4320:
URL: https://github.com/apache/ozone/pull/4320#discussion_r1118738326
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ECBlockOutputStream.java:
##########
@@ -128,8 +128,13 @@ ContainerCommandResponseProto> executePutBlock(boolean close,
List<ChunkInfo> currentChunks = getContainerBlockData().getChunksList();
List<ChunkInfo> checksumBlockDataChunks = checksumBlockData.getChunks();
- Preconditions.checkArgument(
- currentChunks.size() == checksumBlockDataChunks.size());
+ if (currentChunks.size() != checksumBlockDataChunks.size()) {
+ throw new IllegalArgumentException("The chunk list has " +
+ currentChunks.size() +
+ " entries, but the checksum chunks has " +
+ checksumBlockDataChunks.size() +
+ " entries. They should be equal in size.");
+ }
Review Comment:
Why not just add the message in Preconditions.checkArgument itself,
```
Preconditions.checkArgument(
currentChunks.size() == checksumBlockDataChunks.size(),
"The chunk list has " + currentChunks.size()
+ " entries, but the checksum chunks has "
+ checksumBlockDataChunks.size()
+ " entries. They should be equal in size.");
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a diff in pull request #4320: HDDS-8037. Improve logging in EC Reconstruction putBlock precondition check
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #4320:
URL: https://github.com/apache/ozone/pull/4320#discussion_r1118749693
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/ECBlockOutputStream.java:
##########
@@ -128,8 +128,13 @@ ContainerCommandResponseProto> executePutBlock(boolean close,
List<ChunkInfo> currentChunks = getContainerBlockData().getChunksList();
List<ChunkInfo> checksumBlockDataChunks = checksumBlockData.getChunks();
- Preconditions.checkArgument(
- currentChunks.size() == checksumBlockDataChunks.size());
+ if (currentChunks.size() != checksumBlockDataChunks.size()) {
+ throw new IllegalArgumentException("The chunk list has " +
+ currentChunks.size() +
+ " entries, but the checksum chunks has " +
+ checksumBlockDataChunks.size() +
+ " entries. They should be equal in size.");
+ }
Review Comment:
I didn't realize it took an extra argument. I will change it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #4320: HDDS-8037. Improve logging in EC Reconstruction putBlock precondition check
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4320:
URL: https://github.com/apache/ozone/pull/4320#discussion_r1118959061
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ec/reconstruction/ECReconstructionCoordinator.java:
##########
@@ -324,9 +324,11 @@ private void checkFailures(ECBlockOutputStream targetBlockStream,
// Even after retries if it failed, we should declare the
// reconstruction as failed.
// For now, let's throw the exception.
- throw new IOException(
- "Chunk write failed at the new target node: " + targetBlockStream
- .getDatanodeDetails() + ". Aborting the reconstruction process.");
+ String message = "Chunk write failed at the new target node: " +
Review Comment:
This would create redundant logs. As far as I understand this is already being logged in the class ECReconstructionCoordinatorTask.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org