HDDS-6400. EC: Refactor ECKeyOutputStream for better code reuse

[kaijchen]

What changes were proposed in this pull request?

Refactor ECKeyOutputStream to make the code cleaner.
Reuse code when writing the last stripe or rewriting failed stripes.

The changes includes:

1. handleDataWrite() has been changed to handleWrite(),
   it will check strip full and call encodeAndWriteParityCells().
2. checkAndWriteParityCells() has been changed to encodeAndWriteParityCells(),
   and it can be reused to encode partial stripe in close().
3. handleParityWrites() has seperated generateParityCells(),
   it can be reused in rewriteStripeToNewBlockGroup().
4. handleFailedStreams() has been changed to excludePipelineAndFailedDN(),
   it will exclude both failed pipeline and failed datanodes.
5. Reuse handleOutputStreamWrite() in close()
6. generateParityCells() will remove padding from dataBuffers after encoding,
   so we don't have to track stripe length in rewrite.
7. Moved isLastBlock check inside handleParityWrites(),
   so that we don't need to pass isClose parameter around in rewrite.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6400

How was this patch tested?

Existing CI

[umamaheswararao]

Thanks @kaijchen for working on this patch. Overall it looks good to me.
Could you please make it non draft patch?

[umamaheswararao]

LGTM, pending CI

[kaijchen]

Thanks @umamaheswararao for the review, actually I have a few more commits working on.

I have included them in this PR, please take another look at the 2 commits added.
The additional changes were:

1. Added ECChunkBuffer#encode(), which will not change the position of dataBuffers when encoding, so we don't have to track stripe length in rewrite.
2. Moved isLastBlock check inside handleParityWrites() so that we don't need to pass isClose parameter around in rewrite.

[umamaheswararao]

LGTM. Lets get this in.

[umamaheswararao]

Please resolve conflicts and pending CI one more time just to make sure. Thanks

[umamaheswararao]

Isn't this buffers we are creating out side of cache?

I think this may be a performance impact if there are many small files?
Pretty much for every file we will endup creating this buffer once outside of cache.

[kaijchen]

The duplicate() don't create extra buffer, it shares underlying content with the original ByteBuffer.

https://docs.oracle.com/javase/8/docs/api/java/nio/ByteBuffer.html#duplicate--

[kaijchen]

Or can we add dataBuffersForRead in ECChunkBuffers to avoid the object creation cost here?

[umamaheswararao]

Yeah you are right. Also this will happen sequentially so underlying buffer sharing is ok.
IIUC, How about we do this only when padding is necessary? Otherwise we may can use original buffers when all databuffers actually full right? So, min size of databuffers should tell use whether we will need padding or not right?
That way this extra object will be created only for last stripe right.

[kaijchen]

I don't think it's safe to use the original buffers here because encoder.encode() will modify the position of the input ByteBuffer.

[kaijchen]

Do you think adding a dataBufferForEncode like kaijchen@b15799b is OK?

[umamaheswararao]

That would too much usage of cache buffers class than needed.

IIUC, How about we do this only when padding is necessary? Otherwise we may can use original buffers when all databuffers actually full right?

Not sure you got what I was trying to say. Why original buffers will be modified when original buffers already full (all full with ecChunkSize)?
I was suggesting to create this duplicate buffers only when we really do padding. Let's say if all data buffers are full, then we will not do any padding. But still we create this duplicate right.

I don't think it's safe to use the original buffers here because encoder.encode() will modify the position of the input ByteBuffer.

Just for argument: Ideally you will modify original buffer here also. But we will have our own tracking pointers. Similar way, when you change the original buffer directly and we track that length, we are ok too.

[kaijchen]

OK, I got your point.

[umamaheswararao]

LGTM

[umamaheswararao]

Yeah generateParityCells looks little bit messy, but it should be ok as it got some checks needed there.

[kaijchen]

Thanks @umamaheswararao for the review.