[GitHub] [ozone] sodonnel commented on pull request #5364: HDDS-7228. Avoid excessive memory consumpiton in checksum calculation

["sodonnel (via GitHub)" <gi...@apache.org>]

sodonnel commented on PR #5364:
URL: https://github.com/apache/ozone/pull/5364#issuecomment-1735849785

   I recall an earlier conversation we had on Slack, where you pointed out that the reason the current code does not work well, is because it does this:
   
   ```
     public void update(ByteBuffer buffer) {
       if (buffer.hasArray()) {
         checksum.update(buffer.array(), buffer.position() + buffer.arrayOffset(),
             buffer.remaining());
       } else {
         byte[] b = new byte[buffer.remaining()];
         buffer.get(b);
         checksum.update(b, 0, b.length);
       }
     }
   ```
   
   `buffer.hasArray()` always returns false if the buffer is readOnly, and it our case, the buffer is created by a call to `writeChunk.getData().asReadOnlyByteBufferList()` which is `ByteString.asReadOnlyByteBufferList()`.
   
   Looking inside `KeyValueHandler.java` where the chunk writes happen on the Datanode and where the checksum validation happens, the code looks like:
   
   ```
         WriteChunkRequestProto writeChunk = request.getWriteChunk();
         BlockID blockID = BlockID.getFromProtobuf(writeChunk.getBlockID());
         ContainerProtos.ChunkInfo chunkInfoProto = writeChunk.getChunkData();
         ChunkInfo chunkInfo = ChunkInfo.getFromProtoBuf(chunkInfoProto);
         Preconditions.checkNotNull(chunkInfo);
   
         ChunkBuffer data = null;
         if (dispatcherContext == null) {
           dispatcherContext = new DispatcherContext.Builder().build();
         }
         WriteChunkStage stage = dispatcherContext.getStage();
         if (stage == WriteChunkStage.WRITE_DATA ||
             stage == WriteChunkStage.COMBINED) {
           data =
               ChunkBuffer.wrap(writeChunk.getData().asReadOnlyByteBufferList());
           validateChunkChecksumData(data, chunkInfo);
   ```
   
   We can see that pull the data from the proto as a byteString and then create the asReadOnlyByteBufferList, which basically copies the proto into a ByteBuffer.
   
   Then inside `validateChunkChecksumData`, the code looks like:
   
   ```
     private void validateChunkChecksumData(ChunkBuffer data, ChunkInfo info)
         throws StorageContainerException {
       if (validateChunkChecksumData) {
         try {
           Checksum.verifyChecksum(data.toByteString(byteBufferToByteString),
               info.getChecksumData(), 0);
         } catch (OzoneChecksumException ex) {
           throw ChunkUtils.wrapInStorageContainerException(ex);
         }
       }
     }
   ```
   
   And then:
   
   ```
     public static boolean verifyChecksum(ByteString byteString,
         ChecksumData checksumData, int startIndex) throws OzoneChecksumException {
       final ByteBuffer buffer = byteString.asReadOnlyByteBuffer();
       return verifyChecksum(buffer, checksumData, startIndex);
   }
   ```
   
   So we start with a byteString in the protobuf. Then we convert that into a ByteBuffer, back to a byteString and back to a ByteBuffer and then in this PR a final copy from ByteBuffer to ByteBuffer!
   
   Were you seeing excessive memory usage on the DN side when writing chunks and with chunk.data.validation.check=true (the default is false, which is frankly crazy and will result in dataloss some day).


-- 
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