You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by GitBox <gi...@apache.org> on 2022/02/15 02:55:25 UTC
[GitHub] [rocketmq] Erik1288 opened a new pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Erik1288 opened a new pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848
…ge disks.
**Make sure set the target branch to `develop`**
## What is the purpose of the change
Swapping mapped-byte-buffer to save memory on a system with ultra-large disks(more than 10TB).
## Brief changelog
Periodically clean up the mmap of "cold data".
## Verifying this change
pass ut
Follow this checklist to help us incorporate your contribution quickly and easily. Notice, `it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR`.
- [x] Make sure there is a [Github issue](https://github.com/apache/rocketmq/issues) filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
- [x] Format the pull request title like `[ISSUE #123] Fix UnknownException when host config not exist`. Each commit in the pull request should have a meaningful subject line and body.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in [test module](https://github.com/apache/rocketmq/tree/master/test).
- [x] Run `mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle` to make sure basic checks pass. Run `mvn clean install -DskipITs` to make sure unit-test pass. Run `mvn clean test-compile failsafe:integration-test` to make sure integration-test pass.
- [ ] If this contribution is large, please file an [Apache Individual Contributor License Agreement](http://www.apache.org/licenses/#clas).
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] coveralls commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1039840402
[](https://coveralls.io/builds/46545129)
Coverage increased (+0.2%) to 52.798% when pulling **d064ddd3650efe41f9736435b8aec8695419546a on Erik1288:5.0.0-alpha-swapping** into **de76e06d6da8e8646d2ac59ea061e8dc2653aae8 on apache:5.0.0-alpha**.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] coveralls commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1039840402
[](https://coveralls.io/builds/46545129)
Coverage increased (+0.2%) to 52.798% when pulling **d064ddd3650efe41f9736435b8aec8695419546a on Erik1288:5.0.0-alpha-swapping** into **de76e06d6da8e8646d2ac59ea061e8dc2653aae8 on apache:5.0.0-alpha**.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever edited a comment on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
dongeforever edited a comment on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1040050655
It seems to have a concurrent problem.
If the reader thread gets the mapped byte buffer and then the cleaner thread cleans it later. It will cause the jvm crash.
Usually, the reader thread uses the SelectedByteBuffer, which means the data maybe be read later on.
The cleanswap method should be called after the swap method for a long time enough, and also it should check the getRefCount() == 1.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhouxinyu edited a comment on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
zhouxinyu edited a comment on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1039825743
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] vongosling commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
vongosling commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1085332263
Any update?
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] codecov-commenter commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1039840487
# [Codecov](https://codecov.io/gh/apache/rocketmq/pull/3848?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 [#3848](https://codecov.io/gh/apache/rocketmq/pull/3848?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d064ddd) into [5.0.0-alpha](https://codecov.io/gh/apache/rocketmq/commit/de76e06d6da8e8646d2ac59ea061e8dc2653aae8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (de76e06) will **increase** coverage by `0.10%`.
> The diff coverage is `85.18%`.
[](https://codecov.io/gh/apache/rocketmq/pull/3848?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## 5.0.0-alpha #3848 +/- ##
=================================================
+ Coverage 46.50% 46.61% +0.10%
- Complexity 5330 5350 +20
=================================================
Files 641 641
Lines 44348 44375 +27
Branches 6117 6121 +4
=================================================
+ Hits 20624 20684 +60
+ Misses 20998 20960 -38
- Partials 2726 2731 +5
```
| [Impacted Files](https://codecov.io/gh/apache/rocketmq/pull/3848?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...org/apache/rocketmq/store/DefaultMessageStore.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL0RlZmF1bHRNZXNzYWdlU3RvcmUuamF2YQ==) | `58.27% <85.18%> (+0.49%)` | :arrow_up: |
| [...rocketmq/broker/filtersrv/FilterServerManager.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvZmlsdGVyc3J2L0ZpbHRlclNlcnZlck1hbmFnZXIuamF2YQ==) | `20.00% <0.00%> (-14.29%)` | :arrow_down: |
| [...che/rocketmq/acl/plain/PlainPermissionManager.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-YWNsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9hY2wvcGxhaW4vUGxhaW5QZXJtaXNzaW9uTWFuYWdlci5qYXZh) | `70.53% <0.00%> (-1.34%)` | :arrow_down: |
| [...a/org/apache/rocketmq/store/StoreStatsService.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL1N0b3JlU3RhdHNTZXJ2aWNlLmphdmE=) | `40.65% <0.00%> (-0.66%)` | :arrow_down: |
| [...main/java/org/apache/rocketmq/store/CommitLog.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL0NvbW1pdExvZy5qYXZh) | `67.95% <0.00%> (+0.49%)` | :arrow_up: |
| [...cketmq/broker/processor/PopBufferMergeService.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvcHJvY2Vzc29yL1BvcEJ1ZmZlck1lcmdlU2VydmljZS5qYXZh) | `45.89% <0.00%> (+0.76%)` | :arrow_up: |
| [...he/rocketmq/client/trace/AsyncTraceDispatcher.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvdHJhY2UvQXN5bmNUcmFjZURpc3BhdGNoZXIuamF2YQ==) | `78.71% <0.00%> (+0.99%)` | :arrow_up: |
| [...va/org/apache/rocketmq/store/util/PerfCounter.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL3V0aWwvUGVyZkNvdW50ZXIuamF2YQ==) | `65.96% <0.00%> (+1.04%)` | :arrow_up: |
| [...n/java/org/apache/rocketmq/store/ConsumeQueue.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL0NvbnN1bWVRdWV1ZS5qYXZh) | `67.20% <0.00%> (+1.06%)` | :arrow_up: |
| ... and [6 more](https://codecov.io/gh/apache/rocketmq/pull/3848/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/rocketmq/pull/3848?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/rocketmq/pull/3848?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [de76e06...d064ddd](https://codecov.io/gh/apache/rocketmq/pull/3848?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] coveralls edited a comment on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1039840402
[](https://coveralls.io/builds/46669060)
Coverage increased (+0.06%) to 52.674% when pulling **26beaa250ec9049857364514cb33d599f336e170 on Erik1288:5.0.0-alpha-swapping** into **de76e06d6da8e8646d2ac59ea061e8dc2653aae8 on apache:5.0.0-alpha**.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
dongeforever commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1040050655
>
It seems to have a concurrent problem.
If the reader thread gets the mapped byte buffer and then the cleaner thread cleans it later. It will cause the jvm crash.
Usually, the reader thread uses the SelectedByteBuffer, which means the data maybe be read later on.
The cleanswap method should be called after the swap method for a long time enough, and also it should check the getRefCount() == 1.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
dongeforever commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1040035317
It will reserve 3 files, which will only swap the readable file for an append-only system. So it is safe in practice.
As for the technical discussion, the mapped file is implemented with page cache internally by os. So it is safe to write with two mappedbytebuffer as long as the write pos is not intersected.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhouxinyu edited a comment on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
zhouxinyu edited a comment on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1040031406
> @zhouxinyu It may have difficulty doing the lazy mapping safely while the mapped file is being used concurrently, which may need to introduce a lock.
>
> The current code is clear, avoiding the anxiety of concurrent error, and is used for a long time in the production env.
>
> BTW, if you have a simple way to implement the lazy mapping, it is ok to show it in detail.
Yeap, a lock is necessary for this lazy mapping solution, implementing it is not too complex, we convert direct access `this.mappedByteBuffer` to a getter method `this.getBuffer`:
```
public MappedByteBuffer getBuffer() {
if (this.mappedByteBuffer != null) {
return this.mappedByteBuffer;
}
synchronized (this) {
if (this.mappedByteBuffer == null) {
this.mappedByteBuffer = this.fileChannel.map();
}
}
return this.mappedByteBuffer;
}
```
Of course, more corner cases need to be considered.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhouxinyu edited a comment on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
zhouxinyu edited a comment on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1039825743
Another technical suggestion:
1. How about only providing the `unmap` method for MappedFile, and adding the lazy-map ability for MappedFile? If so, we don't need two timed tasks to coordinate the swap and clean operations.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhouxinyu commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
zhouxinyu commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1039798492
More suggested comments:
1. Add some comments for interface Swappable, it's difficult to understand the specific meaning of `swapMap` and `cleanSwappedMap` directly.
2. Add unit tests for this PR, and the implementations of `Swappable`.
3. Maybe only MappedFile and MappedFileQueue implement `Swappable` is enough, it's a little strange that CommitLog and ConsumeQueueStore also is a subclass of this interface.
And a technical question:
1. We mapped twice for writing when the old buffer hasn't been cleaned, is there a write conflict problem in this situation? Although the cleaner won't clean the last writing file, the file doesn't know 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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhouxinyu commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
zhouxinyu commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1039798492
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
dongeforever commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1040058432
@Erik1288 I found other problems.
* getRecentSwapMapTime is fixed to zero, but in fact, it should return swapMapTime
* cleanSwapedMap, shoud check getRefCount() == 1 and it dose not need to sleep but just return.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhouxinyu commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
zhouxinyu commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1040044030
> and is used for a long time in the production env.
If this swap mechanism is a mature production solution, take it easy.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] Erik1288 commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
Erik1288 commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1041034063
> @Erik1288 I found other problems.
>
> * getRecentSwapMapTime is fixed to zero, but in fact, it should return swapMapTime
> * cleanSwapedMap, shoud check getRefCount() <= 1 and it dose not need to sleep but just return.
Thanks for pointing it out, I'm working on 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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] codecov-commenter commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1039840487
# [Codecov](https://codecov.io/gh/apache/rocketmq/pull/3848?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 [#3848](https://codecov.io/gh/apache/rocketmq/pull/3848?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d064ddd) into [5.0.0-alpha](https://codecov.io/gh/apache/rocketmq/commit/de76e06d6da8e8646d2ac59ea061e8dc2653aae8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (de76e06) will **increase** coverage by `0.10%`.
> The diff coverage is `85.18%`.
[](https://codecov.io/gh/apache/rocketmq/pull/3848?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## 5.0.0-alpha #3848 +/- ##
=================================================
+ Coverage 46.50% 46.61% +0.10%
- Complexity 5330 5350 +20
=================================================
Files 641 641
Lines 44348 44375 +27
Branches 6117 6121 +4
=================================================
+ Hits 20624 20684 +60
+ Misses 20998 20960 -38
- Partials 2726 2731 +5
```
| [Impacted Files](https://codecov.io/gh/apache/rocketmq/pull/3848?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...org/apache/rocketmq/store/DefaultMessageStore.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL0RlZmF1bHRNZXNzYWdlU3RvcmUuamF2YQ==) | `58.27% <85.18%> (+0.49%)` | :arrow_up: |
| [...rocketmq/broker/filtersrv/FilterServerManager.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvZmlsdGVyc3J2L0ZpbHRlclNlcnZlck1hbmFnZXIuamF2YQ==) | `20.00% <0.00%> (-14.29%)` | :arrow_down: |
| [...che/rocketmq/acl/plain/PlainPermissionManager.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-YWNsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9hY2wvcGxhaW4vUGxhaW5QZXJtaXNzaW9uTWFuYWdlci5qYXZh) | `70.53% <0.00%> (-1.34%)` | :arrow_down: |
| [...a/org/apache/rocketmq/store/StoreStatsService.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL1N0b3JlU3RhdHNTZXJ2aWNlLmphdmE=) | `40.65% <0.00%> (-0.66%)` | :arrow_down: |
| [...main/java/org/apache/rocketmq/store/CommitLog.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL0NvbW1pdExvZy5qYXZh) | `67.95% <0.00%> (+0.49%)` | :arrow_up: |
| [...cketmq/broker/processor/PopBufferMergeService.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvcHJvY2Vzc29yL1BvcEJ1ZmZlck1lcmdlU2VydmljZS5qYXZh) | `45.89% <0.00%> (+0.76%)` | :arrow_up: |
| [...he/rocketmq/client/trace/AsyncTraceDispatcher.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvdHJhY2UvQXN5bmNUcmFjZURpc3BhdGNoZXIuamF2YQ==) | `78.71% <0.00%> (+0.99%)` | :arrow_up: |
| [...va/org/apache/rocketmq/store/util/PerfCounter.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL3V0aWwvUGVyZkNvdW50ZXIuamF2YQ==) | `65.96% <0.00%> (+1.04%)` | :arrow_up: |
| [...n/java/org/apache/rocketmq/store/ConsumeQueue.java](https://codecov.io/gh/apache/rocketmq/pull/3848/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL0NvbnN1bWVRdWV1ZS5qYXZh) | `67.20% <0.00%> (+1.06%)` | :arrow_up: |
| ... and [6 more](https://codecov.io/gh/apache/rocketmq/pull/3848/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/rocketmq/pull/3848?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/rocketmq/pull/3848?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [de76e06...d064ddd](https://codecov.io/gh/apache/rocketmq/pull/3848?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhouxinyu commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
zhouxinyu commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1039825743
Another technical suggestion:
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
dongeforever commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1040029990
@Erik1288 Why do the commitlog and consumequeuestore need to implement the swappable?
It seems a little strange indeed.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] Erik1288 commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
Erik1288 commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1041033566
Thanks for your comments.
> More suggested comments:
>
> 1. Add some comments for interface Swappable, it's difficult to understand the specific meaning of `swapMap` and `cleanSwappedMap` directly.
Re: Sure, more meaningful comments will be added.
> 2. Add unit tests for this PR, and the implementations of `Swappable`.
Re: Unit-test and Implementation were pushed but not be called.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhouxinyu commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
zhouxinyu commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1040031406
> @zhouxinyu It may have difficulty doing the lazy mapping safely while the mapped file is being used concurrently, which may need to introduce a lock.
>
> The current code is clear, avoiding the anxiety of concurrent error, and is used for a long time in the production env.
>
> BTW, if you have a simple way to implement the lazy mapping, it is ok to show it in detail.
Yeap, a lock is necessary for this lazy mapping solution, implementing it is not too complex, we convert direct access `this.mappedByteBuffer` to a getter method `this.getBuffer`:
```
public MappedByteBuffer getBuffer() {
if (this.mappedByteBuffer != null) {
return this.mappedByteBuffer;
}
synchronized (this) {
if (this.mappedByteBuffer == null) {
this.mappedByteBuffer = this.fileChannel.map();
}
}
return this.mappedByteBuffer;
}
```
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
dongeforever commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1040076720
@zhouxinyu It come into another design consideration. The current swap mechnism will make sure the existed message can be read correctly.
Otherwise, it may get null temporately, which may cause message loss.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever edited a comment on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
dongeforever edited a comment on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1040050655
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] zhouxinyu commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
zhouxinyu commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1040055936
>
Yes, `unmap` should be protected by `RefCount`
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
dongeforever commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1040015139
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever commented on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
dongeforever commented on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1040015139
@zhouxinyu It may have difficulty doing the lazy mapping safely while the mapped file is being used concurrently, which may need to introduce a lock.
The current code is clear, avoiding the anxiety of concurrent error, and is used for a long time in the production env.
BTW, if you have a simple way to implement the lazy mapping, it is ok to show it in detail.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] dongeforever edited a comment on pull request #3848: [ISSUE #3831] Swapping mapped-byte-buffer to save memory on a system with extra-large disks.
Posted by GitBox <gi...@apache.org>.
dongeforever edited a comment on pull request #3848:
URL: https://github.com/apache/rocketmq/pull/3848#issuecomment-1040058432
@Erik1288 I found other problems.
* getRecentSwapMapTime is fixed to zero, but in fact, it should return swapMapTime
* cleanSwapedMap, shoud check getRefCount() <= 1 and it dose not need to sleep but just return.
--
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: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org