You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2022/03/18 12:05:31 UTC
[GitHub] [ratis] szetszwo opened a new pull request #626: RATIS-1556. Support reference-counted buffer in StateMachine.DataChannel
szetszwo opened a new pull request #626:
URL: https://github.com/apache/ratis/pull/626
See https://issues.apache.org/jira/browse/RATIS-1556
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] lokeshj1703 commented on pull request #626: RATIS-1556. Support reference-counted buffer in StateMachine.DataChannel
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #626:
URL: https://github.com/apache/ratis/pull/626#issuecomment-1076036219
@szetszwo Thanks for the contribution! @captainzmc Thanks for the review! I have committed the PR to master branch.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] lokeshj1703 merged pull request #626: RATIS-1556. Support reference-counted buffer in StateMachine.DataChannel
Posted by GitBox <gi...@apache.org>.
lokeshj1703 merged pull request #626:
URL: https://github.com/apache/ratis/pull/626
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #626: RATIS-1556. Support reference-counted buffer in StateMachine.DataChannel
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #626:
URL: https://github.com/apache/ratis/pull/626#issuecomment-1076133091
@captainzmc , @lokeshj1703, thanks both 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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] lokeshj1703 commented on a change in pull request #626: RATIS-1556. Support reference-counted buffer in StateMachine.DataChannel
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #626:
URL: https://github.com/apache/ratis/pull/626#discussion_r832869423
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -294,8 +293,9 @@ static long writeTo(ByteBuf buf, WriteOption[] options, DataStream stream) {
final DataChannel channel = stream.getDataChannel();
long byteWritten = 0;
for (ByteBuffer buffer : buf.nioBuffers()) {
+ final ReferenceCountedObject<ByteBuffer> wrapped = ReferenceCountedObject.wrap(buffer, buf::retain, buf::release);
Review comment:
I see. I was thinking of a case where there are more than one buffers. For first buffer, buf retain and release is called by Ratis and reference count for buf would reach zero. According to ByteBuf#release documentation, if ref count goes to zero buf will be released. Based on this after one buffer is processed, buf will be released and subsequent buffer processing would fail?
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a change in pull request #626: RATIS-1556. Support reference-counted buffer in StateMachine.DataChannel
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #626:
URL: https://github.com/apache/ratis/pull/626#discussion_r832905021
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -294,8 +293,9 @@ static long writeTo(ByteBuf buf, WriteOption[] options, DataStream stream) {
final DataChannel channel = stream.getDataChannel();
long byteWritten = 0;
for (ByteBuffer buffer : buf.nioBuffers()) {
+ final ReferenceCountedObject<ByteBuffer> wrapped = ReferenceCountedObject.wrap(buffer, buf::retain, buf::release);
Review comment:
When there are more than one ByteBuffer objects backed by a ByteBuf, Ratis retains the ByteBuf first and then calls write(ByteBuffer) for each ByteBuffer objects. Once all the write calls have completed, it releases the ByteBuf.
In other words, whenever Ratis calling write(ByteBuffer), it makes sure that the backed ByteBuf has reference count >= 1. Logically, it is equivalent to do the following for each write(..) call.
1. Ratis retains.
2. Ratis calls write(..).
3. Ratis releases.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] lokeshj1703 commented on a change in pull request #626: RATIS-1556. Support reference-counted buffer in StateMachine.DataChannel
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #626:
URL: https://github.com/apache/ratis/pull/626#discussion_r832948207
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -294,8 +293,9 @@ static long writeTo(ByteBuf buf, WriteOption[] options, DataStream stream) {
final DataChannel channel = stream.getDataChannel();
long byteWritten = 0;
for (ByteBuffer buffer : buf.nioBuffers()) {
+ final ReferenceCountedObject<ByteBuffer> wrapped = ReferenceCountedObject.wrap(buffer, buf::retain, buf::release);
Review comment:
Got it. Thanks for the explanation @szetszwo!
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] lokeshj1703 commented on a change in pull request #626: RATIS-1556. Support reference-counted buffer in StateMachine.DataChannel
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #626:
URL: https://github.com/apache/ratis/pull/626#discussion_r832022724
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -294,8 +293,9 @@ static long writeTo(ByteBuf buf, WriteOption[] options, DataStream stream) {
final DataChannel channel = stream.getDataChannel();
long byteWritten = 0;
for (ByteBuffer buffer : buf.nioBuffers()) {
+ final ReferenceCountedObject<ByteBuffer> wrapped = ReferenceCountedObject.wrap(buffer, buf::retain, buf::release);
Review comment:
Is it possible that for a buffer, buf retain and release are both called while no other retain invocation is done? In such a case buf would get deallocated and can not be used by other buffers.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a change in pull request #626: RATIS-1556. Support reference-counted buffer in StateMachine.DataChannel
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #626:
URL: https://github.com/apache/ratis/pull/626#discussion_r832091581
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -294,8 +293,9 @@ static long writeTo(ByteBuf buf, WriteOption[] options, DataStream stream) {
final DataChannel channel = stream.getDataChannel();
long byteWritten = 0;
for (ByteBuffer buffer : buf.nioBuffers()) {
+ final ReferenceCountedObject<ByteBuffer> wrapped = ReferenceCountedObject.wrap(buffer, buf::retain, buf::release);
Review comment:
@lokeshj1703 , thanks for reviewing this.
Ratis would have buf.retain() called at least once before passing it to StateMachine via write(wrapped). After write(wrapped), Ratis will release it. StateMachine implementation may choose to further retain it. In such case, the buf will be released until both Ratis and the StateMachine have released it. If StateMachine don't retain it, the buf will be released by Ratis as usual.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org