You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "szetszwo (via GitHub)" <gi...@apache.org> on 2023/09/29 21:12:40 UTC
[GitHub] [ozone] szetszwo opened a new pull request, #5377: HDDS-9370. Avoid copying ByteString in ByteStringCodec.
szetszwo opened a new pull request, #5377:
URL: https://github.com/apache/ozone/pull/5377
## 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.
--
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
Re: [PR] HDDS-9370. Avoid copying ByteString in ByteStringCodec. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #5377:
URL: https://github.com/apache/ozone/pull/5377#issuecomment-1747279787
@sodonnel , thanks a lot for reviewing this!
--
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
Re: [PR] HDDS-9370. Avoid copying ByteString in ByteStringCodec. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #5377:
URL: https://github.com/apache/ozone/pull/5377#issuecomment-1745266010
> 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.
--
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
Re: [PR] HDDS-9370. Avoid copying ByteString in ByteStringCodec. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #5377:
URL: https://github.com/apache/ozone/pull/5377
--
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
Re: [PR] HDDS-9370. Avoid copying ByteString in ByteStringCodec. [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #5377:
URL: https://github.com/apache/ozone/pull/5377#issuecomment-1745214223
> 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?
--
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
Re: [PR] HDDS-9370. Avoid copying ByteString in ByteStringCodec. [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #5377:
URL: https://github.com/apache/ozone/pull/5377#issuecomment-1742810694
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.
--
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
Re: [PR] HDDS-9370. Avoid copying ByteString in ByteStringCodec. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #5377:
URL: https://github.com/apache/ozone/pull/5377#issuecomment-1743453465
@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.
--
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
Re: [PR] HDDS-9370. Avoid copying ByteString in ByteStringCodec. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #5377:
URL: https://github.com/apache/ozone/pull/5377#issuecomment-1745270908
> 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.
--
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