You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/02/23 11:49:13 UTC
[GitHub] [ozone] guihecheng opened a new pull request #3128: HDDS-6368. EC: Fix broken future chain and cleanup unnecessary validation function.
guihecheng opened a new pull request #3128:
URL: https://github.com/apache/ozone/pull/3128
## What changes were proposed in this pull request?
Fix broken future chain and cleanup unnecessary validation function.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6368
## How was this patch tested?
Exisiting CI covers.
--
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] guihecheng commented on a change in pull request #3128: HDDS-6368. EC: Fix broken future chain and cleanup unnecessary validation function.
Posted by GitBox <gi...@apache.org>.
guihecheng commented on a change in pull request #3128:
URL: https://github.com/apache/ozone/pull/3128#discussion_r821361279
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/ECBlockOutputStreamEntry.java
##########
@@ -338,30 +334,16 @@ private boolean isFailed(
= null;
try {
containerCommandResponseProto = chunkWriteResponseFuture != null ?
- chunkWriteResponseFuture.get() :
- null;
+ chunkWriteResponseFuture.get() : null;
} catch (InterruptedException e) {
outputStream.setIoException(e);
Thread.currentThread().interrupt();
} catch (ExecutionException e) {
outputStream.setIoException(e);
}
- if ((outputStream != null && containerCommandResponseProto != null)
- && (outputStream.getIoException() != null || isStreamFailed(
- containerCommandResponseProto, outputStream))) {
- return true;
- }
- return false;
- }
-
- boolean isStreamFailed(
- ContainerProtos.ContainerCommandResponseProto responseProto,
- ECBlockOutputStream stream) {
- try {
- ContainerProtocolCalls.validateContainerResponse(responseProto);
- } catch (StorageContainerException sce) {
- stream.setIoException(sce);
+ if (containerCommandResponseProto != null
Review comment:
Oh, I'm fine with stricter checks for null here, it seems there's a complex problem that chen has hit.
Will update soon.
--
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] umamaheswararao merged pull request #3128: HDDS-6368. EC: Fix broken future chain and cleanup unnecessary validation function.
Posted by GitBox <gi...@apache.org>.
umamaheswararao merged pull request #3128:
URL: https://github.com/apache/ozone/pull/3128
--
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] guihecheng commented on a change in pull request #3128: HDDS-6368. EC: Fix broken future chain and cleanup unnecessary validation function.
Posted by GitBox <gi...@apache.org>.
guihecheng commented on a change in pull request #3128:
URL: https://github.com/apache/ozone/pull/3128#discussion_r820344295
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/ECBlockOutputStreamEntry.java
##########
@@ -338,30 +334,16 @@ private boolean isFailed(
= null;
try {
containerCommandResponseProto = chunkWriteResponseFuture != null ?
- chunkWriteResponseFuture.get() :
- null;
+ chunkWriteResponseFuture.get() : null;
} catch (InterruptedException e) {
outputStream.setIoException(e);
Thread.currentThread().interrupt();
} catch (ExecutionException e) {
outputStream.setIoException(e);
}
- if ((outputStream != null && containerCommandResponseProto != null)
- && (outputStream.getIoException() != null || isStreamFailed(
- containerCommandResponseProto, outputStream))) {
- return true;
- }
- return false;
- }
-
- boolean isStreamFailed(
- ContainerProtos.ContainerCommandResponseProto responseProto,
- ECBlockOutputStream stream) {
- try {
- ContainerProtocolCalls.validateContainerResponse(responseProto);
- } catch (StorageContainerException sce) {
- stream.setIoException(sce);
+ if (containerCommandResponseProto != null
Review comment:
Sure, a simple check for null and debug log is inserted above.
--
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] umamaheswararao commented on a change in pull request #3128: HDDS-6368. EC: Fix broken future chain and cleanup unnecessary validation function.
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #3128:
URL: https://github.com/apache/ozone/pull/3128#discussion_r819829343
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/ECBlockOutputStreamEntry.java
##########
@@ -338,30 +334,16 @@ private boolean isFailed(
= null;
try {
containerCommandResponseProto = chunkWriteResponseFuture != null ?
- chunkWriteResponseFuture.get() :
- null;
+ chunkWriteResponseFuture.get() : null;
} catch (InterruptedException e) {
outputStream.setIoException(e);
Thread.currentThread().interrupt();
} catch (ExecutionException e) {
outputStream.setIoException(e);
}
- if ((outputStream != null && containerCommandResponseProto != null)
- && (outputStream.getIoException() != null || isStreamFailed(
- containerCommandResponseProto, outputStream))) {
- return true;
- }
- return false;
- }
-
- boolean isStreamFailed(
- ContainerProtos.ContainerCommandResponseProto responseProto,
- ECBlockOutputStream stream) {
- try {
- ContainerProtocolCalls.validateContainerResponse(responseProto);
- } catch (StorageContainerException sce) {
- stream.setIoException(sce);
+ if (containerCommandResponseProto != null
Review comment:
While helping @cchenax to debug on the issue for root cause, should we go ahead committing this by adding more debug logs? If you agree, please add few debug logs as we are touching these files.
--
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] guihecheng commented on a change in pull request #3128: HDDS-6368. EC: Fix broken future chain and cleanup unnecessary validation function.
Posted by GitBox <gi...@apache.org>.
guihecheng commented on a change in pull request #3128:
URL: https://github.com/apache/ozone/pull/3128#discussion_r819250085
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/ECBlockOutputStreamEntry.java
##########
@@ -338,30 +334,16 @@ private boolean isFailed(
= null;
try {
containerCommandResponseProto = chunkWriteResponseFuture != null ?
- chunkWriteResponseFuture.get() :
- null;
+ chunkWriteResponseFuture.get() : null;
} catch (InterruptedException e) {
outputStream.setIoException(e);
Thread.currentThread().interrupt();
} catch (ExecutionException e) {
outputStream.setIoException(e);
}
- if ((outputStream != null && containerCommandResponseProto != null)
- && (outputStream.getIoException() != null || isStreamFailed(
- containerCommandResponseProto, outputStream))) {
- return true;
- }
- return false;
- }
-
- boolean isStreamFailed(
- ContainerProtos.ContainerCommandResponseProto responseProto,
- ECBlockOutputStream stream) {
- try {
- ContainerProtocolCalls.validateContainerResponse(responseProto);
- } catch (StorageContainerException sce) {
- stream.setIoException(sce);
+ if (containerCommandResponseProto != null
Review comment:
Oh, I'm curious to hear this problem and wonders why the future could be null, but the current path doesn't handle this 'future == null' case indeed.
I think if it really happens, we could show debug info here as you suggested and consider the stream as failed.
And let's help @cchenax dig into the issue he faced to get the root cause.
--
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] umamaheswararao commented on a change in pull request #3128: HDDS-6368. EC: Fix broken future chain and cleanup unnecessary validation function.
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #3128:
URL: https://github.com/apache/ozone/pull/3128#discussion_r820870123
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/ECBlockOutputStreamEntry.java
##########
@@ -338,30 +334,16 @@ private boolean isFailed(
= null;
try {
containerCommandResponseProto = chunkWriteResponseFuture != null ?
- chunkWriteResponseFuture.get() :
- null;
+ chunkWriteResponseFuture.get() : null;
} catch (InterruptedException e) {
outputStream.setIoException(e);
Thread.currentThread().interrupt();
} catch (ExecutionException e) {
outputStream.setIoException(e);
}
- if ((outputStream != null && containerCommandResponseProto != null)
- && (outputStream.getIoException() != null || isStreamFailed(
- containerCommandResponseProto, outputStream))) {
- return true;
- }
- return false;
- }
-
- boolean isStreamFailed(
- ContainerProtos.ContainerCommandResponseProto responseProto,
- ECBlockOutputStream stream) {
- try {
- ContainerProtocolCalls.validateContainerResponse(responseProto);
- } catch (StorageContainerException sce) {
- stream.setIoException(sce);
+ if (containerCommandResponseProto != null
Review comment:
I think here it seems there is a problem right?
It's not from this patch, but existing code has issue looks like.
Let's say when we catch ExecutionException above, we set the IOEception on stream, but containerCommandResponseProto will be null right. So, the below case will not be evaluated. and we may return false. get may return null if interrupted.Should we just log and ignore or we consider null also failed stream?
```
/**
* Returns raw result after waiting, or null if interruptible and
* interrupted.
*/
```
So, I think we should seperate the checks...Let's keep if(outputStream.getIoException() != null) check first and return true.
Then handle the containerCommandResponseProto == null case?
CC: @cchenax
--
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] umamaheswararao commented on a change in pull request #3128: HDDS-6368. EC: Fix broken future chain and cleanup unnecessary validation function.
Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #3128:
URL: https://github.com/apache/ozone/pull/3128#discussion_r819210873
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/ECBlockOutputStreamEntry.java
##########
@@ -338,30 +334,16 @@ private boolean isFailed(
= null;
try {
containerCommandResponseProto = chunkWriteResponseFuture != null ?
- chunkWriteResponseFuture.get() :
- null;
+ chunkWriteResponseFuture.get() : null;
} catch (InterruptedException e) {
outputStream.setIoException(e);
Thread.currentThread().interrupt();
} catch (ExecutionException e) {
outputStream.setIoException(e);
}
- if ((outputStream != null && containerCommandResponseProto != null)
- && (outputStream.getIoException() != null || isStreamFailed(
- containerCommandResponseProto, outputStream))) {
- return true;
- }
- return false;
- }
-
- boolean isStreamFailed(
- ContainerProtos.ContainerCommandResponseProto responseProto,
- ECBlockOutputStream stream) {
- try {
- ContainerProtocolCalls.validateContainerResponse(responseProto);
- } catch (StorageContainerException sce) {
- stream.setIoException(sce);
+ if (containerCommandResponseProto != null
Review comment:
I think there are cases when this response can be null ( I am not 100% sure, what case it will be null). In the case of null also, should we consider stream as failed?
@cchenax faced an issue and in his debug logs he was claiming one of future response was null. SO, we may need to rethink here, what if response future null?
And let's add some debug info in this response handling path?
--
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