You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "zuston (via GitHub)" <gi...@apache.org> on 2023/12/01 06:16:34 UTC
[PR] [#1267] fix(client): fast fail without retry when oom occurs [incubator-uniffle]
zuston opened a new pull request, #1344:
URL: https://github.com/apache/incubator-uniffle/pull/1344
### What changes were proposed in this pull request?
fast fail without retry when oom occurs
### Why are the changes needed?
Fix: #1267
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing UTs
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1267] fix(client): fast fail without retry when oom occurs [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1344:
URL: https://github.com/apache/incubator-uniffle/pull/1344#discussion_r1411928017
##########
internal-client/src/main/java/org/apache/uniffle/client/impl/grpc/ShuffleServerGrpcClient.java:
##########
@@ -400,61 +400,65 @@ public RssSendShuffleDataResponse sendShuffleData(RssSendShuffleDataRequest requ
try {
RetryUtils.retry(
() -> {
- long requireId =
- requirePreAllocation(
- appId,
- shuffleId,
- partitionIds,
- allocateSize,
- request.getRetryMax() / maxRetryAttempts,
- request.getRetryIntervalMax());
- if (requireId == FAILED_REQUIRE_ID) {
- throw new RssException(
- String.format(
- "requirePreAllocation failed! size[%s], host[%s], port[%s]",
- allocateSize, host, port));
- }
- long start = System.currentTimeMillis();
- SendShuffleDataRequest rpcRequest =
- SendShuffleDataRequest.newBuilder()
- .setAppId(appId)
- .setShuffleId(stb.getKey())
- .setRequireBufferId(requireId)
- .addAllShuffleData(shuffleData)
- .setTimestamp(start)
- .build();
- SendShuffleDataResponse response = getBlockingStub().sendShuffleData(rpcRequest);
- if (LOG.isDebugEnabled()) {
- LOG.debug(
- "Do sendShuffleData to {}:{} rpc cost:"
- + (System.currentTimeMillis() - start)
- + " ms for "
- + allocateSize
- + " bytes with "
- + finalBlockNum
- + " blocks",
- host,
- port);
- }
- if (response.getStatus() != RssProtos.StatusCode.SUCCESS) {
- String msg =
- "Can't send shuffle data with "
- + finalBlockNum
- + " blocks to "
- + host
- + ":"
- + port
- + ", statusCode="
- + response.getStatus()
- + ", errorMsg:"
- + response.getRetMsg();
- if (response.getStatus() == RssProtos.StatusCode.NO_REGISTER) {
- throw new NotRetryException(msg);
- } else {
- throw new RssException(msg);
+ try {
+ long requireId =
+ requirePreAllocation(
+ appId,
+ shuffleId,
+ partitionIds,
+ allocateSize,
+ request.getRetryMax() / maxRetryAttempts,
+ request.getRetryIntervalMax());
+ if (requireId == FAILED_REQUIRE_ID) {
+ throw new RssException(
+ String.format(
+ "requirePreAllocation failed! size[%s], host[%s], port[%s]",
+ allocateSize, host, port));
+ }
+ long start = System.currentTimeMillis();
+ SendShuffleDataRequest rpcRequest =
+ SendShuffleDataRequest.newBuilder()
+ .setAppId(appId)
+ .setShuffleId(stb.getKey())
+ .setRequireBufferId(requireId)
+ .addAllShuffleData(shuffleData)
+ .setTimestamp(start)
+ .build();
+ SendShuffleDataResponse response = getBlockingStub().sendShuffleData(rpcRequest);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Do sendShuffleData to {}:{} rpc cost:"
+ + (System.currentTimeMillis() - start)
+ + " ms for "
+ + allocateSize
+ + " bytes with "
+ + finalBlockNum
+ + " blocks",
+ host,
+ port);
+ }
+ if (response.getStatus() != RssProtos.StatusCode.SUCCESS) {
+ String msg =
+ "Can't send shuffle data with "
+ + finalBlockNum
+ + " blocks to "
+ + host
+ + ":"
+ + port
+ + ", statusCode="
+ + response.getStatus()
+ + ", errorMsg:"
+ + response.getRetMsg();
+ if (response.getStatus() == RssProtos.StatusCode.NO_REGISTER) {
+ throw new NotRetryException(msg);
+ } else {
+ throw new RssException(msg);
+ }
}
+ return response;
+ } catch (OutOfMemoryError outOfMemoryError) {
Review Comment:
We shouldn't catch OutOfMemoryError atually.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1267] fix(client): fast fail without retry when oom occurs [incubator-uniffle]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1344:
URL: https://github.com/apache/incubator-uniffle/pull/1344#issuecomment-1835538720
## [Codecov](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1344?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `65 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`64dd847`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/64dd847177709d971a82b34afd4806649404cf75?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 53.25% compared to head [(`0f688ca`)](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1344?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 53.43%.
| [Files](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1344?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...client/impl/grpc/ShuffleServerGrpcNettyClient.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1344?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW50ZXJuYWwtY2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NsaWVudC9pbXBsL2dycGMvU2h1ZmZsZVNlcnZlckdycGNOZXR0eUNsaWVudC5qYXZh) | 0.00% | [32 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1344?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ffle/client/impl/grpc/ShuffleServerGrpcClient.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1344?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW50ZXJuYWwtY2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NsaWVudC9pbXBsL2dycGMvU2h1ZmZsZVNlcnZlckdycGNDbGllbnQuamF2YQ==) | 0.00% | [31 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1344?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...he/uniffle/common/exception/NotRetryException.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1344?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9leGNlcHRpb24vTm90UmV0cnlFeGNlcHRpb24uamF2YQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1344?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #1344 +/- ##
============================================
+ Coverage 53.25% 53.43% +0.17%
+ Complexity 2706 2176 -530
============================================
Files 418 341 -77
Lines 23866 15319 -8547
Branches 2036 1419 -617
============================================
- Hits 12709 8185 -4524
+ Misses 10374 6673 -3701
+ Partials 783 461 -322
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1344?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1267] fix(client): fast fail without retry when oom occurs [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1344:
URL: https://github.com/apache/incubator-uniffle/pull/1344#discussion_r1413291342
##########
internal-client/src/main/java/org/apache/uniffle/client/impl/grpc/ShuffleServerGrpcClient.java:
##########
@@ -400,61 +400,65 @@ public RssSendShuffleDataResponse sendShuffleData(RssSendShuffleDataRequest requ
try {
RetryUtils.retry(
() -> {
- long requireId =
- requirePreAllocation(
- appId,
- shuffleId,
- partitionIds,
- allocateSize,
- request.getRetryMax() / maxRetryAttempts,
- request.getRetryIntervalMax());
- if (requireId == FAILED_REQUIRE_ID) {
- throw new RssException(
- String.format(
- "requirePreAllocation failed! size[%s], host[%s], port[%s]",
- allocateSize, host, port));
- }
- long start = System.currentTimeMillis();
- SendShuffleDataRequest rpcRequest =
- SendShuffleDataRequest.newBuilder()
- .setAppId(appId)
- .setShuffleId(stb.getKey())
- .setRequireBufferId(requireId)
- .addAllShuffleData(shuffleData)
- .setTimestamp(start)
- .build();
- SendShuffleDataResponse response = getBlockingStub().sendShuffleData(rpcRequest);
- if (LOG.isDebugEnabled()) {
- LOG.debug(
- "Do sendShuffleData to {}:{} rpc cost:"
- + (System.currentTimeMillis() - start)
- + " ms for "
- + allocateSize
- + " bytes with "
- + finalBlockNum
- + " blocks",
- host,
- port);
- }
- if (response.getStatus() != RssProtos.StatusCode.SUCCESS) {
- String msg =
- "Can't send shuffle data with "
- + finalBlockNum
- + " blocks to "
- + host
- + ":"
- + port
- + ", statusCode="
- + response.getStatus()
- + ", errorMsg:"
- + response.getRetMsg();
- if (response.getStatus() == RssProtos.StatusCode.NO_REGISTER) {
- throw new NotRetryException(msg);
- } else {
- throw new RssException(msg);
+ try {
+ long requireId =
+ requirePreAllocation(
+ appId,
+ shuffleId,
+ partitionIds,
+ allocateSize,
+ request.getRetryMax() / maxRetryAttempts,
+ request.getRetryIntervalMax());
+ if (requireId == FAILED_REQUIRE_ID) {
+ throw new RssException(
+ String.format(
+ "requirePreAllocation failed! size[%s], host[%s], port[%s]",
+ allocateSize, host, port));
+ }
+ long start = System.currentTimeMillis();
+ SendShuffleDataRequest rpcRequest =
+ SendShuffleDataRequest.newBuilder()
+ .setAppId(appId)
+ .setShuffleId(stb.getKey())
+ .setRequireBufferId(requireId)
+ .addAllShuffleData(shuffleData)
+ .setTimestamp(start)
+ .build();
+ SendShuffleDataResponse response = getBlockingStub().sendShuffleData(rpcRequest);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Do sendShuffleData to {}:{} rpc cost:"
+ + (System.currentTimeMillis() - start)
+ + " ms for "
+ + allocateSize
+ + " bytes with "
+ + finalBlockNum
+ + " blocks",
+ host,
+ port);
+ }
+ if (response.getStatus() != RssProtos.StatusCode.SUCCESS) {
+ String msg =
+ "Can't send shuffle data with "
+ + finalBlockNum
+ + " blocks to "
+ + host
+ + ":"
+ + port
+ + ", statusCode="
+ + response.getStatus()
+ + ", errorMsg:"
+ + response.getRetMsg();
+ if (response.getStatus() == RssProtos.StatusCode.NO_REGISTER) {
+ throw new NotRetryException(msg);
+ } else {
+ throw new RssException(msg);
+ }
}
+ return response;
+ } catch (OutOfMemoryError outOfMemoryError) {
Review Comment:
I mean whether we wrap the exception of OutofMemoryError with NotRetryException. Maybe we throw an OutofMemoryError here and add a method` isThrowableNotRetry` in the RetryUtil, then we modify other related code.
```
private bool isThrowableNotRetry(Throwable throw) {
if (throw instanceof OutOfMemoryError) {
return true;
}
}
```
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1267] fix(client): fast fail without retry when oom occurs [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #1344:
URL: https://github.com/apache/incubator-uniffle/pull/1344#discussion_r1412793300
##########
internal-client/src/main/java/org/apache/uniffle/client/impl/grpc/ShuffleServerGrpcClient.java:
##########
@@ -400,61 +400,65 @@ public RssSendShuffleDataResponse sendShuffleData(RssSendShuffleDataRequest requ
try {
RetryUtils.retry(
() -> {
- long requireId =
- requirePreAllocation(
- appId,
- shuffleId,
- partitionIds,
- allocateSize,
- request.getRetryMax() / maxRetryAttempts,
- request.getRetryIntervalMax());
- if (requireId == FAILED_REQUIRE_ID) {
- throw new RssException(
- String.format(
- "requirePreAllocation failed! size[%s], host[%s], port[%s]",
- allocateSize, host, port));
- }
- long start = System.currentTimeMillis();
- SendShuffleDataRequest rpcRequest =
- SendShuffleDataRequest.newBuilder()
- .setAppId(appId)
- .setShuffleId(stb.getKey())
- .setRequireBufferId(requireId)
- .addAllShuffleData(shuffleData)
- .setTimestamp(start)
- .build();
- SendShuffleDataResponse response = getBlockingStub().sendShuffleData(rpcRequest);
- if (LOG.isDebugEnabled()) {
- LOG.debug(
- "Do sendShuffleData to {}:{} rpc cost:"
- + (System.currentTimeMillis() - start)
- + " ms for "
- + allocateSize
- + " bytes with "
- + finalBlockNum
- + " blocks",
- host,
- port);
- }
- if (response.getStatus() != RssProtos.StatusCode.SUCCESS) {
- String msg =
- "Can't send shuffle data with "
- + finalBlockNum
- + " blocks to "
- + host
- + ":"
- + port
- + ", statusCode="
- + response.getStatus()
- + ", errorMsg:"
- + response.getRetMsg();
- if (response.getStatus() == RssProtos.StatusCode.NO_REGISTER) {
- throw new NotRetryException(msg);
- } else {
- throw new RssException(msg);
+ try {
+ long requireId =
+ requirePreAllocation(
+ appId,
+ shuffleId,
+ partitionIds,
+ allocateSize,
+ request.getRetryMax() / maxRetryAttempts,
+ request.getRetryIntervalMax());
+ if (requireId == FAILED_REQUIRE_ID) {
+ throw new RssException(
+ String.format(
+ "requirePreAllocation failed! size[%s], host[%s], port[%s]",
+ allocateSize, host, port));
+ }
+ long start = System.currentTimeMillis();
+ SendShuffleDataRequest rpcRequest =
+ SendShuffleDataRequest.newBuilder()
+ .setAppId(appId)
+ .setShuffleId(stb.getKey())
+ .setRequireBufferId(requireId)
+ .addAllShuffleData(shuffleData)
+ .setTimestamp(start)
+ .build();
+ SendShuffleDataResponse response = getBlockingStub().sendShuffleData(rpcRequest);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Do sendShuffleData to {}:{} rpc cost:"
+ + (System.currentTimeMillis() - start)
+ + " ms for "
+ + allocateSize
+ + " bytes with "
+ + finalBlockNum
+ + " blocks",
+ host,
+ port);
+ }
+ if (response.getStatus() != RssProtos.StatusCode.SUCCESS) {
+ String msg =
+ "Can't send shuffle data with "
+ + finalBlockNum
+ + " blocks to "
+ + host
+ + ":"
+ + port
+ + ", statusCode="
+ + response.getStatus()
+ + ", errorMsg:"
+ + response.getRetMsg();
+ if (response.getStatus() == RssProtos.StatusCode.NO_REGISTER) {
+ throw new NotRetryException(msg);
+ } else {
+ throw new RssException(msg);
+ }
}
+ return response;
+ } catch (OutOfMemoryError outOfMemoryError) {
Review Comment:
Oh yes. But I hope the concrete error of `unable to create to native thread` could be caught, because oom killer won't catch this error.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1267] fix(client): fast fail without retry when oom occurs [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1344:
URL: https://github.com/apache/incubator-uniffle/pull/1344#discussion_r1411928017
##########
internal-client/src/main/java/org/apache/uniffle/client/impl/grpc/ShuffleServerGrpcClient.java:
##########
@@ -400,61 +400,65 @@ public RssSendShuffleDataResponse sendShuffleData(RssSendShuffleDataRequest requ
try {
RetryUtils.retry(
() -> {
- long requireId =
- requirePreAllocation(
- appId,
- shuffleId,
- partitionIds,
- allocateSize,
- request.getRetryMax() / maxRetryAttempts,
- request.getRetryIntervalMax());
- if (requireId == FAILED_REQUIRE_ID) {
- throw new RssException(
- String.format(
- "requirePreAllocation failed! size[%s], host[%s], port[%s]",
- allocateSize, host, port));
- }
- long start = System.currentTimeMillis();
- SendShuffleDataRequest rpcRequest =
- SendShuffleDataRequest.newBuilder()
- .setAppId(appId)
- .setShuffleId(stb.getKey())
- .setRequireBufferId(requireId)
- .addAllShuffleData(shuffleData)
- .setTimestamp(start)
- .build();
- SendShuffleDataResponse response = getBlockingStub().sendShuffleData(rpcRequest);
- if (LOG.isDebugEnabled()) {
- LOG.debug(
- "Do sendShuffleData to {}:{} rpc cost:"
- + (System.currentTimeMillis() - start)
- + " ms for "
- + allocateSize
- + " bytes with "
- + finalBlockNum
- + " blocks",
- host,
- port);
- }
- if (response.getStatus() != RssProtos.StatusCode.SUCCESS) {
- String msg =
- "Can't send shuffle data with "
- + finalBlockNum
- + " blocks to "
- + host
- + ":"
- + port
- + ", statusCode="
- + response.getStatus()
- + ", errorMsg:"
- + response.getRetMsg();
- if (response.getStatus() == RssProtos.StatusCode.NO_REGISTER) {
- throw new NotRetryException(msg);
- } else {
- throw new RssException(msg);
+ try {
+ long requireId =
+ requirePreAllocation(
+ appId,
+ shuffleId,
+ partitionIds,
+ allocateSize,
+ request.getRetryMax() / maxRetryAttempts,
+ request.getRetryIntervalMax());
+ if (requireId == FAILED_REQUIRE_ID) {
+ throw new RssException(
+ String.format(
+ "requirePreAllocation failed! size[%s], host[%s], port[%s]",
+ allocateSize, host, port));
+ }
+ long start = System.currentTimeMillis();
+ SendShuffleDataRequest rpcRequest =
+ SendShuffleDataRequest.newBuilder()
+ .setAppId(appId)
+ .setShuffleId(stb.getKey())
+ .setRequireBufferId(requireId)
+ .addAllShuffleData(shuffleData)
+ .setTimestamp(start)
+ .build();
+ SendShuffleDataResponse response = getBlockingStub().sendShuffleData(rpcRequest);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Do sendShuffleData to {}:{} rpc cost:"
+ + (System.currentTimeMillis() - start)
+ + " ms for "
+ + allocateSize
+ + " bytes with "
+ + finalBlockNum
+ + " blocks",
+ host,
+ port);
+ }
+ if (response.getStatus() != RssProtos.StatusCode.SUCCESS) {
+ String msg =
+ "Can't send shuffle data with "
+ + finalBlockNum
+ + " blocks to "
+ + host
+ + ":"
+ + port
+ + ", statusCode="
+ + response.getStatus()
+ + ", errorMsg:"
+ + response.getRetMsg();
+ if (response.getStatus() == RssProtos.StatusCode.NO_REGISTER) {
+ throw new NotRetryException(msg);
+ } else {
+ throw new RssException(msg);
+ }
}
+ return response;
+ } catch (OutOfMemoryError outOfMemoryError) {
Review Comment:
We shouldn't catch OutOfMemoryError actually. It's weird that we process OutofMemoryError.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1267] fix(client): fast fail without retry when oom occurs [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #1344:
URL: https://github.com/apache/incubator-uniffle/pull/1344#discussion_r1416608112
##########
internal-client/src/main/java/org/apache/uniffle/client/impl/grpc/ShuffleServerGrpcClient.java:
##########
@@ -400,61 +400,65 @@ public RssSendShuffleDataResponse sendShuffleData(RssSendShuffleDataRequest requ
try {
RetryUtils.retry(
() -> {
- long requireId =
- requirePreAllocation(
- appId,
- shuffleId,
- partitionIds,
- allocateSize,
- request.getRetryMax() / maxRetryAttempts,
- request.getRetryIntervalMax());
- if (requireId == FAILED_REQUIRE_ID) {
- throw new RssException(
- String.format(
- "requirePreAllocation failed! size[%s], host[%s], port[%s]",
- allocateSize, host, port));
- }
- long start = System.currentTimeMillis();
- SendShuffleDataRequest rpcRequest =
- SendShuffleDataRequest.newBuilder()
- .setAppId(appId)
- .setShuffleId(stb.getKey())
- .setRequireBufferId(requireId)
- .addAllShuffleData(shuffleData)
- .setTimestamp(start)
- .build();
- SendShuffleDataResponse response = getBlockingStub().sendShuffleData(rpcRequest);
- if (LOG.isDebugEnabled()) {
- LOG.debug(
- "Do sendShuffleData to {}:{} rpc cost:"
- + (System.currentTimeMillis() - start)
- + " ms for "
- + allocateSize
- + " bytes with "
- + finalBlockNum
- + " blocks",
- host,
- port);
- }
- if (response.getStatus() != RssProtos.StatusCode.SUCCESS) {
- String msg =
- "Can't send shuffle data with "
- + finalBlockNum
- + " blocks to "
- + host
- + ":"
- + port
- + ", statusCode="
- + response.getStatus()
- + ", errorMsg:"
- + response.getRetMsg();
- if (response.getStatus() == RssProtos.StatusCode.NO_REGISTER) {
- throw new NotRetryException(msg);
- } else {
- throw new RssException(msg);
+ try {
+ long requireId =
+ requirePreAllocation(
+ appId,
+ shuffleId,
+ partitionIds,
+ allocateSize,
+ request.getRetryMax() / maxRetryAttempts,
+ request.getRetryIntervalMax());
+ if (requireId == FAILED_REQUIRE_ID) {
+ throw new RssException(
+ String.format(
+ "requirePreAllocation failed! size[%s], host[%s], port[%s]",
+ allocateSize, host, port));
+ }
+ long start = System.currentTimeMillis();
+ SendShuffleDataRequest rpcRequest =
+ SendShuffleDataRequest.newBuilder()
+ .setAppId(appId)
+ .setShuffleId(stb.getKey())
+ .setRequireBufferId(requireId)
+ .addAllShuffleData(shuffleData)
+ .setTimestamp(start)
+ .build();
+ SendShuffleDataResponse response = getBlockingStub().sendShuffleData(rpcRequest);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Do sendShuffleData to {}:{} rpc cost:"
+ + (System.currentTimeMillis() - start)
+ + " ms for "
+ + allocateSize
+ + " bytes with "
+ + finalBlockNum
+ + " blocks",
+ host,
+ port);
+ }
+ if (response.getStatus() != RssProtos.StatusCode.SUCCESS) {
+ String msg =
+ "Can't send shuffle data with "
+ + finalBlockNum
+ + " blocks to "
+ + host
+ + ":"
+ + port
+ + ", statusCode="
+ + response.getStatus()
+ + ", errorMsg:"
+ + response.getRetMsg();
+ if (response.getStatus() == RssProtos.StatusCode.NO_REGISTER) {
+ throw new NotRetryException(msg);
+ } else {
+ throw new RssException(msg);
+ }
}
+ return response;
+ } catch (OutOfMemoryError outOfMemoryError) {
Review Comment:
Sounds good. Let me optimize this in the next days.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1267] fix(client): fast fail without retry when oom occurs [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston merged PR #1344:
URL: https://github.com/apache/incubator-uniffle/pull/1344
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org