You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "atris (via GitHub)" <gi...@apache.org> on 2023/06/05 06:03:23 UTC
[GitHub] [pinot] atris opened a new pull request, #10842: Support Off Heap for Native Text Indices
atris opened a new pull request, #10842:
URL: https://github.com/apache/pinot/pull/10842
This commit allows native text indices to be loaded off heap.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10842: Support Off Heap for Native Text Indices
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10842:
URL: https://github.com/apache/pinot/pull/10842#discussion_r1218435393
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/NativeTextIndexReader.java:
##########
@@ -52,7 +52,10 @@ public class NativeTextIndexReader implements TextIndexReader {
public NativeTextIndexReader(String column, File indexDir) {
_column = column;
try {
- _buffer = PinotDataBuffer.loadBigEndianFile(getTextIndexFile(indexDir));
+ String desc = "Native text index buffer: " + column;
Review Comment:
Both ways load the index off-heap. The difference is that in the original code it is loaded into direct memory, in the new code it is memory mapped. Ideally we should choose how to load the file based on the `ReadMode` (`heap` actually means direct memory), but since that cannot be easily passed to here, we may add a TODO here to address it later.
Looking at the code, I found the `_buffer` is not closed (not introduced in this PR), which can cause memory leak.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10842: Support Off Heap for Native Text Indices
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10842:
URL: https://github.com/apache/pinot/pull/10842#issuecomment-1576196265
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10842?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10842](https://app.codecov.io/gh/apache/pinot/pull/10842?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (67aaed4) into [master](https://app.codecov.io/gh/apache/pinot/commit/dc35068da1c253222de76027d8ca2c9458814dfd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (dc35068) will **decrease** coverage by `3.66%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10842 +/- ##
============================================
- Coverage 27.45% 23.79% -3.66%
Complexity 58 58
============================================
Files 2154 2154
Lines 116255 116258 +3
Branches 17606 17606
============================================
- Hits 31919 27669 -4250
- Misses 81175 85727 +4552
+ Partials 3161 2862 -299
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `23.79% <0.00%> (-0.10%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ment/index/readers/text/NativeTextIndexReader.java](https://app.codecov.io/gh/apache/pinot/pull/10842?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvdGV4dC9OYXRpdmVUZXh0SW5kZXhSZWFkZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
... and [265 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10842/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] atris commented on a diff in pull request #10842: Support Off Heap for Native Text Indices
Posted by "atris (via GitHub)" <gi...@apache.org>.
atris commented on code in PR #10842:
URL: https://github.com/apache/pinot/pull/10842#discussion_r1218520645
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/NativeTextIndexReader.java:
##########
@@ -52,7 +52,10 @@ public class NativeTextIndexReader implements TextIndexReader {
public NativeTextIndexReader(String column, File indexDir) {
_column = column;
try {
- _buffer = PinotDataBuffer.loadBigEndianFile(getTextIndexFile(indexDir));
+ String desc = "Native text index buffer: " + column;
Review Comment:
Fixed the issue and added a todo
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] atris merged pull request #10842: Support Off Heap for Native Text Indices
Posted by "atris (via GitHub)" <gi...@apache.org>.
atris merged PR #10842:
URL: https://github.com/apache/pinot/pull/10842
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org