You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "AngersZhuuuu (via GitHub)" <gi...@apache.org> on 2024/04/07 09:28:16 UTC
[PR] [CELEBORN-1376] Push data failed should always release request body [celeborn]
AngersZhuuuu opened a new pull request, #2449:
URL: https://github.com/apache/celeborn/pull/2449
### What changes were proposed in this pull request?
Worker netty not release
<img width="1729" alt="截屏2024-04-07 17 26 40" src="https://github.com/apache/celeborn/assets/46485123/5774f735-570b-448e-ab94-4c78661717f5">
Many push failed
<img width="767" alt="截屏2024-04-07 17 27 46" src="https://github.com/apache/celeborn/assets/46485123/41866bd0-d634-4dbf-8518-b474c8d1faad">
### Why are the changes needed?
### Does this PR introduce _any_ user-facing change?
### How was this patch tested?
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1376] Push data failed should always release request body [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #2449:
URL: https://github.com/apache/celeborn/pull/2449#discussion_r1555187698
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClient.java:
##########
@@ -225,14 +226,25 @@ public ChannelFuture pushData(
PushRequestInfo info = new PushRequestInfo(dueTime, callback);
handler.addPushRequest(requestId, info);
pushData.requestId = requestId;
- PushChannelListener listener = new PushChannelListener(requestId, rpcSendoutCallback);
+ PushChannelListener listener =
+ new PushChannelListener(requestId, rpcSendoutCallback, rpcFailureCallback);
ChannelFuture channelFuture = channel.writeAndFlush(pushData).addListener(listener);
info.setChannelFuture(channelFuture);
return channelFuture;
}
public ChannelFuture pushMergedData(
PushMergedData pushMergedData, long pushDataTimeout, RpcResponseCallback callback) {
+ return pushMergedData(
+ pushMergedData, pushDataTimeout, callback, null, () -> pushMergedData.body().release());
Review Comment:
I think this could introduce IllegalReferenceCountException. Maybe here needs more 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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1376] Push data failed should always release request body [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2449:
URL: https://github.com/apache/celeborn/pull/2449#discussion_r1555175589
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClient.java:
##########
@@ -415,29 +437,29 @@ protected void handleFailure(String errorMsg, Throwable cause) {
private class PushChannelListener extends StdChannelListener {
final long pushRequestId;
- Runnable rpcSendOutCallback;
+ Runnable rpcFailureCallback;
PushChannelListener(long pushRequestId) {
this(pushRequestId, null);
}
- PushChannelListener(long pushRequestId, Runnable rpcSendOutCallback) {
+ PushChannelListener(long pushRequestId, Runnable rpcFailureCallback) {
super("PUSH " + pushRequestId);
this.pushRequestId = pushRequestId;
- this.rpcSendOutCallback = rpcSendOutCallback;
+ this.rpcFailureCallback = rpcFailureCallback;
}
@Override
public void operationComplete(Future<? super Void> future) throws Exception {
super.operationComplete(future);
- if (rpcSendOutCallback != null) {
Review Comment:
> How about current?
Make sense.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1376] Push data failed should always release request body [celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #2449:
URL: https://github.com/apache/celeborn/pull/2449#discussion_r1554998050
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClient.java:
##########
@@ -415,29 +437,29 @@ protected void handleFailure(String errorMsg, Throwable cause) {
private class PushChannelListener extends StdChannelListener {
final long pushRequestId;
- Runnable rpcSendOutCallback;
+ Runnable rpcFailureCallback;
PushChannelListener(long pushRequestId) {
this(pushRequestId, null);
}
- PushChannelListener(long pushRequestId, Runnable rpcSendOutCallback) {
+ PushChannelListener(long pushRequestId, Runnable rpcFailureCallback) {
super("PUSH " + pushRequestId);
this.pushRequestId = pushRequestId;
- this.rpcSendOutCallback = rpcSendOutCallback;
+ this.rpcFailureCallback = rpcFailureCallback;
}
@Override
public void operationComplete(Future<? super Void> future) throws Exception {
super.operationComplete(future);
- if (rpcSendOutCallback != null) {
Review Comment:
This changes behavior, is it OK? cc @RexXiong @FMX
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1376] Push data failed should always release request body [celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2449:
URL: https://github.com/apache/celeborn/pull/2449#discussion_r1558978671
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClient.java:
##########
@@ -216,6 +216,23 @@ public ChannelFuture pushData(
long pushDataTimeout,
RpcResponseCallback callback,
Runnable rpcSendoutCallback) {
+ Runnable rpcFailureCallback =
+ () -> {
+ try {
+ pushData.body().release();
+ } catch (Throwable e) {
+ logger.error("Error release buffer for PUSH_DATA request " + pushData.requestId, e);
Review Comment:
Done
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClient.java:
##########
@@ -225,14 +242,33 @@ public ChannelFuture pushData(
PushRequestInfo info = new PushRequestInfo(dueTime, callback);
handler.addPushRequest(requestId, info);
pushData.requestId = requestId;
- PushChannelListener listener = new PushChannelListener(requestId, rpcSendoutCallback);
+ PushChannelListener listener =
+ new PushChannelListener(requestId, rpcSendoutCallback, rpcFailureCallback);
ChannelFuture channelFuture = channel.writeAndFlush(pushData).addListener(listener);
info.setChannelFuture(channelFuture);
return channelFuture;
}
public ChannelFuture pushMergedData(
PushMergedData pushMergedData, long pushDataTimeout, RpcResponseCallback callback) {
+ Runnable rpcFailureCallback =
+ () -> {
+ try {
+ pushMergedData.body().release();
+ } catch (Throwable e) {
+ logger.error(
+ "Error release buffer for PUSH_MERGED_DATA request " + pushMergedData.requestId, e);
Review Comment:
Done
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1376] Push data failed should always release request body [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2449:
URL: https://github.com/apache/celeborn/pull/2449#discussion_r1555159963
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClient.java:
##########
@@ -415,29 +437,29 @@ protected void handleFailure(String errorMsg, Throwable cause) {
private class PushChannelListener extends StdChannelListener {
final long pushRequestId;
- Runnable rpcSendOutCallback;
+ Runnable rpcFailureCallback;
PushChannelListener(long pushRequestId) {
this(pushRequestId, null);
}
- PushChannelListener(long pushRequestId, Runnable rpcSendOutCallback) {
+ PushChannelListener(long pushRequestId, Runnable rpcFailureCallback) {
super("PUSH " + pushRequestId);
this.pushRequestId = pushRequestId;
- this.rpcSendOutCallback = rpcSendOutCallback;
+ this.rpcFailureCallback = rpcFailureCallback;
}
@Override
public void operationComplete(Future<? super Void> future) throws Exception {
super.operationComplete(future);
- if (rpcSendOutCallback != null) {
Review Comment:
IIRC this will affect buffer release from flink, IMO we can retain the current setup and remove the rpcFailureCallback call from handleFailure, since operationComplete will be invoked regardless of whether the channel fails
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1376] Push data failed should always release request body [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on PR #2449:
URL: https://github.com/apache/celeborn/pull/2449#issuecomment-2047304786
Thanks. Merged into main(v0.5.0) and branch-0.4(v0.4.1)
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1376] Push data failed should always release request body [celeborn]
Posted by "littlexyw (via GitHub)" <gi...@apache.org>.
littlexyw commented on code in PR #2449:
URL: https://github.com/apache/celeborn/pull/2449#discussion_r1584470037
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClient.java:
##########
Review Comment:
@FMX
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1376] Push data failed should always release request body [celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2449:
URL: https://github.com/apache/celeborn/pull/2449#discussion_r1555167919
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClient.java:
##########
@@ -415,29 +437,29 @@ protected void handleFailure(String errorMsg, Throwable cause) {
private class PushChannelListener extends StdChannelListener {
final long pushRequestId;
- Runnable rpcSendOutCallback;
+ Runnable rpcFailureCallback;
PushChannelListener(long pushRequestId) {
this(pushRequestId, null);
}
- PushChannelListener(long pushRequestId, Runnable rpcSendOutCallback) {
+ PushChannelListener(long pushRequestId, Runnable rpcFailureCallback) {
super("PUSH " + pushRequestId);
this.pushRequestId = pushRequestId;
- this.rpcSendOutCallback = rpcSendOutCallback;
+ this.rpcFailureCallback = rpcFailureCallback;
}
@Override
public void operationComplete(Future<? super Void> future) throws Exception {
super.operationComplete(future);
- if (rpcSendOutCallback != null) {
Review Comment:
How about current?
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1376] Push data failed should always release request body [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX closed pull request #2449: [CELEBORN-1376] Push data failed should always release request body
URL: https://github.com/apache/celeborn/pull/2449
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1376] Push data failed should always release request body [celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #2449:
URL: https://github.com/apache/celeborn/pull/2449#discussion_r1584691458
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClient.java:
##########
Review Comment:
That's because in this case error/Exception occurs before data is sent to pipeline. `future.isSuccess()` returns false in `StdChannelListener#operationComplete`
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1376] Push data failed should always release request body [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2449:
URL: https://github.com/apache/celeborn/pull/2449#discussion_r1883285432
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClient.java:
##########
Review Comment:
> "When an outbound (a.k.a. downstream) message reaches at the beginning of the pipeline, Netty will release it after writing it out." So why is pushdata not released when rpc failure occurs?
You are right, we have already reverted this pr.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1376] Push data failed should always release request body [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #2449:
URL: https://github.com/apache/celeborn/pull/2449#discussion_r1558909688
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClient.java:
##########
@@ -216,6 +216,23 @@ public ChannelFuture pushData(
long pushDataTimeout,
RpcResponseCallback callback,
Runnable rpcSendoutCallback) {
+ Runnable rpcFailureCallback =
+ () -> {
+ try {
+ pushData.body().release();
+ } catch (Throwable e) {
+ logger.error("Error release buffer for PUSH_DATA request " + pushData.requestId, e);
Review Comment:
Here is a nit. I think you should use {} instead of + when logging.
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClient.java:
##########
@@ -225,14 +242,33 @@ public ChannelFuture pushData(
PushRequestInfo info = new PushRequestInfo(dueTime, callback);
handler.addPushRequest(requestId, info);
pushData.requestId = requestId;
- PushChannelListener listener = new PushChannelListener(requestId, rpcSendoutCallback);
+ PushChannelListener listener =
+ new PushChannelListener(requestId, rpcSendoutCallback, rpcFailureCallback);
ChannelFuture channelFuture = channel.writeAndFlush(pushData).addListener(listener);
info.setChannelFuture(channelFuture);
return channelFuture;
}
public ChannelFuture pushMergedData(
PushMergedData pushMergedData, long pushDataTimeout, RpcResponseCallback callback) {
+ Runnable rpcFailureCallback =
+ () -> {
+ try {
+ pushMergedData.body().release();
+ } catch (Throwable e) {
+ logger.error(
+ "Error release buffer for PUSH_MERGED_DATA request " + pushMergedData.requestId, e);
Review Comment:
Ditto.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1376] Push data failed should always release request body [celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2449:
URL: https://github.com/apache/celeborn/pull/2449#discussion_r1555289732
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClient.java:
##########
@@ -225,14 +226,25 @@ public ChannelFuture pushData(
PushRequestInfo info = new PushRequestInfo(dueTime, callback);
handler.addPushRequest(requestId, info);
pushData.requestId = requestId;
- PushChannelListener listener = new PushChannelListener(requestId, rpcSendoutCallback);
+ PushChannelListener listener =
+ new PushChannelListener(requestId, rpcSendoutCallback, rpcFailureCallback);
ChannelFuture channelFuture = channel.writeAndFlush(pushData).addListener(listener);
info.setChannelFuture(channelFuture);
return channelFuture;
}
public ChannelFuture pushMergedData(
PushMergedData pushMergedData, long pushDataTimeout, RpcResponseCallback callback) {
+ return pushMergedData(
+ pushMergedData, pushDataTimeout, callback, null, () -> pushMergedData.body().release());
Review Comment:
How about current?
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1376] Push data failed should always release request body [celeborn]
Posted by "littlexyw (via GitHub)" <gi...@apache.org>.
littlexyw commented on code in PR #2449:
URL: https://github.com/apache/celeborn/pull/2449#discussion_r1584334926
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClient.java:
##########
Review Comment:
"When an outbound (a.k.a. downstream) message reaches at the beginning of the pipeline, Netty will release it after writing it out." So why is pushdata not released when rpc failure occurs?
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org