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 2024/05/08 05:48:40 UTC
[PR] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
zuston opened a new pull request, #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679
### What changes were proposed in this pull request?
Descrease the disk size that calculated by local storage self on removing resources with AppPurgeEvent
### Why are the changes needed?
Fix: #1678
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Unit tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2100780175
If the disk is broken, the shuffle may fail to remove. Will the size be calculated correctly?
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston merged PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2099789649
This is also should be cherry picked into branch-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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#discussion_r1593671689
##########
server/src/test/java/org/apache/uniffle/server/ShuffleTaskManagerTest.java:
##########
@@ -104,6 +104,67 @@ public void afterEach() throws Exception {
ShuffleServerMetrics.clear();
}
+ /** Test the shuffleMeta's diskSize when app is removed. */
+ @Test
+ public void appPurgeWithLocalfileTest() throws Exception {
+ String confFile = ClassLoader.getSystemResource("server.conf").getFile();
+ ShuffleServerConf conf = new ShuffleServerConf(confFile);
+ conf.set(ShuffleServerConf.RPC_SERVER_PORT, 1234);
+ conf.set(ShuffleServerConf.RSS_COORDINATOR_QUORUM, "localhost:9527");
+ conf.set(ShuffleServerConf.JETTY_HTTP_PORT, 12345);
+ conf.set(ShuffleServerConf.JETTY_CORE_POOL_SIZE, 64);
+ conf.set(ShuffleServerConf.SERVER_BUFFER_CAPACITY, 1000L);
+ conf.set(ShuffleServerConf.SERVER_MEMORY_SHUFFLE_HIGHWATERMARK_PERCENTAGE, 50.0);
+ conf.set(ShuffleServerConf.SERVER_MEMORY_SHUFFLE_LOWWATERMARK_PERCENTAGE, 0.0);
+ conf.set(ShuffleServerConf.SERVER_COMMIT_TIMEOUT, 10000L);
+ conf.set(ShuffleServerConf.SERVER_APP_EXPIRED_WITHOUT_HEARTBEAT, 100000L);
+ conf.set(ShuffleServerConf.HEALTH_CHECK_ENABLE, false);
+
+ conf.setString(ShuffleServerConf.RSS_STORAGE_TYPE.key(), "LOCALFILE");
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2099872199
cc @rickyma
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2101855476
> > > > This is another case and that is why I prefer not using the disk size that uniffle maintained.
> > >
> > >
> > > Here we have calculated the space usage of the server, could we use it for judgment? @jerqi @zuston
> > > https://github.com/apache/incubator-uniffle/blob/43bd09c652889919ad888d31d4e3a5461c93f21d/server/src/main/java/org/apache/uniffle/server/LocalStorageChecker.java#L116
> >
> >
> > This can't handle mix deployment with other services.
>
> No, it can.
>
> https://github.com/apache/incubator-uniffle/blob/43bd09c652889919ad888d31d4e3a5461c93f21d/server/src/main/java/org/apache/uniffle/server/LocalStorageChecker.java#L200-L224
Ok for me.
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2101774705
> > If the disk is broken, the shuffle may fail to remove. Will the size be calculated correctly? I haven't seen similar test case.
>
> This is another case and that is why I prefer not using the disk size that uniffle maintained.
+1. I'm planning to add a switch to skip determine whether it is writable by using the disk size in metaData.
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on code in PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#discussion_r1594903515
##########
server/src/test/java/org/apache/uniffle/server/ShuffleTaskManagerTest.java:
##########
@@ -104,6 +104,67 @@ public void afterEach() throws Exception {
ShuffleServerMetrics.clear();
}
+ /** Test the shuffleMeta's diskSize when app is removed. */
+ @Test
+ public void appPurgeWithLocalfileTest() throws Exception {
+ String confFile = ClassLoader.getSystemResource("server.conf").getFile();
+ ShuffleServerConf conf = new ShuffleServerConf(confFile);
+ conf.set(ShuffleServerConf.RPC_SERVER_PORT, 1234);
+ conf.set(ShuffleServerConf.RSS_COORDINATOR_QUORUM, "localhost:9527");
+ conf.set(ShuffleServerConf.JETTY_HTTP_PORT, 12345);
+ conf.set(ShuffleServerConf.JETTY_CORE_POOL_SIZE, 64);
+ conf.set(ShuffleServerConf.SERVER_BUFFER_CAPACITY, 1000L);
+ conf.set(ShuffleServerConf.SERVER_MEMORY_SHUFFLE_HIGHWATERMARK_PERCENTAGE, 50.0);
+ conf.set(ShuffleServerConf.SERVER_MEMORY_SHUFFLE_LOWWATERMARK_PERCENTAGE, 0.0);
+ conf.set(ShuffleServerConf.SERVER_COMMIT_TIMEOUT, 10000L);
+ conf.set(ShuffleServerConf.SERVER_APP_EXPIRED_WITHOUT_HEARTBEAT, 100000L);
+ conf.set(ShuffleServerConf.HEALTH_CHECK_ENABLE, false);
+
+ conf.setString(ShuffleServerConf.RSS_STORAGE_TYPE.key(), "LOCALFILE");
Review Comment:
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2101841778
> > This is another case and that is why I prefer not using the disk size that uniffle maintained.
>
> Here we have calculated the space usage of the server, could we use it for judgment? @jerqi @zuston
>
> https://github.com/apache/incubator-uniffle/blob/43bd09c652889919ad888d31d4e3a5461c93f21d/server/src/main/java/org/apache/uniffle/server/LocalStorageChecker.java#L116
+1 for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2101885203
> > Another concern is performance. Is it related to system call?
>
> Yeah, do you know the original background of `LocalStorageMeta`? Was it designed to store disk size in `LocalStorageMeta` to maintain performance, considering that frequent access to disk size might affect performance?
No, this is used for uploading data from the disk to HDFS in the origin design.
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on code in PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#discussion_r1593639143
##########
server/src/test/java/org/apache/uniffle/server/ShuffleTaskManagerTest.java:
##########
@@ -104,6 +104,67 @@ public void afterEach() throws Exception {
ShuffleServerMetrics.clear();
}
+ /** Test the shuffleMeta's diskSize when app is removed. */
+ @Test
+ public void appPurgeWithLocalfileTest() throws Exception {
+ String confFile = ClassLoader.getSystemResource("server.conf").getFile();
+ ShuffleServerConf conf = new ShuffleServerConf(confFile);
+ conf.set(ShuffleServerConf.RPC_SERVER_PORT, 1234);
+ conf.set(ShuffleServerConf.RSS_COORDINATOR_QUORUM, "localhost:9527");
+ conf.set(ShuffleServerConf.JETTY_HTTP_PORT, 12345);
+ conf.set(ShuffleServerConf.JETTY_CORE_POOL_SIZE, 64);
+ conf.set(ShuffleServerConf.SERVER_BUFFER_CAPACITY, 1000L);
+ conf.set(ShuffleServerConf.SERVER_MEMORY_SHUFFLE_HIGHWATERMARK_PERCENTAGE, 50.0);
+ conf.set(ShuffleServerConf.SERVER_MEMORY_SHUFFLE_LOWWATERMARK_PERCENTAGE, 0.0);
+ conf.set(ShuffleServerConf.SERVER_COMMIT_TIMEOUT, 10000L);
+ conf.set(ShuffleServerConf.SERVER_APP_EXPIRED_WITHOUT_HEARTBEAT, 100000L);
+ conf.set(ShuffleServerConf.HEALTH_CHECK_ENABLE, false);
+
+ conf.setString(ShuffleServerConf.RSS_STORAGE_TYPE.key(), "LOCALFILE");
Review Comment:
Also, if you have time, it will be better if the following code in this file could be extracted as a private base method to reduce code duplication. I see many duplicated codes in this file:
```
String confFile = ClassLoader.getSystemResource("server.conf").getFile();
ShuffleServerConf conf = new ShuffleServerConf(confFile);
conf.set(ShuffleServerConf.RPC_SERVER_PORT, 1234);
conf.set(ShuffleServerConf.RSS_COORDINATOR_QUORUM, "localhost:9527");
conf.set(ShuffleServerConf.JETTY_HTTP_PORT, 12345);
conf.set(ShuffleServerConf.JETTY_CORE_POOL_SIZE, 64);
conf.set(ShuffleServerConf.SERVER_BUFFER_CAPACITY, 1000L);
conf.set(ShuffleServerConf.SERVER_MEMORY_SHUFFLE_HIGHWATERMARK_PERCENTAGE, 50.0);
conf.set(ShuffleServerConf.SERVER_MEMORY_SHUFFLE_LOWWATERMARK_PERCENTAGE, 0.0);
conf.set(ShuffleServerConf.SERVER_COMMIT_TIMEOUT, 10000L);
conf.set(ShuffleServerConf.SERVER_APP_EXPIRED_WITHOUT_HEARTBEAT, 100000L);
conf.set(ShuffleServerConf.HEALTH_CHECK_ENABLE, false);
```
##########
server/src/test/java/org/apache/uniffle/server/ShuffleTaskManagerTest.java:
##########
@@ -104,6 +104,67 @@ public void afterEach() throws Exception {
ShuffleServerMetrics.clear();
}
+ /** Test the shuffleMeta's diskSize when app is removed. */
+ @Test
+ public void appPurgeWithLocalfileTest() throws Exception {
+ String confFile = ClassLoader.getSystemResource("server.conf").getFile();
+ ShuffleServerConf conf = new ShuffleServerConf(confFile);
+ conf.set(ShuffleServerConf.RPC_SERVER_PORT, 1234);
+ conf.set(ShuffleServerConf.RSS_COORDINATOR_QUORUM, "localhost:9527");
+ conf.set(ShuffleServerConf.JETTY_HTTP_PORT, 12345);
+ conf.set(ShuffleServerConf.JETTY_CORE_POOL_SIZE, 64);
+ conf.set(ShuffleServerConf.SERVER_BUFFER_CAPACITY, 1000L);
+ conf.set(ShuffleServerConf.SERVER_MEMORY_SHUFFLE_HIGHWATERMARK_PERCENTAGE, 50.0);
+ conf.set(ShuffleServerConf.SERVER_MEMORY_SHUFFLE_LOWWATERMARK_PERCENTAGE, 0.0);
+ conf.set(ShuffleServerConf.SERVER_COMMIT_TIMEOUT, 10000L);
+ conf.set(ShuffleServerConf.SERVER_APP_EXPIRED_WITHOUT_HEARTBEAT, 100000L);
+ conf.set(ShuffleServerConf.HEALTH_CHECK_ENABLE, false);
+
+ conf.setString(ShuffleServerConf.RSS_STORAGE_TYPE.key(), "LOCALFILE");
Review Comment:
"LOCALFILE" -> StorageType.LOCALFILE.name()
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2103767250
Merged. Thanks for your review @rickyma @jerqi @xianjingfeng
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2099830262
## Test Results
2 384 files ± 0 2 384 suites ±0 4h 39m 12s :stopwatch: -15s
923 tests + 1 922 :white_check_mark: + 1 1 :zzz: ±0 0 :x: ±0
10 705 runs +14 10 691 :white_check_mark: +14 14 :zzz: ±0 0 :x: ±0
Results for commit 4d3b2c11. ± Comparison against base commit 917456c5.
[test-results]:data:application/gzip;base64,H4sIALYaO2YC/03MTQ6DIBCG4asY1l0wIFh7mQZGTEj9aRBWTe/ewSBlRd4H+D5s9os72KMT8t7fOnYkH9ueUjDR7xsJ6EEJIrqP+cUo5FXPIyGeJP708u/8q8Js/ELAK7gQ9lAkpC1vAh+4KnmNAtcjVCur/QXN6tntKO7r6iMF6ydpBQJoBxwHOg0ngHlUVlplnDNSGFTIvj9fFtjREgEAAA==
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#discussion_r1594805230
##########
server/src/test/java/org/apache/uniffle/server/ShuffleTaskManagerTest.java:
##########
@@ -104,6 +104,67 @@ public void afterEach() throws Exception {
ShuffleServerMetrics.clear();
}
+ /** Test the shuffleMeta's diskSize when app is removed. */
+ @Test
+ public void appPurgeWithLocalfileTest() throws Exception {
+ String confFile = ClassLoader.getSystemResource("server.conf").getFile();
+ ShuffleServerConf conf = new ShuffleServerConf(confFile);
+ conf.set(ShuffleServerConf.RPC_SERVER_PORT, 1234);
+ conf.set(ShuffleServerConf.RSS_COORDINATOR_QUORUM, "localhost:9527");
+ conf.set(ShuffleServerConf.JETTY_HTTP_PORT, 12345);
+ conf.set(ShuffleServerConf.JETTY_CORE_POOL_SIZE, 64);
+ conf.set(ShuffleServerConf.SERVER_BUFFER_CAPACITY, 1000L);
+ conf.set(ShuffleServerConf.SERVER_MEMORY_SHUFFLE_HIGHWATERMARK_PERCENTAGE, 50.0);
+ conf.set(ShuffleServerConf.SERVER_MEMORY_SHUFFLE_LOWWATERMARK_PERCENTAGE, 0.0);
+ conf.set(ShuffleServerConf.SERVER_COMMIT_TIMEOUT, 10000L);
+ conf.set(ShuffleServerConf.SERVER_APP_EXPIRED_WITHOUT_HEARTBEAT, 100000L);
+ conf.set(ShuffleServerConf.HEALTH_CHECK_ENABLE, false);
+
+ conf.setString(ShuffleServerConf.RSS_STORAGE_TYPE.key(), "LOCALFILE");
Review Comment:
Let’s put this optimization in another PR as the good first issue
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2103771223
> Merged. Thanks for your review @rickyma @jerqi @xianjingfeng
@zuston Could you commit this pr to branch 0.9, too?
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2099980188
> I've seen it already, that's why I think this should be a common issue. It should happen easily in a prod env. Isn't removing app happening all the time, is it?
Yes. But I think this will happen when AppPurgeEvent is triggered and ShufflePurgeEvent is not triggered at the same time, maybe this is rare.
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2101817848
> This is another case and that is why I prefer not using the disk size that uniffle maintained.
Could we use this variable instead? @zuston @jerqi
https://github.com/apache/incubator-uniffle/blob/43bd09c652889919ad888d31d4e3a5461c93f21d/storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java#L49
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2099960470
> If I get this correctly, this should be a bug that is easily encountered in a production environment, unless you don't primarily write to local disks in your production env.
You can see the test case to understand this. And this has happened in our internal env.
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2099998948
> Yes. But I think this will happen when AppPurgeEvent is triggered and ShufflePurgeEvent is not triggered at the same time, maybe this is rare.
ShufflePurgeEvent will only be created when an unregister shuffle request is received.
So I think it will help a lot in my case. This is very common in my env. Because Spark jobs might be killed by users, which will cause the threads interrupted and fail to continue to send unregister shuffle requests to shuffle servers. The stack trace is something like:
```
[21:57:36:865] [unregister-shuffle-0] WARN org.apache.uniffle.client.impl.ShuffleWriteClientImpl.lambda$unregisterShuffle$28:1021 - Error happened when unregistering to ShuffleServerInfo{host[123.123.123.123], grpc port[12345], netty port[17000]}
io.grpc.StatusRuntimeException: CANCELLED: Thread interrupted
at io.grpc.stub.ClientCalls.toStatusRuntimeException(ClientCalls.java:268) ~[rss-client-spark3-shaded-0.9.0-SNAPSHOT.jar:?]
at io.grpc.stub.ClientCalls.getUnchecked(ClientCalls.java:249) ~[rss-client-spark3-shaded-0.9.0-SNAPSHOT.jar:?]
at io.grpc.stub.ClientCalls.blockingUnaryCall(ClientCalls.java:167) ~[rss-client-spark3-shaded-0.9.0-SNAPSHOT.jar:?]
at org.apache.uniffle.proto.ShuffleServerGrpc$ShuffleServerBlockingStub.unregisterShuffleByAppId(ShuffleServerGrpc.java:772) ~[rss-client-spark3-shaded-0.9.0-SNAPSHOT.jar:?]
at org.apache.uniffle.client.impl.grpc.ShuffleServerGrpcClient.doUnregisterShuffleByAppId(ShuffleServerGrpcClient.java:337) ~[rss-client-spark3-shaded-0.9.0-SNAPSHOT.jar:?]
at org.apache.uniffle.client.impl.grpc.ShuffleServerGrpcClient.unregisterShuffleByAppId(ShuffleServerGrpcClient.java:344) ~[rss-client-spark3-shaded-0.9.0-SNAPSHOT.jar:?]
at org.apache.uniffle.client.impl.ShuffleWriteClientImpl.lambda$unregisterShuffle$28(ShuffleWriteClientImpl.java:1016) ~[rss-client-spark3-shaded-0.9.0-SNAPSHOT.jar:?]
at org.apache.uniffle.common.util.ThreadUtils.lambda$null$0(ThreadUtils.java:110) ~[rss-client-spark3-shaded-0.9.0-SNAPSHOT.jar:?]
at java.util.concurrent.FutureTask.run(FutureTask.java:266) ~[?:1.8.0_352]
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) ~[?:1.8.0_352]
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) ~[?:1.8.0_352]
at java.lang.Thread.run(Thread.java:750) ~[?:1.8.0_352]
Caused by: java.lang.InterruptedException
at io.grpc.stub.ClientCalls$ThreadlessExecutor.throwIfInterrupted(ClientCalls.java:750) ~[rss-client-spark3-shaded-0.9.0-SNAPSHOT.jar:?]
at io.grpc.stub.ClientCalls$ThreadlessExecutor.waitAndDrain(ClientCalls.java:711) ~[rss-client-spark3-shaded-0.9.0-SNAPSHOT.jar:?]
at io.grpc.stub.ClientCalls.blockingUnaryCall(ClientCalls.java:159) ~[rss-client-spark3-shaded-0.9.0-SNAPSHOT.jar:?]
... 9 more
```
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2101841612
> > This is another case and that is why I prefer not using the disk size that uniffle maintained.
>
> Here we have calculated the space usage of the server, could we use it for judgment? @jerqi @zuston
>
> https://github.com/apache/incubator-uniffle/blob/43bd09c652889919ad888d31d4e3a5461c93f21d/server/src/main/java/org/apache/uniffle/server/LocalStorageChecker.java#L116
This can't handle mix deployment with other services.
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2101868429
> Another concern is performance. Is it related to system call?
Yeah, do you know the original background of `LocalStorageMeta`? Was it designed to store disk size in `LocalStorageMeta` to maintain performance, considering that frequent access to disk size might affect 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
Re: [PR] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2101847924
> > > This is another case and that is why I prefer not using the disk size that uniffle maintained.
> >
> >
> > Here we have calculated the space usage of the server, could we use it for judgment? @jerqi @zuston
> > https://github.com/apache/incubator-uniffle/blob/43bd09c652889919ad888d31d4e3a5461c93f21d/server/src/main/java/org/apache/uniffle/server/LocalStorageChecker.java#L116
>
> This can't handle mix deployment with other services.
No, it can.
https://github.com/apache/incubator-uniffle/blob/43bd09c652889919ad888d31d4e3a5461c93f21d/server/src/main/java/org/apache/uniffle/server/LocalStorageChecker.java#L200-L224
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#discussion_r1594906134
##########
server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java:
##########
@@ -771,7 +772,9 @@ public void removeResources(String appId, boolean checkAppExpired) {
partitionsToBlockIds.remove(appId);
shuffleBufferManager.removeBuffer(appId);
shuffleFlushManager.removeResources(appId);
- storageManager.removeResources(new AppPurgeEvent(appId, shuffleTaskInfo.getUser()));
+ storageManager.removeResources(
+ new AppPurgeEvent(
Review Comment:
This will bring too many changes.
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2101822630
> This is another case and that is why I prefer not using the disk size that uniffle maintained.
Here we have calculated the space usage of the server, could we use it for judgment?
https://github.com/apache/incubator-uniffle/blob/43bd09c652889919ad888d31d4e3a5461c93f21d/server/src/main/java/org/apache/uniffle/server/LocalStorageChecker.java#L116
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2101855943
Another concern is performance. Is it related to system call?
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2099971906
I've seen it already, that's why I think this should be a common issue. It should happen easily in a prod env. Isn't removing app happening all the time, is 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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on code in PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#discussion_r1593962065
##########
server/src/test/java/org/apache/uniffle/server/ShuffleTaskManagerTest.java:
##########
@@ -104,6 +104,67 @@ public void afterEach() throws Exception {
ShuffleServerMetrics.clear();
}
+ /** Test the shuffleMeta's diskSize when app is removed. */
+ @Test
+ public void appPurgeWithLocalfileTest() throws Exception {
+ String confFile = ClassLoader.getSystemResource("server.conf").getFile();
+ ShuffleServerConf conf = new ShuffleServerConf(confFile);
+ conf.set(ShuffleServerConf.RPC_SERVER_PORT, 1234);
+ conf.set(ShuffleServerConf.RSS_COORDINATOR_QUORUM, "localhost:9527");
+ conf.set(ShuffleServerConf.JETTY_HTTP_PORT, 12345);
+ conf.set(ShuffleServerConf.JETTY_CORE_POOL_SIZE, 64);
+ conf.set(ShuffleServerConf.SERVER_BUFFER_CAPACITY, 1000L);
+ conf.set(ShuffleServerConf.SERVER_MEMORY_SHUFFLE_HIGHWATERMARK_PERCENTAGE, 50.0);
+ conf.set(ShuffleServerConf.SERVER_MEMORY_SHUFFLE_LOWWATERMARK_PERCENTAGE, 0.0);
+ conf.set(ShuffleServerConf.SERVER_COMMIT_TIMEOUT, 10000L);
+ conf.set(ShuffleServerConf.SERVER_APP_EXPIRED_WITHOUT_HEARTBEAT, 100000L);
+ conf.set(ShuffleServerConf.HEALTH_CHECK_ENABLE, false);
+
+ conf.setString(ShuffleServerConf.RSS_STORAGE_TYPE.key(), "LOCALFILE");
Review Comment:
Can you refactor all the other codes in `ShuffleTaskManagerTest.java` using `constructServerConfWithLocalfile`? If you search for `String confFile = ClassLoader.getSystemResource("server.conf").getFile()`, you will find a lot of duplicated codes.
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#discussion_r1593799289
##########
server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java:
##########
@@ -771,7 +772,9 @@ public void removeResources(String appId, boolean checkAppExpired) {
partitionsToBlockIds.remove(appId);
shuffleBufferManager.removeBuffer(appId);
shuffleFlushManager.removeResources(appId);
- storageManager.removeResources(new AppPurgeEvent(appId, shuffleTaskInfo.getUser()));
+ storageManager.removeResources(
+ new AppPurgeEvent(
Review Comment:
Should we remove the following constructor? For prevent the same problem from happening again.
https://github.com/apache/incubator-uniffle/blob/917456c5eb0d7b70f7faa5da489aaa29d54cde40/server/src/main/java/org/apache/uniffle/server/event/AppPurgeEvent.java#L28-L30
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2099920454
If I get this correctly, this should be a bug that is easily encountered in a production environment, unless you don't primarily write to local disks in your production env.
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2100008980
This is a easy-to-understand bug. Could you help review this? @rickyma
--
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] [#1678] fix(server): disk size leak on removing resources by AppPurgeEvent [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #1679:
URL: https://github.com/apache/incubator-uniffle/pull/1679#issuecomment-2101658602
> If the disk is broken, the shuffle may fail to remove. Will the size be calculated correctly? I haven't seen similar test case.
This is another case and that is why I prefer not using the disk size that uniffle maintained.
--
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