HDDS-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine.

[szetszwo]

What changes were proposed in this pull request?

In OzoneManagerStateMachine, the same log data is deserialized to the same OMRequest multiple times. It can be avoided by setting stateMachineContext.

What is the link to the Apache JIRA

HDDS-9369

How was this patch tested?

By existing tests.

[kerneltime]

cc @tanvipenumudy

[kerneltime]

Please add a line that this case depends on the deserialization done in startTransaction.

[szetszwo]

Updated. Thanks.

[kerneltime]

Looks good, pending CI and minor nit.

[duongkame]

Thanks for the change @szetszwo. LGTM, but I think we should move the validations in preAppendTransaction to startTransaction as well.

[duongkame]

All these validations should be on startTransaction as well. The creation of UGI is quite expensive as well.

[szetszwo]

The UGI is only for upgrade Prepare. Let's don't optimize it here.

[sodonnel]

In looking at this, I checked:

  public static OMRequest convertByteStringToOMRequest(ByteString byteString)
      throws InvalidProtocolBufferException {
    byte[] bytes = byteString.toByteArray();
    return OMRequest.parseFrom(bytes);
  }

And I noticed it takes the passed byteString and converts it to Bytes and then passes the bytes to OMRequest.parseFrom(). OMRequest.parseFrom also accepts a ByteString as a parameter, avoiding this copy - however, the parameter to this method is org.apache.ratis.thirdparty.com.google.protobuf.ByteString; and the parameter in the method is com.google.protobuf.ByteString.

I don't know much about how this Ratis stuff is put together, but it seems that we are paying the price for an extra copy on every message received due to "different" but likely the same classes being used behind the scenes.

[jojochuang]

We had the similar problem in HBase where the protobuf it used collides with the one shaded by Hadoop 3.3.
IIRC we solved it using reflection. Not the best solution IMO.

HBASE-23833

[szetszwo]

Proto 3 support wrapping a ByteBuffer to a ByteString but proto 2 does not. We may be able to copy the proto 3 wrapper code to proto 2. Then, the conversion can be done by

ByteString proto2 = wrap(proto3byteString.asReadOnlyByteBuffer());

Let me try it in a separated JIRA.

[duongkame]

LGTM, thanks for the patch @szetszwo .

[duongkame]

So, the followers need to deserialize. That's OK if applyTransaction on the followers does not slow the commit phase down. Is that the case?
I think applyTransaction is done after the majority commit, just wanna double check.

[szetszwo]

... applyTransaction is done after the majority commit, ...

That is correct.

[szetszwo]

@kerneltime , @duongkame , @sodonnel , @jojochuang , thanks a lot for the review and ideas!

[szetszwo]

... We may be able to copy the proto 3 wrapper code to proto 2. ...

Tried it a little bit but not succeeded. Many ByteString subclasses in proto 3 were missing in proto 2 and the wrap method needed them.