You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "zwangsheng (via GitHub)" <gi...@apache.org> on 2023/08/14 07:06:53 UTC
[GitHub] [incubator-celeborn] zwangsheng opened a new pull request, #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
zwangsheng opened a new pull request, #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
Tweak the logic of `MemoryManager#currentServingState`
Add Unit Test for this function
### Why are the changes needed?
Make this method logical, and add unit test to ensure logic won't be accidental modification
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Add Unit Test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] waitinfuture closed pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
URL: https://github.com/apache/incubator-celeborn/pull/1811
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#issuecomment-1676810763
> Make this method logical ...
Any details? What problem do you find in the current implementation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] cfmcgrady commented on pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#issuecomment-1679900797
> As sbt unit test fail, WIP @cfmcgrady
Fixed in https://github.com/apache/incubator-celeborn/pull/1818. cc @zwangsheng
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] zwangsheng commented on a diff in pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on code in PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#discussion_r1301269058
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -253,27 +258,24 @@ private MemoryManager(CelebornConf conf) {
public ServingState currentServingState() {
long memoryUsage = getMemoryUsage();
- boolean pausePushData = memoryUsage > pausePushDataThreshold;
- boolean pauseReplicate = memoryUsage > pauseReplicateThreshold;
- boolean resume = memoryUsage < resumeThreshold;
- if (pausePushData || pauseReplicate) {
- underPressure = true;
- } else if (resume) {
- underPressure = false;
- }
- if (pausePushData && pauseReplicate) {
+ // trigger pause both push and replicate
+ if (memoryUsage > pauseReplicateThreshold) {
Review Comment:
Have same idea, maybe we should can this conf `celeborn.worker.directMemoryRatioToPauseReplicate` => `celeborn.worker.directMemoryRatioToPauseAll`?
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] zwangsheng commented on a diff in pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on code in PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#discussion_r1301269058
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -253,27 +258,24 @@ private MemoryManager(CelebornConf conf) {
public ServingState currentServingState() {
long memoryUsage = getMemoryUsage();
- boolean pausePushData = memoryUsage > pausePushDataThreshold;
- boolean pauseReplicate = memoryUsage > pauseReplicateThreshold;
- boolean resume = memoryUsage < resumeThreshold;
- if (pausePushData || pauseReplicate) {
- underPressure = true;
- } else if (resume) {
- underPressure = false;
- }
- if (pausePushData && pauseReplicate) {
+ // trigger pause both push and replicate
+ if (memoryUsage > pauseReplicateThreshold) {
Review Comment:
Have same idea, maybe we should call conf `celeborn.worker.directMemoryRatioToPauseReplicate` => `celeborn.worker.directMemoryRatioToPauseAll`?
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] zwangsheng commented on pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#issuecomment-1676850566
As sbt unit test fail, WIP @cfmcgrady
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] FMX commented on a diff in pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#discussion_r1296624487
##########
worker/src/test/scala/org/apache/celeborn/service/deploy/memory/MemoryManagerSuite.scala:
##########
@@ -42,4 +41,42 @@ class MemoryManagerSuite extends AnyFunSuite with BeforeAndAfterEach {
caught.getMessage == s"Invalid config, ${WORKER_DIRECT_MEMORY_RATIO_PAUSE_REPLICATE.key}(0.85) " +
s"should be greater than ${WORKER_DIRECT_MEMORY_RATIO_PAUSE_RECEIVE.key}(0.95)")
}
+
+ test("[CELEBORN-888] Test MemoryManager#currentServingState trigger case") {
Review Comment:
This test case looks good to 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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] waitinfuture commented on a diff in pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#discussion_r1301590217
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -253,27 +258,24 @@ private MemoryManager(CelebornConf conf) {
public ServingState currentServingState() {
long memoryUsage = getMemoryUsage();
- boolean pausePushData = memoryUsage > pausePushDataThreshold;
- boolean pauseReplicate = memoryUsage > pauseReplicateThreshold;
- boolean resume = memoryUsage < resumeThreshold;
- if (pausePushData || pauseReplicate) {
- underPressure = true;
- } else if (resume) {
- underPressure = false;
- }
- if (pausePushData && pauseReplicate) {
+ // trigger pause both push and replicate
+ if (memoryUsage > pauseReplicateThreshold) {
Review Comment:
IMO we can add comment describing the logic so that we don't need to change the config 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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] FMX commented on a diff in pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#discussion_r1301238863
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -253,27 +258,24 @@ private MemoryManager(CelebornConf conf) {
public ServingState currentServingState() {
long memoryUsage = getMemoryUsage();
- boolean pausePushData = memoryUsage > pausePushDataThreshold;
- boolean pauseReplicate = memoryUsage > pauseReplicateThreshold;
- boolean resume = memoryUsage < resumeThreshold;
- if (pausePushData || pauseReplicate) {
- underPressure = true;
- } else if (resume) {
- underPressure = false;
- }
- if (pausePushData && pauseReplicate) {
+ // trigger pause both push and replicate
+ if (memoryUsage > pauseReplicateThreshold) {
Review Comment:
Let's assume that a newbie comes and try to understand the method. Why should this method return PUSH_AND_REPLICATE_PAUSED when memory usage is above pauseReplicateThreshold? Will renaming pauseReplicateThreshold to pausePushAndReplicateThreshold be a better name for understanding?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] zwangsheng commented on pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#issuecomment-1689770995
> Could you please help by revising the state diagram docs/assets/img/backpressure.svg in line with this refactor? The current state diagram docs/assets/img/backpressure.svg is unclear.
Sure, i'd like to raise a pr to make those diagram clearly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] zwangsheng commented on a diff in pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on code in PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#discussion_r1293081901
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -139,51 +139,56 @@ private MemoryManager(CelebornConf conf) {
readBufferTarget = (long) (readBufferThreshold * readBufferTargetRatio);
memoryShuffleStorageThreshold = (long) (maxDirectorMemory * shuffleStorageRatio);
- checkService.scheduleWithFixedDelay(
- () -> {
- try {
- ServingState lastState = servingState;
- servingState = currentServingState();
- if (lastState != servingState) {
- logger.info("Serving state transformed from {} to {}", lastState, servingState);
- if (servingState == ServingState.PUSH_PAUSED) {
- pausePushDataCounter.increment();
- logger.info("Trigger action: PAUSE PUSH, RESUME REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onResume(TransportModuleConstants.REPLICATE_MODULE));
- trimAllListeners();
- } else if (servingState == ServingState.PUSH_AND_REPLICATE_PAUSED) {
- pausePushDataAndReplicateCounter.increment();
- logger.info("Trigger action: PAUSE PUSH and REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.REPLICATE_MODULE));
- trimAllListeners();
+ // disabled trim thread in test case
Review Comment:
During the unit test, we want to trigger the threshold for each state, in order to avoid this thread, reset the current service state.
For example, we set sortMemory to trigger PAUSE_PUSH state, we don't want this thread trim 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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] zwangsheng commented on pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#issuecomment-1676831813
> > Make this method logical ...
>
> Any details? What problem do you find in the current implementation?
The main thing is to add unit tests to ensure that this piece of logic is changed by mistake.
The modification of the method is to reduce judgments and avoid making unnecessary comparisons before the method returns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] FMX commented on a diff in pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#discussion_r1296625236
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -251,29 +256,30 @@ private MemoryManager(CelebornConf conf) {
Utils.bytesToString(memoryShuffleStorageThreshold));
}
+ /**
+ * Get current worker serving state. isPaused should be set true when trigger pause, and set false
+ * when trigger resume.
+ */
public ServingState currentServingState() {
Review Comment:
What if we removed the comments from this method? Will this method be easier to understand?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] cfmcgrady commented on pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#issuecomment-1689676341
@zwangsheng Could you please help by revising the state diagram `docs/assets/img/backpressure.svg` in line with this refactor? The current state diagram `docs/assets/img/backpressure.svg` is unclear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#discussion_r1293065029
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -139,51 +139,56 @@ private MemoryManager(CelebornConf conf) {
readBufferTarget = (long) (readBufferThreshold * readBufferTargetRatio);
memoryShuffleStorageThreshold = (long) (maxDirectorMemory * shuffleStorageRatio);
- checkService.scheduleWithFixedDelay(
- () -> {
- try {
- ServingState lastState = servingState;
- servingState = currentServingState();
- if (lastState != servingState) {
- logger.info("Serving state transformed from {} to {}", lastState, servingState);
- if (servingState == ServingState.PUSH_PAUSED) {
- pausePushDataCounter.increment();
- logger.info("Trigger action: PAUSE PUSH, RESUME REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onResume(TransportModuleConstants.REPLICATE_MODULE));
- trimAllListeners();
- } else if (servingState == ServingState.PUSH_AND_REPLICATE_PAUSED) {
- pausePushDataAndReplicateCounter.increment();
- logger.info("Trigger action: PAUSE PUSH and REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.REPLICATE_MODULE));
- trimAllListeners();
+ // disabled trim thread in test case
Review Comment:
this is hacky, and why?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#discussion_r1293106187
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -139,51 +139,56 @@ private MemoryManager(CelebornConf conf) {
readBufferTarget = (long) (readBufferThreshold * readBufferTargetRatio);
memoryShuffleStorageThreshold = (long) (maxDirectorMemory * shuffleStorageRatio);
- checkService.scheduleWithFixedDelay(
- () -> {
- try {
- ServingState lastState = servingState;
- servingState = currentServingState();
- if (lastState != servingState) {
- logger.info("Serving state transformed from {} to {}", lastState, servingState);
- if (servingState == ServingState.PUSH_PAUSED) {
- pausePushDataCounter.increment();
- logger.info("Trigger action: PAUSE PUSH, RESUME REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onResume(TransportModuleConstants.REPLICATE_MODULE));
- trimAllListeners();
- } else if (servingState == ServingState.PUSH_AND_REPLICATE_PAUSED) {
- pausePushDataAndReplicateCounter.increment();
- logger.info("Trigger action: PAUSE PUSH and REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.REPLICATE_MODULE));
- trimAllListeners();
+ // disabled trim thread in test case
Review Comment:
I don't think it makes much sense. Instead, `memoryPressureListeners` may be a good joint.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] zwangsheng commented on a diff in pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on code in PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#discussion_r1302382868
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -253,27 +258,24 @@ private MemoryManager(CelebornConf conf) {
public ServingState currentServingState() {
long memoryUsage = getMemoryUsage();
- boolean pausePushData = memoryUsage > pausePushDataThreshold;
- boolean pauseReplicate = memoryUsage > pauseReplicateThreshold;
- boolean resume = memoryUsage < resumeThreshold;
- if (pausePushData || pauseReplicate) {
- underPressure = true;
- } else if (resume) {
- underPressure = false;
- }
- if (pausePushData && pauseReplicate) {
+ // trigger pause both push and replicate
+ if (memoryUsage > pauseReplicateThreshold) {
Review Comment:
added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] codecov[bot] commented on pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#issuecomment-1687548439
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1811?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#1811](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1811?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (2c6f7e3) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/4f47d0a56b3d56fbdc4b9c93053231545228b9b4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (4f47d0a) will **not change** coverage.
> Report is 3 commits behind head on main.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #1811 +/- ##
=======================================
Coverage 46.43% 46.43%
=======================================
Files 163 163
Lines 10135 10135
Branches 934 934
=======================================
Hits 4705 4705
Misses 5118 5118
Partials 312 312
```
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] cfmcgrady commented on pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#issuecomment-1676868947
> As sbt unit test fail, WIP @cfmcgrady
It occurred randomly, and I'm still investigating. Let's temporarily disregard 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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] zwangsheng commented on a diff in pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on code in PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#discussion_r1302380149
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -139,51 +139,56 @@ private MemoryManager(CelebornConf conf) {
readBufferTarget = (long) (readBufferThreshold * readBufferTargetRatio);
memoryShuffleStorageThreshold = (long) (maxDirectorMemory * shuffleStorageRatio);
- checkService.scheduleWithFixedDelay(
- () -> {
- try {
- ServingState lastState = servingState;
- servingState = currentServingState();
- if (lastState != servingState) {
- logger.info("Serving state transformed from {} to {}", lastState, servingState);
- if (servingState == ServingState.PUSH_PAUSED) {
- pausePushDataCounter.increment();
- logger.info("Trigger action: PAUSE PUSH, RESUME REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onResume(TransportModuleConstants.REPLICATE_MODULE));
- trimAllListeners();
- } else if (servingState == ServingState.PUSH_AND_REPLICATE_PAUSED) {
- pausePushDataAndReplicateCounter.increment();
- logger.info("Trigger action: PAUSE PUSH and REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.REPLICATE_MODULE));
- trimAllListeners();
+ // disabled trim thread in test case
+ if (!Boolean.valueOf(conf.get("celeborn.testing", "false"))) {
Review Comment:
Never thought this! Thanks for reminding, i will make 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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] zwangsheng commented on pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#issuecomment-1679905998
> > As sbt unit test fail, WIP @cfmcgrady
>
> Fixed in #1818. cc @zwangsheng
Thanks, i'll rebase and retest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] FMX commented on a diff in pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#discussion_r1296620016
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -139,51 +139,56 @@ private MemoryManager(CelebornConf conf) {
readBufferTarget = (long) (readBufferThreshold * readBufferTargetRatio);
memoryShuffleStorageThreshold = (long) (maxDirectorMemory * shuffleStorageRatio);
- checkService.scheduleWithFixedDelay(
- () -> {
- try {
- ServingState lastState = servingState;
- servingState = currentServingState();
- if (lastState != servingState) {
- logger.info("Serving state transformed from {} to {}", lastState, servingState);
- if (servingState == ServingState.PUSH_PAUSED) {
- pausePushDataCounter.increment();
- logger.info("Trigger action: PAUSE PUSH, RESUME REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onResume(TransportModuleConstants.REPLICATE_MODULE));
- trimAllListeners();
- } else if (servingState == ServingState.PUSH_AND_REPLICATE_PAUSED) {
- pausePushDataAndReplicateCounter.increment();
- logger.info("Trigger action: PAUSE PUSH and REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.REPLICATE_MODULE));
- trimAllListeners();
+ // disabled trim thread in test case
Review Comment:
In the test scenario, memoryPressureListeners shall be empty. The trim won't do anything.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] zwangsheng commented on a diff in pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on code in PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#discussion_r1301054996
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -251,29 +256,30 @@ private MemoryManager(CelebornConf conf) {
Utils.bytesToString(memoryShuffleStorageThreshold));
}
+ /**
+ * Get current worker serving state. isPaused should be set true when trigger pause, and set false
+ * when trigger resume.
+ */
public ServingState currentServingState() {
Review Comment:
Sounds reasonable, done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] FMX commented on a diff in pull request #1811: [CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #1811:
URL: https://github.com/apache/incubator-celeborn/pull/1811#discussion_r1301232465
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -139,51 +139,56 @@ private MemoryManager(CelebornConf conf) {
readBufferTarget = (long) (readBufferThreshold * readBufferTargetRatio);
memoryShuffleStorageThreshold = (long) (maxDirectorMemory * shuffleStorageRatio);
- checkService.scheduleWithFixedDelay(
- () -> {
- try {
- ServingState lastState = servingState;
- servingState = currentServingState();
- if (lastState != servingState) {
- logger.info("Serving state transformed from {} to {}", lastState, servingState);
- if (servingState == ServingState.PUSH_PAUSED) {
- pausePushDataCounter.increment();
- logger.info("Trigger action: PAUSE PUSH, RESUME REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onResume(TransportModuleConstants.REPLICATE_MODULE));
- trimAllListeners();
- } else if (servingState == ServingState.PUSH_AND_REPLICATE_PAUSED) {
- pausePushDataAndReplicateCounter.increment();
- logger.info("Trigger action: PAUSE PUSH and REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.REPLICATE_MODULE));
- trimAllListeners();
+ // disabled trim thread in test case
Review Comment:
I see that in your test scenario the memory pressure listeners are empty so maybe this check is unnecessary?
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -139,51 +139,56 @@ private MemoryManager(CelebornConf conf) {
readBufferTarget = (long) (readBufferThreshold * readBufferTargetRatio);
memoryShuffleStorageThreshold = (long) (maxDirectorMemory * shuffleStorageRatio);
- checkService.scheduleWithFixedDelay(
- () -> {
- try {
- ServingState lastState = servingState;
- servingState = currentServingState();
- if (lastState != servingState) {
- logger.info("Serving state transformed from {} to {}", lastState, servingState);
- if (servingState == ServingState.PUSH_PAUSED) {
- pausePushDataCounter.increment();
- logger.info("Trigger action: PAUSE PUSH, RESUME REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onResume(TransportModuleConstants.REPLICATE_MODULE));
- trimAllListeners();
- } else if (servingState == ServingState.PUSH_AND_REPLICATE_PAUSED) {
- pausePushDataAndReplicateCounter.increment();
- logger.info("Trigger action: PAUSE PUSH and REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.REPLICATE_MODULE));
- trimAllListeners();
+ // disabled trim thread in test case
+ if (!Boolean.valueOf(conf.get("celeborn.testing", "false"))) {
Review Comment:
I see that in your test scenario the memory pressure listeners are empty so maybe this check is unnecessary?
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java:
##########
@@ -139,51 +139,56 @@ private MemoryManager(CelebornConf conf) {
readBufferTarget = (long) (readBufferThreshold * readBufferTargetRatio);
memoryShuffleStorageThreshold = (long) (maxDirectorMemory * shuffleStorageRatio);
- checkService.scheduleWithFixedDelay(
- () -> {
- try {
- ServingState lastState = servingState;
- servingState = currentServingState();
- if (lastState != servingState) {
- logger.info("Serving state transformed from {} to {}", lastState, servingState);
- if (servingState == ServingState.PUSH_PAUSED) {
- pausePushDataCounter.increment();
- logger.info("Trigger action: PAUSE PUSH, RESUME REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onResume(TransportModuleConstants.REPLICATE_MODULE));
- trimAllListeners();
- } else if (servingState == ServingState.PUSH_AND_REPLICATE_PAUSED) {
- pausePushDataAndReplicateCounter.increment();
- logger.info("Trigger action: PAUSE PUSH and REPLICATE");
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.PUSH_MODULE));
- memoryPressureListeners.forEach(
- memoryPressureListener ->
- memoryPressureListener.onPause(TransportModuleConstants.REPLICATE_MODULE));
- trimAllListeners();
+ // disabled trim thread in test case
Review Comment:
I see that in your test scenario the memory pressure listeners are empty so maybe this check is unnecessary?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org