HDDS-9370. Avoid copying ByteString in ByteStringCodec.

[szetszwo]

What changes were proposed in this pull request?

In ByteStringCodec, we may wrap the ByteString instead of copying it in the toCodecBuffer(..) method.

What is the link to the Apache JIRA

HDDS-9370

How was this patch tested?

Added new tests.

[sodonnel]

I think this change looks good, although there is one failing test which might be related.

Error:  Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.914 s <<< FAILURE! - in org.apache.hadoop.hdds.scm.ha.TestStatefulServiceStateManagerImpl
Error:  org.apache.hadoop.hdds.scm.ha.TestStatefulServiceStateManagerImpl.testSaveConfiguration  Time elapsed: 0.888 s  <<< ERROR!
org.apache.hadoop.hdds.scm.exceptions.SCMException: org.apache.ratis.protocol.exceptions.StateMachineException: java.lang.ClassCastException from Server peer@group-5A17DA7EDC91: java.lang.AssertionError cannot be cast to java.lang.Exception
	at org.apache.hadoop.hdds.scm.ha.SCMHAInvocationHandler.translateException(SCMHAInvocationHandler.java:165)
	at org.apache.hadoop.hdds.scm.ha.SCMHAInvocationHandler.invokeRatis(SCMHAInvocationHandler.java:115)
	at org.apache.hadoop.hdds.scm.ha.SCMHAInvocationHandler.invoke(SCMHAInvocationHandler.java:74)
	at com.sun.proxy.$Proxy16.saveConfiguration(Unknown Source)
	at org.apache.hadoop.hdds.scm.ha.TestStatefulServiceStateManagerImpl.testSaveConfiguration(TestStatefulServiceStateManagerImpl.java:81)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)

I have kicked it off again to see how it does on a retry.

[szetszwo]

@sodonnel , thanks for taking a look!

The test failure is related. It failed with an AssertError (but the SCMHAManagerStub code hided it) since rocksdb requires direct buffer. The wrapping work only for the non-direct buffer cases. Let me update the code.

[sodonnel]

The wrapping work only for the non-direct buffer cases. Let me update the code.

What does this do for us eliminating the copy? Most requests will end up in the RocksDB or come out of the RocksDB I think, so does that mean most of the time we will need to make an extra copy of the buffer, or most of the time we will not with this change?

[szetszwo]

What does this do for us eliminating the copy? Most requests will end up in the RocksDB or come out of the RocksDB I think, so does that mean most of the time we will need to make an extra copy of the buffer, or most of the time we will not with this change?

When the backing buffer of the ByteString is direct, this change can eliminate the copying. In TestStatefulServiceStateManagerImpl, we did not use direct buffer. However, the ByteString received by gRPC could use direct buffer.

[szetszwo]

The wrapping work only for the non-direct buffer cases. Let me update the code.

Oops, it should be -- The wrapping work for the cases that the backing buffer of the ByteString matches the allocation requirement.

[sodonnel]

Change LGTM. I am not sure how often the wrapping will help or not as I am not sure where we have a miss-match between direct vs non-direct etc, but hopefully this will make a good difference.

[szetszwo]

@sodonnel , thanks a lot for reviewing this!