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 16:02:38 UTC
[GitHub] [ozone] szetszwo opened a new pull request, #5376: HDDS-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine.
szetszwo opened a new pull request, #5376:
URL: https://github.com/apache/ozone/pull/5376
## 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.
--
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-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #5376:
URL: https://github.com/apache/ozone/pull/5376#issuecomment-1747118845
> ... 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.
--
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-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #5376:
URL: https://github.com/apache/ozone/pull/5376#issuecomment-1743519164
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
```java
ByteString proto2 = wrap(proto3byteString.asReadOnlyByteBuffer());
```
Let me try it in a separated JIRA.
--
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
[GitHub] [ozone] kerneltime commented on a diff in pull request #5376: HDDS-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine.
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on code in PR #5376:
URL: https://github.com/apache/ozone/pull/5376#discussion_r1341576497
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -314,8 +313,7 @@ public TransactionContext preAppendTransaction(TransactionContext trx)
@Override
public CompletableFuture<Message> applyTransaction(TransactionContext trx) {
try {
- OMRequest request = OMRatisHelper.convertByteStringToOMRequest(
- trx.getStateMachineLogEntry().getLogData());
+ final OMRequest request = (OMRequest) trx.getStateMachineContext();
Review Comment:
Please add a line that this case depends on the deserialization done in preAppendTransaction.
--
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
[GitHub] [ozone] duongkame commented on a diff in pull request #5376: HDDS-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine.
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #5376:
URL: https://github.com/apache/ozone/pull/5376#discussion_r1341986291
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -267,8 +267,7 @@ public TransactionContext startTransaction(
@Override
public TransactionContext preAppendTransaction(TransactionContext trx)
throws IOException {
- OMRequest request = OMRatisHelper.convertByteStringToOMRequest(
- trx.getStateMachineLogEntry().getLogData());
+ final OMRequest request = (OMRequest) trx.getStateMachineContext();
OzoneManagerProtocolProtos.Type cmdType = request.getCmdType();
Review Comment:
All these validations should be on `startTransaction` as well. The creation of UGI is quite expensive as well.
--
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
[GitHub] [ozone] kerneltime commented on pull request #5376: HDDS-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine.
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on PR #5376:
URL: https://github.com/apache/ozone/pull/5376#issuecomment-1741175170
cc @tanvipenumudy
--
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-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #5376:
URL: https://github.com/apache/ozone/pull/5376
--
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
[GitHub] [ozone] szetszwo commented on a diff in pull request #5376: HDDS-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine.
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #5376:
URL: https://github.com/apache/ozone/pull/5376#discussion_r1341659910
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -314,8 +313,7 @@ public TransactionContext preAppendTransaction(TransactionContext trx)
@Override
public CompletableFuture<Message> applyTransaction(TransactionContext trx) {
try {
- OMRequest request = OMRatisHelper.convertByteStringToOMRequest(
- trx.getStateMachineLogEntry().getLogData());
+ final OMRequest request = (OMRequest) trx.getStateMachineContext();
Review Comment:
Updated. Thanks.
--
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-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine. [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #5376:
URL: https://github.com/apache/ozone/pull/5376#issuecomment-1742837270
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.
--
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
[GitHub] [ozone] kerneltime commented on a diff in pull request #5376: HDDS-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine.
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on code in PR #5376:
URL: https://github.com/apache/ozone/pull/5376#discussion_r1341576497
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -314,8 +313,7 @@ public TransactionContext preAppendTransaction(TransactionContext trx)
@Override
public CompletableFuture<Message> applyTransaction(TransactionContext trx) {
try {
- OMRequest request = OMRatisHelper.convertByteStringToOMRequest(
- trx.getStateMachineLogEntry().getLogData());
+ final OMRequest request = (OMRequest) trx.getStateMachineContext();
Review Comment:
Please add a line that this case depends on the deserialization done in `startTransaction`.
--
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-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #5376:
URL: https://github.com/apache/ozone/pull/5376#issuecomment-1743328707
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.
--
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-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #5376:
URL: https://github.com/apache/ozone/pull/5376#discussion_r1342990346
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -267,8 +267,7 @@ public TransactionContext startTransaction(
@Override
public TransactionContext preAppendTransaction(TransactionContext trx)
throws IOException {
- OMRequest request = OMRatisHelper.convertByteStringToOMRequest(
- trx.getStateMachineLogEntry().getLogData());
+ final OMRequest request = (OMRequest) trx.getStateMachineContext();
OzoneManagerProtocolProtos.Type cmdType = request.getCmdType();
Review Comment:
The UGI is only for upgrade `Prepare`. Let's don't optimize it here.
--
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
[GitHub] [ozone] kerneltime commented on a diff in pull request #5376: HDDS-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine.
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on code in PR #5376:
URL: https://github.com/apache/ozone/pull/5376#discussion_r1341576497
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -314,8 +313,7 @@ public TransactionContext preAppendTransaction(TransactionContext trx)
@Override
public CompletableFuture<Message> applyTransaction(TransactionContext trx) {
try {
- OMRequest request = OMRatisHelper.convertByteStringToOMRequest(
- trx.getStateMachineLogEntry().getLogData());
+ final OMRequest request = (OMRequest) trx.getStateMachineContext();
Review Comment:
Please add a line that this case depends on the deserialization done in `handleStartTransactionRequests`.
--
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-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine. [ozone]
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #5376:
URL: https://github.com/apache/ozone/pull/5376#discussion_r1344556949
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -314,7 +313,11 @@ public TransactionContext preAppendTransaction(TransactionContext trx)
@Override
public CompletableFuture<Message> applyTransaction(TransactionContext trx) {
try {
- OMRequest request = OMRatisHelper.convertByteStringToOMRequest(
+ // For the Leader, the OMRequest is set in trx in startTransaction.
+ // For Followers, the OMRequest hast to be converted from the log entry.
+ final Object context = trx.getStateMachineContext();
+ final OMRequest request = context != null ? (OMRequest) context
Review Comment:
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.
--
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-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #5376:
URL: https://github.com/apache/ozone/pull/5376#discussion_r1344685578
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -314,7 +313,11 @@ public TransactionContext preAppendTransaction(TransactionContext trx)
@Override
public CompletableFuture<Message> applyTransaction(TransactionContext trx) {
try {
- OMRequest request = OMRatisHelper.convertByteStringToOMRequest(
+ // For the Leader, the OMRequest is set in trx in startTransaction.
+ // For Followers, the OMRequest hast to be converted from the log entry.
+ final Object context = trx.getStateMachineContext();
+ final OMRequest request = context != null ? (OMRequest) context
Review Comment:
> ... `applyTransaction` is done after the majority commit, ...
That is correct.
--
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-9369. Avoid deserializing OMRequest multiple times in OzoneManagerStateMachine. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #5376:
URL: https://github.com/apache/ozone/pull/5376#issuecomment-1747112756
@kerneltime , @duongkame , @sodonnel , @jojochuang , thanks a lot for the review and ideas!
--
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