You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by GitBox <gi...@apache.org> on 2022/07/26 08:10:05 UTC
[GitHub] [incubator-uniffle] xianjingfeng opened a new pull request, #74: [Improvement] ShuffleBlock should be release when finished reading
xianjingfeng opened a new pull request, #74:
URL: https://github.com/apache/incubator-uniffle/pull/74
issue: #73
--
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
[GitHub] [incubator-uniffle] jerqi commented on pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#issuecomment-1196936302
> > We will replace grpc with netty in the future. We hope we can use offheap memory totally.
>
> When? Do we have a detailed plan？
Maybe October, we hope to do that, but there are always other important things to do. For 0.6 version, we plan to support to deploy Uniffle. For 0.7 version, we plan to replace grpc with netty, but we only replace part interface (read shuffle data and write shuffle data).
--
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
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r932462578
##########
common/src/test/java/org/apache/uniffle/common/RssShuffleUtilsTest.java:
##########
@@ -46,4 +48,27 @@ public void testCompression(int size) {
assertArrayEquals(data, buffer2);
}
+ @Test
+ public void testDestroyDirectByteBuffer() throws Exception{
+ int size = 10;
+ byte b = 1;
+ System.out.println(b);
+ ByteBuffer byteBuffer = ByteBuffer.allocateDirect(size);
+ for (int i = 0; i < size; i++) {
+ byteBuffer.put(b);
+ }
+ byteBuffer.flip();
+ RssShuffleUtils.destroyDirectByteBuffer(byteBuffer);
+ Thread.sleep(200); //Memory release maybe not fast enough
Review Comment:
```
// Memory release may be not fast enough.
Thread.sleep(200);
```
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
xianjingfeng commented on PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#issuecomment-1195405767
> Did you verify this patch? I think we would better have ut for every pr, we should test the feature by hand at least.
We have tested in production environment. Sometimes it is difficult to write ut and , I will try to write ut for this pr later
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
xianjingfeng commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r930567865
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java:
##########
@@ -38,6 +38,7 @@
import org.apache.uniffle.client.api.ShuffleReadClient;
import org.apache.uniffle.client.response.CompressedShuffleBlock;
import org.apache.uniffle.common.RssShuffleUtils;
+import sun.nio.ch.DirectBuffer;
Review Comment:
I think there is no need to use `DirectBuffer` here, use `HeapByteBuffer` when decompress is better, in our internal version, we can use `HeapByteBuffer` or `DirectBuffer`, and we found no apparent difference in performance.
--
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
[GitHub] [incubator-uniffle] colinmjj commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
colinmjj commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r930578314
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java:
##########
@@ -106,8 +108,11 @@ public boolean hasNext() {
shuffleReadMetrics.incFetchWaitTime(fetchDuration);
if (compressedData != null) {
shuffleReadMetrics.incRemoteBytesRead(compressedData.limit() - compressedData.position());
+ if (uncompressedData != null && uncompressedData.isDirect()) {
+ ((DirectBuffer)uncompressedData).cleaner().clean();
Review Comment:
@xianjingfeng Good catch for these possible resource leak, but there should be different methods to do the clean staff according to JDK's version. Here is how spark deal with it, https://github.com/apache/spark/blob/master/common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java#L63
--
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
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r930138067
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java:
##########
@@ -106,8 +108,11 @@ public boolean hasNext() {
shuffleReadMetrics.incFetchWaitTime(fetchDuration);
if (compressedData != null) {
shuffleReadMetrics.incRemoteBytesRead(compressedData.limit() - compressedData.position());
Review Comment:
We should add some comments to explain why we need to do these `clean operation`.
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java:
##########
@@ -38,6 +38,7 @@
import org.apache.uniffle.client.api.ShuffleReadClient;
import org.apache.uniffle.client.response.CompressedShuffleBlock;
import org.apache.uniffle.common.RssShuffleUtils;
+import sun.nio.ch.DirectBuffer;
Review Comment:
Should we use ` java.nio.DirectBuffer`?
--
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
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r932403800
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java:
##########
@@ -106,8 +107,17 @@ public boolean hasNext() {
shuffleReadMetrics.incFetchWaitTime(fetchDuration);
if (compressedData != null) {
shuffleReadMetrics.incRemoteBytesRead(compressedData.limit() - compressedData.position());
+ // Directbytebuffers are not collected in time will cause executor easy
+ // be killed by cluster managers(such as YARN) for using too much offheap memory
+ if (uncompressedData != null && uncompressedData.isDirect()) {
+ try {
+ RssShuffleUtils.destroyDirectByteBuffer(uncompressedData);
+ } catch (Exception e) {
+ throw new RuntimeException("Destroy DirectByteBuffer failed!", e);
Review Comment:
RuntimeException -> RssException
For Spark Client, we usually throw a RssException. So we can count how many fail tasks are due to rss by the type of exception.
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
xianjingfeng commented on PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#issuecomment-1196197955
> curious
SF
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
xianjingfeng commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r932809957
##########
common/src/test/java/org/apache/uniffle/common/RssShuffleUtilsTest.java:
##########
@@ -46,4 +48,27 @@ public void testCompression(int size) {
assertArrayEquals(data, buffer2);
}
+ @Test
+ public void testDestroyDirectByteBuffer() throws Exception{
+ int size = 10;
+ byte b = 1;
+ System.out.println(b);
Review Comment:
agree
--
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
[GitHub] [incubator-uniffle] codecov-commenter commented on pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#issuecomment-1198826560
# [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/74?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#74](https://codecov.io/gh/apache/incubator-uniffle/pull/74?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fdbd88c) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/aa18be05ab7c00cd04b98134c11fdbe898893e95?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (aa18be0) will **decrease** coverage by `1.01%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #74 +/- ##
============================================
- Coverage 56.39% 55.37% -1.02%
+ Complexity 1173 1101 -72
============================================
Files 149 140 -9
Lines 7953 7617 -336
Branches 761 735 -26
============================================
- Hits 4485 4218 -267
+ Misses 3226 3165 -61
+ Partials 242 234 -8
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/74?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ava/org/apache/uniffle/common/RssShuffleUtils.java](https://codecov.io/gh/apache/incubator-uniffle/pull/74/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9Sc3NTaHVmZmxlVXRpbHMuamF2YQ==) | `60.86% <0.00%> (-32.47%)` | :arrow_down: |
| [.../apache/uniffle/common/exception/RssException.java](https://codecov.io/gh/apache/incubator-uniffle/pull/74/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9leGNlcHRpb24vUnNzRXhjZXB0aW9uLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...storage/handler/impl/DataSkippableReadHandler.java](https://codecov.io/gh/apache/incubator-uniffle/pull/74/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmFnZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zdG9yYWdlL2hhbmRsZXIvaW1wbC9EYXRhU2tpcHBhYmxlUmVhZEhhbmRsZXIuamF2YQ==) | `81.25% <0.00%> (-3.13%)` | :arrow_down: |
| [.../apache/uniffle/coordinator/CoordinatorServer.java](https://codecov.io/gh/apache/incubator-uniffle/pull/74/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvQ29vcmRpbmF0b3JTZXJ2ZXIuamF2YQ==) | `68.67% <0.00%> (-2.22%)` | :arrow_down: |
| [...org/apache/uniffle/server/ShuffleFlushManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/74/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlRmx1c2hNYW5hZ2VyLmphdmE=) | `76.70% <0.00%> (-1.71%)` | :arrow_down: |
| [.../apache/uniffle/coordinator/ClientConfManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/74/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvQ2xpZW50Q29uZk1hbmFnZXIuamF2YQ==) | `91.54% <0.00%> (-1.41%)` | :arrow_down: |
| [.../java/org/apache/uniffle/server/ShuffleServer.java](https://codecov.io/gh/apache/incubator-uniffle/pull/74/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlU2VydmVyLmphdmE=) | `63.41% <0.00%> (-1.30%)` | :arrow_down: |
| [...he/uniffle/client/impl/ShuffleWriteClientImpl.java](https://codecov.io/gh/apache/incubator-uniffle/pull/74/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NsaWVudC9pbXBsL1NodWZmbGVXcml0ZUNsaWVudEltcGwuamF2YQ==) | `25.95% <0.00%> (-0.10%)` | :arrow_down: |
| [...he/uniffle/coordinator/CoordinatorGrpcService.java](https://codecov.io/gh/apache/incubator-uniffle/pull/74/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvQ29vcmRpbmF0b3JHcnBjU2VydmljZS5qYXZh) | `2.36% <0.00%> (-0.06%)` | :arrow_down: |
| [...java/org/apache/uniffle/common/rpc/GrpcServer.java](https://codecov.io/gh/apache/incubator-uniffle/pull/74/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9ycGMvR3JwY1NlcnZlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [11 more](https://codecov.io/gh/apache/incubator-uniffle/pull/74/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [incubator-uniffle] jerqi commented on pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#issuecomment-1195335362
Did you verify this patch? I think we would better have ut for every pr, we should test the feature by hand at least.
--
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
[GitHub] [incubator-uniffle] colinmjj commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
colinmjj commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r930587396
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java:
##########
@@ -106,8 +108,11 @@ public boolean hasNext() {
shuffleReadMetrics.incFetchWaitTime(fetchDuration);
if (compressedData != null) {
shuffleReadMetrics.incRemoteBytesRead(compressedData.limit() - compressedData.position());
+ if (uncompressedData != null && uncompressedData.isDirect()) {
+ ((DirectBuffer)uncompressedData).cleaner().clean();
Review Comment:
can it be compatible with jdk9 or higher?
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
xianjingfeng commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r931020371
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java:
##########
@@ -38,6 +38,7 @@
import org.apache.uniffle.client.api.ShuffleReadClient;
import org.apache.uniffle.client.response.CompressedShuffleBlock;
import org.apache.uniffle.common.RssShuffleUtils;
+import sun.nio.ch.DirectBuffer;
Review Comment:
No, we add this config option just for test and comparison
--
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
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r932462578
##########
common/src/test/java/org/apache/uniffle/common/RssShuffleUtilsTest.java:
##########
@@ -46,4 +48,27 @@ public void testCompression(int size) {
assertArrayEquals(data, buffer2);
}
+ @Test
+ public void testDestroyDirectByteBuffer() throws Exception{
+ int size = 10;
+ byte b = 1;
+ System.out.println(b);
+ ByteBuffer byteBuffer = ByteBuffer.allocateDirect(size);
+ for (int i = 0; i < size; i++) {
+ byteBuffer.put(b);
+ }
+ byteBuffer.flip();
+ RssShuffleUtils.destroyDirectByteBuffer(byteBuffer);
+ Thread.sleep(200); //Memory release maybe not fast enough
Review Comment:
// Memory release may be not fast enough.
Thread.sleep(200);
--
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
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r930575394
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java:
##########
@@ -38,6 +38,7 @@
import org.apache.uniffle.client.api.ShuffleReadClient;
import org.apache.uniffle.client.response.CompressedShuffleBlock;
import org.apache.uniffle.common.RssShuffleUtils;
+import sun.nio.ch.DirectBuffer;
Review Comment:
Should we add a config option to decide which ByteBuffer to use?
--
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
[GitHub] [incubator-uniffle] jerqi commented on pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#issuecomment-1196813253
> I think use HeapByteBuffer is better here. What do you think?
We find Uniffle's GC time is longer than Spark origin shuffle in our test when the task read shuffle data. So we'd better use off heap memory. But grpc use heap memory, so we can't use offheap memory totally. We will replace grpc with netty.
--
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
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r932462578
##########
common/src/test/java/org/apache/uniffle/common/RssShuffleUtilsTest.java:
##########
@@ -46,4 +48,27 @@ public void testCompression(int size) {
assertArrayEquals(data, buffer2);
}
+ @Test
+ public void testDestroyDirectByteBuffer() throws Exception{
+ int size = 10;
+ byte b = 1;
+ System.out.println(b);
+ ByteBuffer byteBuffer = ByteBuffer.allocateDirect(size);
+ for (int i = 0; i < size; i++) {
+ byteBuffer.put(b);
+ }
+ byteBuffer.flip();
+ RssShuffleUtils.destroyDirectByteBuffer(byteBuffer);
+ Thread.sleep(200); //Memory release maybe not fast enough
Review Comment:
```
// The memory may not be released fast enough.
Thread.sleep(200);
```
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
xianjingfeng commented on PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#issuecomment-1196710538
I think use HeapByteBuffer is better here. What do you think?
--
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
[GitHub] [incubator-uniffle] jerqi commented on pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#issuecomment-1195412814
> > Did you verify this patch? I think we would better have ut for every pr, we should test the feature by hand at least.
>
> We have tested in production environment. Sometimes it is difficult to write ut, I will try to write ut for this pr later
Have your company used the Uniffle in your production environment?
--
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
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r932818260
##########
common/src/test/java/org/apache/uniffle/common/RssShuffleUtilsTest.java:
##########
@@ -46,4 +48,27 @@ public void testCompression(int size) {
assertArrayEquals(data, buffer2);
}
+ @Test
+ public void testDestroyDirectByteBuffer() throws Exception{
+ int size = 10;
+ byte b = 1;
+ System.out.println(b);
Review Comment:
I means that we remove
```
System.out.println
```
I think this test is necessary. We shouldn't remove 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
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r932816357
##########
common/src/test/java/org/apache/uniffle/common/RssShuffleUtilsTest.java:
##########
@@ -46,4 +48,27 @@ public void testCompression(int size) {
assertArrayEquals(data, buffer2);
}
+ @Test
+ public void testDestroyDirectByteBuffer() throws Exception{
Review Comment:
```
public void testDestroyDirectByteBuffer() throws Exception {
```
There should be a blank after Exception.
--
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
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r932462578
##########
common/src/test/java/org/apache/uniffle/common/RssShuffleUtilsTest.java:
##########
@@ -46,4 +48,27 @@ public void testCompression(int size) {
assertArrayEquals(data, buffer2);
}
+ @Test
+ public void testDestroyDirectByteBuffer() throws Exception{
+ int size = 10;
+ byte b = 1;
+ System.out.println(b);
+ ByteBuffer byteBuffer = ByteBuffer.allocateDirect(size);
+ for (int i = 0; i < size; i++) {
+ byteBuffer.put(b);
+ }
+ byteBuffer.flip();
+ RssShuffleUtils.destroyDirectByteBuffer(byteBuffer);
+ Thread.sleep(200); //Memory release maybe not fast enough
Review Comment:
```
// Memory release maybe not fast enough.
Thread.sleep(200);
```
--
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
[GitHub] [incubator-uniffle] jerqi commented on pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#issuecomment-1195658953
> > Did you verify this patch? I think we would better have ut for every pr, we should test the feature by hand at least.
>
> We have tested in production environment. Sometimes it is difficult to write ut, I will try to write ut for this pr later
I know it's sometimes diffcult. If you can't write ut for it. you should provide some production envrionment detailed data to prove the effect of the pr. It's best to have screenshot.
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
xianjingfeng commented on PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#issuecomment-1196198215
> > > Did you verify this patch? I think we would better have ut for every pr, we should test the feature by hand at least.
> >
> >
> > We have tested in production environment. Sometimes it is difficult to write ut, I will try to write ut for this pr later
>
> Have your company used the Uniffle in your production environment? Just curious, could you tell me the name of your company?
SF
--
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
[GitHub] [incubator-uniffle] jerqi merged pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi merged PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
xianjingfeng commented on PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#issuecomment-1195407489
> Did you verify this patch? I think we would better have ut for every pr, we should test the feature by hand at least.
We have tested in production environment. Sometimes it is difficult to write ut. I will try to write ut for this pr later
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
xianjingfeng commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r931024452
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java:
##########
@@ -106,8 +108,11 @@ public boolean hasNext() {
shuffleReadMetrics.incFetchWaitTime(fetchDuration);
if (compressedData != null) {
shuffleReadMetrics.incRemoteBytesRead(compressedData.limit() - compressedData.position());
+ if (uncompressedData != null && uncompressedData.isDirect()) {
+ ((DirectBuffer)uncompressedData).cleaner().clean();
Review Comment:
Have not try
--
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
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r930582978
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java:
##########
@@ -106,8 +108,11 @@ public boolean hasNext() {
shuffleReadMetrics.incFetchWaitTime(fetchDuration);
if (compressedData != null) {
shuffleReadMetrics.incRemoteBytesRead(compressedData.limit() - compressedData.position());
Review Comment:
It seems better to extract a method and use reflection.
More information is as below
https://stackoverflow.com/questions/1854398/how-to-garbage-collect-a-direct-buffer-in-java
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
xianjingfeng commented on PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#issuecomment-1196923067
> We will replace grpc with netty in the future. We hope we can use offheap memory totally.
When? Do we have a detailed plan？
--
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
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r932462578
##########
common/src/test/java/org/apache/uniffle/common/RssShuffleUtilsTest.java:
##########
@@ -46,4 +48,27 @@ public void testCompression(int size) {
assertArrayEquals(data, buffer2);
}
+ @Test
+ public void testDestroyDirectByteBuffer() throws Exception{
+ int size = 10;
+ byte b = 1;
+ System.out.println(b);
+ ByteBuffer byteBuffer = ByteBuffer.allocateDirect(size);
+ for (int i = 0; i < size; i++) {
+ byteBuffer.put(b);
+ }
+ byteBuffer.flip();
+ RssShuffleUtils.destroyDirectByteBuffer(byteBuffer);
+ Thread.sleep(200); //Memory release maybe not fast enough
Review Comment:
```
// The memory may not be freed fast enough.
Thread.sleep(200);
```
--
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
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #74: [Improvement] ShuffleBlock should be release when finished reading
Posted by GitBox <gi...@apache.org>.
jerqi commented on code in PR #74:
URL: https://github.com/apache/incubator-uniffle/pull/74#discussion_r932461206
##########
common/src/test/java/org/apache/uniffle/common/RssShuffleUtilsTest.java:
##########
@@ -46,4 +48,27 @@ public void testCompression(int size) {
assertArrayEquals(data, buffer2);
}
+ @Test
+ public void testDestroyDirectByteBuffer() throws Exception{
+ int size = 10;
+ byte b = 1;
+ System.out.println(b);
Review Comment:
remove?
--
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