HDDS-10904. [hsync] Enable PutBlock piggybacking and incremental chunk list by default

[jojochuang]

What changes were proposed in this pull request?

HDDS-10904. [hsync] Enable PutBlock piggybacking and incremental chunk list by default

Please describe your PR in detail:

• Enable WriteChunk/PutBlock piggybacking (ozone.client.stream.putblock.piggybacking) by default (HDDS-9130)
• Enable incremental chunk list feature (ozone.client.incremental.chunk.list) by default (HDDS-8047)
• Disable both features if the DataNode protocol version is older (below COMBINED_PUTBLOCK_WRITECHUNK_RPC/2)
• Deal with empty block case for EC in the DataNode blockDataTable.
• Deal with Ratis client corner case where before a block becomes at least a full chunk long, it may not be found by various utilities (delete block, container inspector,

What is the link to the Apache JIRA

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

How was this patch tested?

Existing unit tests, acceptance compatibility test, new unit tests for empty chunk case.

[chungen0126]

Why should we get the blockData from blockDataTable again after getting a range of blockData from blockDataTable.

[jojochuang]

If incremental chunk list feature is enabled, the last chunk of a block may exist in the lastChunkInfoTable. So getBlockByID() is used here to get the complete block data.

There could be opportunities to optimize later.

[chungen0126]

Can we put merging the last chunk into another function? For example:

  @Override
  public BlockData getBlockByID(BlockID blockID,
      String blockKey) throws IOException {
    // check block data table
    BlockData blockData = getBlockDataTable().get(blockKey);
    return getCompleteBlockData(blockData, blockID, blockKey);
  }

  @Override
  public BlockData getCompleteBlockData(
      BlockData blockData, BlockID blockID, String blockKey) throws IOException {
    BlockData lastChunk = null;
    if (blockData == null || isPartialChunkList(blockData)) {
      // check last chunk table
      lastChunk = getLastChunkInfoTable().get(blockKey);
    }

    if (blockData == null || blockData.getChunks().isEmpty()) {
      if (lastChunk == null) {
        throw new StorageContainerException(
            NO_SUCH_BLOCK_ERR_MSG + " BlockID : " + blockID, NO_SUCH_BLOCK);
      } else {
        if (LOG.isDebugEnabled()) {
          LOG.debug("blockData=(null), lastChunk={}", lastChunk.getChunks());
        }
        return lastChunk;
      }
    } else {
      if (lastChunk != null) {
        reconcilePartialChunks(lastChunk, blockData);
      } else {
        if (LOG.isDebugEnabled()) {
          LOG.debug("blockData={}, lastChunk=(null)", blockData.getChunks());
        }
      }
    }

    return blockData;
  }

Then, we can use it here, and we can avoid getting blockData from the table twice.

for (Table.KeyValue<String, BlockData> entry : range) {
          result.add(db.getStore().getCompleteBlockData(entry.getValue(), null, entry.getKey()));
        }
        return result;

[jojochuang]

Thanks! That makes sense to me.

[ChenSammi]

I would suggest to remove this fallback branch since it's not sure about the DN's version.

[jojochuang]

The thing is if it is not set, it is automatically set to the client's current datanode version, which is 2. So the client wouldn't be able to tell the correct DataNode version.

[jojochuang]

Simplified the PR. Turns out there's no need to handle the EC empty block corner case.

[ChenSammi]

The last patch LGTM. Thanks @jojochuang .

[jojochuang]

Merged. Thanks @ChenSammi and @chungen0126 !