You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "maobaolong (via GitHub)" <gi...@apache.org> on 2024/07/18 13:01:11 UTC
[PR] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
maobaolong opened a new pull request, #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931
<!--
1. Title: [#<issue>] <type>(<scope>): <subject>
Examples:
- "[#123] feat(operator): support xxx"
- "[#233] fix: check null before access result in xxx"
- "[MINOR] refactor: fix typo in variable name"
- "[MINOR] docs: fix typo in README"
- "[#255] test: fix flaky test NameOfTheTest"
Reference: https://www.conventionalcommits.org/en/v1.0.0/
2. Contributor guidelines:
https://github.com/apache/incubator-uniffle/blob/master/CONTRIBUTING.md
3. If the PR is unfinished, please mark this PR as draft.
-->
### What changes were proposed in this pull request?
(Please outline the changes and how this PR fixes the issue.)
### Why are the changes needed?
(Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, describe the bug.)
Fix: # (issue)
### Does this PR introduce _any_ user-facing change?
(Please list the user-facing changes introduced by your change, including
1. Change in user-facing APIs.
2. Addition or removal of property keys.)
No.
### How was this patch tested?
(Please test your changes, and provide instructions on how to test it:
1. If you add a feature or fix a bug, add a test to cover your changes.
2. If you fix a flaky test, repeat it for many times to prove it works.)
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#issuecomment-2236517215
Encountered flaky test.
```
Error: Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 20.238 s <<< FAILURE! - in org.apache.uniffle.test.RSSStageDynamicServerReWriteTest
Error: testRSSStageResubmit Time elapsed: 16.782 s <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <1000> but was: <969>
at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:145)
at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:527)
at org.apache.uniffle.test.SparkIntegrationTestBase.verifyTestResult(SparkIntegrationTestBase.java:149)
at org.apache.uniffle.test.SparkIntegrationTestBase.run(SparkIntegrationTestBase.java:64)
at org.apache.uniffle.test.RSSStageDynamicServerReWriteTest.testRSSStageResubmit(RSSStageDynamicServerReWriteTest.java:119)
```
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on code in PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#discussion_r1723095518
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithLinkedList.java:
##########
@@ -110,8 +110,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocks.forEach(spb -> spb.getData().release());
+ public long release() {
+ Throwable lastException = null;
+ int failedReleaseSize = 0;
+ long releaseSize = 0;
+ for (ShufflePartitionedBlock spb : blocks) {
+ try {
+ spb.getData().release();
+ releaseSize += spb.getSize();
+ } catch (Throwable t) {
+ lastException = t;
+ failedReleaseSize += spb.getSize();
+ }
+ }
+ if (lastException != null) {
+ LOG.warn(
+ "Failed to release shuffle blocks with size {}. Maybe it released by others",
Review Comment:
done
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithSkipList.java:
##########
@@ -111,8 +111,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocksMap.values().forEach(spb -> spb.getData().release());
+ public long release() {
+ Throwable lastException = null;
+ int failedReleaseSize = 0;
+ long releaseSize = 0;
+ for (ShufflePartitionedBlock spb : blocksMap.values()) {
+ try {
+ spb.getData().release();
+ releaseSize += spb.getSize();
+ } catch (Throwable t) {
+ lastException = t;
+ failedReleaseSize += spb.getSize();
+ }
+ }
+ if (lastException != null) {
+ LOG.warn(
+ "Failed to release shuffle blocks with size (). Maybe it released by others",
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@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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#discussion_r1704987271
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithSkipList.java:
##########
@@ -111,8 +111,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocksMap.values().forEach(spb -> spb.getData().release());
+ public long release() {
Review Comment:
> `toFlushEvent` method do not release buf, the buf released by a thread start by `ProcessEventThread` thread
I know this.
> it guarded by `app read lock`
I don't get this point.
And is it possible that the following case？
1. Thread-1( toFlushEvent): List<ShufflePartitionedBlock> spBlocks = new LinkedList<>(blocks);
2. Thread-2(release): blocks.forEach(spb -> spb.getData().release()); (The first release)
3. Thread-1( toFlushEvent): blocks.clear();
4. Thread-3 (DefaultFlushEventHandler#dispatchEvent) (The second release)
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "lwllvyb (via GitHub)" <gi...@apache.org>.
lwllvyb commented on code in PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#discussion_r1705472070
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithSkipList.java:
##########
@@ -111,8 +111,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocksMap.values().forEach(spb -> spb.getData().release());
+ public long release() {
Review Comment:
> Thread-1( toFlushEvent): List spBlocks = new LinkedList<>(blocks);
**toFlushEvent** will be guarded by app read lock.
> Thread-2(release): blocks.forEach(spb -> spb.getData().release()); (The first release)
The **release** function be called in **removeResources** or **removeResourcesByShuffleIds**, guarded by app write lock.
So this situation cannot happen. I don't know if my decription is corret.
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on code in PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#discussion_r1723189775
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferManager.java:
##########
@@ -715,9 +715,18 @@ public void removeBufferByShuffleId(String appId, Collection<Integer> shuffleIds
Collection<ShuffleBuffer> buffers = bufferRangeMap.asMapOfRanges().values();
if (buffers != null) {
for (ShuffleBuffer buffer : buffers) {
- buffer.release();
+ // the actual release size by this thread
+ long releaseSize = buffer.release();
Review Comment:
releasedSize
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithSkipList.java:
##########
@@ -111,8 +111,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocksMap.values().forEach(spb -> spb.getData().release());
+ public long release() {
+ Throwable lastException = null;
+ int failedReleaseSize = 0;
+ long releaseSize = 0;
Review Comment:
ditto
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithSkipList.java:
##########
@@ -111,8 +111,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocksMap.values().forEach(spb -> spb.getData().release());
+ public long release() {
+ Throwable lastException = null;
+ int failedReleaseSize = 0;
Review Comment:
ditto
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferManager.java:
##########
@@ -715,9 +715,18 @@ public void removeBufferByShuffleId(String appId, Collection<Integer> shuffleIds
Collection<ShuffleBuffer> buffers = bufferRangeMap.asMapOfRanges().values();
if (buffers != null) {
for (ShuffleBuffer buffer : buffers) {
- buffer.release();
+ // the actual release size by this thread
Review Comment:
released
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithLinkedList.java:
##########
@@ -110,8 +110,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocks.forEach(spb -> spb.getData().release());
+ public long release() {
+ Throwable lastException = null;
+ int failedReleaseSize = 0;
+ long releaseSize = 0;
Review Comment:
releasedSize
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithLinkedList.java:
##########
@@ -110,8 +110,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocks.forEach(spb -> spb.getData().release());
+ public long release() {
+ Throwable lastException = null;
+ int failedReleaseSize = 0;
Review Comment:
failedToReleaseSize
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on code in PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#discussion_r1682820539
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithLinkedList.java:
##########
@@ -110,8 +110,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocks.forEach(spb -> spb.getData().release());
+ public long release() {
+ Throwable lastException = null;
+ int failedReleaseSize = 0;
+ long releaseSize = 0;
+ for (ShufflePartitionedBlock spb : blocks) {
+ try {
+ spb.getData().release();
+ releaseSize += spb.getSize();
+ } catch (Throwable t) {
+ lastException = t;
Review Comment:
Yeah, but it can be verbose, may be we get last exception is enough?
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#discussion_r1682809743
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithLinkedList.java:
##########
@@ -110,8 +110,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocks.forEach(spb -> spb.getData().release());
+ public long release() {
+ Throwable lastException = null;
+ int failedReleaseSize = 0;
+ long releaseSize = 0;
+ for (ShufflePartitionedBlock spb : blocks) {
+ try {
+ spb.getData().release();
+ releaseSize += spb.getSize();
+ } catch (Throwable t) {
+ lastException = t;
Review Comment:
Should we log 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@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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma merged PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#issuecomment-2300022835
@rickyma The typo issues resolved, PTAL.
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#issuecomment-2270331039
@xianjingfeng Thanks for your information! As there are some reason can cause this redundant release, so I think this PR is important. And we need to try to fix all double release issue in the long term.
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on code in PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#discussion_r1704872794
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithSkipList.java:
##########
@@ -111,8 +111,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocksMap.values().forEach(spb -> spb.getData().release());
+ public long release() {
Review Comment:
`toFlushEvent` method do not release buf, the buf released by a thread start by `ProcessEventThread` thread, it guarded by `app read lock`, but not `ShuffleBufferWithSkipList`.
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#issuecomment-2298539861
@rickyma Thanks for your suggestion, addressed it, PTAL.
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#issuecomment-2270265330
> This may not be the only reason. I also encountered this problem with version 0.9.0.
My mistake. Not the same, but similar.
```
[2024-06-19 21:10:56.200] [FallBackFlushEventThreadPool-3] [ERROR] ShuffleDataFlushEvent.doCleanup - Errors doing cleanup callback. event: ShuffleDataFlushEvent: eventId=25200, a
ppId=application_1710412492003_36511244_1718802346131, shuffleId=0, startPartition=183, endPartition=183, retryTimes=11, underStorage=HadoopStorage, isPended=true, ownedByHugePar
tition=false
io.netty.util.IllegalReferenceCountException: refCnt: 0, decrement: 1
at io.netty.util.internal.ReferenceCountUpdater.toLiveRealRefCnt(ReferenceCountUpdater.java:83)
at io.netty.util.internal.ReferenceCountUpdater.release(ReferenceCountUpdater.java:148)
at io.netty.buffer.AbstractReferenceCountedByteBuf.release(AbstractReferenceCountedByteBuf.java:101)
at org.apache.uniffle.server.buffer.SkipListShuffleBuffer.lambda$null$0(SkipListShuffleBuffer.java:83)
at java.lang.Iterable.forEach(Iterable.java:75)
at org.apache.uniffle.server.buffer.SkipListShuffleBuffer.lambda$toFlushEvent$1(SkipListShuffleBuffer.java:83)
at org.apache.uniffle.server.ShuffleDataFlushEvent.doCleanup(ShuffleDataFlushEvent.java:143)
at org.apache.uniffle.server.DefaultFlushEventHandler.handleEventAndUpdateMetrics(DefaultFlushEventHandler.java:131)
at org.apache.uniffle.server.DefaultFlushEventHandler.lambda$dispatchEvent$0(DefaultFlushEventHandler.java:219)
at java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1626)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:745)
```
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#discussion_r1704857744
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithSkipList.java:
##########
@@ -111,8 +111,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocksMap.values().forEach(spb -> spb.getData().release());
+ public long release() {
Review Comment:
I think we need to add `synchronized` here. Calling the `toFlushEvent` method and the `release` method at the same time may result in duplicate release.
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#discussion_r1682804039
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithSkipList.java:
##########
@@ -111,8 +111,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocksMap.values().forEach(spb -> spb.getData().release());
+ public long release() {
+ Throwable lastException = null;
+ int failedReleaseSize = 0;
+ long releaseSize = 0;
+ for (ShufflePartitionedBlock spb : blocksMap.values()) {
+ try {
+ spb.getData().release();
+ releaseSize += spb.getSize();
+ } catch (Throwable t) {
+ lastException = t;
+ failedReleaseSize += spb.getSize();
+ }
+ }
+ if (lastException != null) {
+ LOG.warn(
+ "Failed to release shuffle blocks with size (). Maybe it released by others",
Review Comment:
Why will it be released by others?
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#issuecomment-2236519257
## Test Results
2 716 files +31 2 716 suites +31 5h 36m 41s :stopwatch: + 12m 50s
968 tests ± 0 966 :white_check_mark: + 1 1 :zzz: ±0 1 :x: - 1
12 128 runs +60 12 112 :white_check_mark: +61 15 :zzz: ±0 1 :x: - 1
For more details on these failures, see [this check](https://github.com/apache/incubator-uniffle/runs/27617267509).
Results for commit 200f77a6. ± Comparison against base commit b5cac305.
[test-results]:data:application/gzip;base64,H4sIALkXmWYC/1XMTQ6DIBCG4asY1l3MTFPQXsYASkLqT4OwMr17ByOWLr+H4d2F89O4iWdDCuWtEVvysd5DCjr6dckCBMjE7zFfdLItq9+StQfJH738m+n60Tvtpz8YQ1gDC7CEtOQmElJ7zhJlQ7rsrD4KVNVj11G7zrOPPAQBOKW0BImkWmUMuG5Q5m5QWzLUSdJOm9GJzxfWInwsEgEAAA==
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#discussion_r1683706031
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithLinkedList.java:
##########
@@ -110,8 +110,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocks.forEach(spb -> spb.getData().release());
+ public long release() {
+ Throwable lastException = null;
+ int failedReleaseSize = 0;
+ long releaseSize = 0;
+ for (ShufflePartitionedBlock spb : blocks) {
+ try {
+ spb.getData().release();
+ releaseSize += spb.getSize();
+ } catch (Throwable t) {
+ lastException = t;
Review Comment:
Think twice. It's ok.
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#discussion_r1682804039
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithSkipList.java:
##########
@@ -111,8 +111,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocksMap.values().forEach(spb -> spb.getData().release());
+ public long release() {
+ Throwable lastException = null;
+ int failedReleaseSize = 0;
+ long releaseSize = 0;
+ for (ShufflePartitionedBlock spb : blocksMap.values()) {
+ try {
+ spb.getData().release();
+ releaseSize += spb.getSize();
+ } catch (Throwable t) {
+ lastException = t;
+ failedReleaseSize += spb.getSize();
+ }
+ }
+ if (lastException != null) {
+ LOG.warn(
+ "Failed to release shuffle blocks with size (). Maybe it released by others",
Review Comment:
Why will it be released by others?
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong closed pull request #1931: [#1930] fix(server): Fix a double release issue about clearResource thread
URL: https://github.com/apache/incubator-uniffle/pull/1931
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#issuecomment-2270196482
@rickyma Thanks for your review before. After some efforts, we find the root cause is that the buffer has been released after `updateCachedBlockIds` throws ExceedPartitionHardLimitException.
Eventually, we have resolved this root cause by removing partition huge limit check logic within `updateCachedBlockIds`.
But for this PR, it is still necessary to warn and handle the exception and avoid leak memory.
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "lwllvyb (via GitHub)" <gi...@apache.org>.
lwllvyb commented on code in PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#discussion_r1705472070
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithSkipList.java:
##########
@@ -111,8 +111,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocksMap.values().forEach(spb -> spb.getData().release());
+ public long release() {
Review Comment:
> Thread-1( toFlushEvent): List spBlocks = new LinkedList<>(blocks);
**toFlushEvent** will be guarded by app read lock.
> Thread-2(release): blocks.forEach(spb -> spb.getData().release()); (The first release)
The **release** function be called in **removeResources** or **removeResourcesByShuffleIds**, guarded by app write lock.
So this situation cannot happen. I don't know if my decription is corret.
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong closed pull request #1931: [#1930] fix(server): Fix a double release issue about clearResource thread
URL: https://github.com/apache/incubator-uniffle/pull/1931
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#discussion_r1704857744
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithSkipList.java:
##########
@@ -111,8 +111,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocksMap.values().forEach(spb -> spb.getData().release());
+ public long release() {
Review Comment:
I think we need to add synchronized here. Calling the `toFlushEvent` method and the `release` method at the same time may result in duplicate release.
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#discussion_r1706283169
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithSkipList.java:
##########
@@ -111,8 +111,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocksMap.values().forEach(spb -> spb.getData().release());
+ public long release() {
Review Comment:
I see. But i think relying on app locks is not a reliable solution. The `release` method may be used by other logic in the future, and the same problem may occur again. It's ok for me not to change it.
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "lwllvyb (via GitHub)" <gi...@apache.org>.
lwllvyb commented on code in PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#discussion_r1706574741
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithSkipList.java:
##########
@@ -111,8 +111,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocksMap.values().forEach(spb -> spb.getData().release());
+ public long release() {
Review Comment:
I agree with your point.
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#issuecomment-2270236068
> @rickyma Thanks for your review before. After some efforts, we find the root cause is that the buffer has been released after `updateCachedBlockIds` throws ExceedPartitionHardLimitException.
>
> Eventually, we have resolved this root cause by removing partition huge limit check logic within `updateCachedBlockIds`.
>
> But for this PR, it is still necessary to warn and handle the exception and avoid leak memory.
This may not be the only reason. I also encountered this problem with version 0.9.0.
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#issuecomment-2270199467
## [Codecov](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1931?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `40.00000%` with `24 lines` in your changes missing coverage. Please review.
> Project coverage is 52.45%. Comparing base [(`0a60834`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/0a608347ef1cbfc7933a3fabf29c2d51520e365d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`c958755`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/c958755ceade3a0e99bdbd3071c0438e5f847954?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 51 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1931?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...iffle/server/buffer/ShuffleBufferWithSkipList.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1931?src=pr&el=tree&filepath=server%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Funiffle%2Fserver%2Fbuffer%2FShuffleBufferWithSkipList.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9idWZmZXIvU2h1ZmZsZUJ1ZmZlcldpdGhTa2lwTGlzdC5qYXZh) | 0.00% | [15 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1931?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...fle/server/buffer/ShuffleBufferWithLinkedList.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1931?src=pr&el=tree&filepath=server%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Funiffle%2Fserver%2Fbuffer%2FShuffleBufferWithLinkedList.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9idWZmZXIvU2h1ZmZsZUJ1ZmZlcldpdGhMaW5rZWRMaXN0LmphdmE=) | 60.00% | [5 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1931?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...he/uniffle/server/buffer/ShuffleBufferManager.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1931?src=pr&el=tree&filepath=server%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Funiffle%2Fserver%2Fbuffer%2FShuffleBufferManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9idWZmZXIvU2h1ZmZsZUJ1ZmZlck1hbmFnZXIuamF2YQ==) | 66.66% | [1 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1931?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 #1931 +/- ##
============================================
- Coverage 53.12% 52.45% -0.67%
+ Complexity 3002 2535 -467
============================================
Files 448 402 -46
Lines 24482 18489 -5993
Branches 2282 1678 -604
============================================
- Hits 13006 9699 -3307
+ Misses 10669 8199 -2470
+ Partials 807 591 -216
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1931?dropdown=coverage&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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on code in PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#discussion_r1722865075
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithSkipList.java:
##########
@@ -111,8 +111,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocksMap.values().forEach(spb -> spb.getData().release());
+ public long release() {
+ Throwable lastException = null;
+ int failedReleaseSize = 0;
+ long releaseSize = 0;
+ for (ShufflePartitionedBlock spb : blocksMap.values()) {
+ try {
+ spb.getData().release();
+ releaseSize += spb.getSize();
+ } catch (Throwable t) {
+ lastException = t;
+ failedReleaseSize += spb.getSize();
+ }
+ }
+ if (lastException != null) {
+ LOG.warn(
+ "Failed to release shuffle blocks with size (). Maybe it released by others",
Review Comment:
ditto
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferWithLinkedList.java:
##########
@@ -110,8 +110,26 @@ public int getBlockCount() {
}
@Override
- public void release() {
- blocks.forEach(spb -> spb.getData().release());
+ public long release() {
+ Throwable lastException = null;
+ int failedReleaseSize = 0;
+ long releaseSize = 0;
+ for (ShufflePartitionedBlock spb : blocks) {
+ try {
+ spb.getData().release();
+ releaseSize += spb.getSize();
+ } catch (Throwable t) {
+ lastException = t;
+ failedReleaseSize += spb.getSize();
+ }
+ }
+ if (lastException != null) {
+ LOG.warn(
+ "Failed to release shuffle blocks with size {}. Maybe it released by others",
Review Comment:
Maybe it has been released by others
--
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] [#1930] fix(server): Fix a double release issue about clearResource thread [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1931:
URL: https://github.com/apache/incubator-uniffle/pull/1931#issuecomment-2237976241
We need to find out the root cause why it has been released multiple times..
--
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