fapifta commented on a change in pull request #2849:
URL: https://github.com/apache/ozone/pull/2849#discussion_r760628648
##########
File path:
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/BlockOutputStreamEntry.java
##########
@@ -113,8 +113,13 @@ public void write(int b) throws IOException {
@Override
public void write(byte[] b, int off, int len) throws IOException {
checkStream();
- getOutputStream().write(b, off, len);
- incCurrentPosition(len);
+ try {
+ getOutputStream().write(b, off, len);
+ }finally {
+ //Even though actual write failed we will move the position and when
Review comment:
Thank you for the explanation, now I think I see why it is enough to do
this change just here and still remain correct.
So:
- write(int) is not used, no problem with how it is implemented
- writeOnRetry is ok to not increase the current position on an error, as it
is not called in EC case so far, and during handleException before writeOnRetry
we reset the position with resetToAckedDataLength in non-EC case.
This basically means that if we would increase first, then write to stream,
the error handling code will possibly always reset (it depends on how we
implement multiple retries in EC case).
I think it took me quite a time to understand and for you to explain why
this is correct.
I see two ways out from this which makes any of our predecessors to
understand the code a bit easier:
1. increase the current position before writing to the underlying stream in
all three methods for clarity, and let error handling reset if there is an error
2. implement a logic to have acked data length persisted in the EC case, and
use the reset method there as well, instead of decrementing current position by
the last stripe size, so that we do not need to ensure that the increase
happens.
Based on my understanding, I think leaving it just this way would work, but
would not help to get what the code does later on when any of us has to come
back and debug or change something if we leave the code this way, and we have
methods that increment the current position differently.
--
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]
