You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "KKcorps (via GitHub)" <gi...@apache.org> on 2023/03/29 12:49:34 UTC
[GitHub] [pinot] KKcorps opened a new pull request, #10502: Use genericRow from the caller to update lookup table results
KKcorps opened a new pull request, #10502:
URL: https://github.com/apache/pinot/pull/10502
Currently we use `_reuse` object in the `MemoryOptimizedDimensionTable` class to update the results.
This however is not thread safe when `.get` function is called from multiple threads.
There are three possible solutions to this -
1. Use synchronized on the .get method which makes it slow
2. Create a new `GenericRow` object for every `.get` call which leads to higher memory usage
3. Pass the `GenericRow` object created in the `LookupTransfrom` function. The transform function should create one object per execution ensuring they are not shared.
I have gone with the 3rd approach here.
--
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 pull request #10502: Use genericRow from the caller to update lookup table results
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #10502:
URL: https://github.com/apache/pinot/pull/10502#issuecomment-1490724884
I prefer `ThreadLocal` since we don't really need the extra buffer in the `FastLookupDimensionTable`. This way the details can be hide within the 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: 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 merged pull request #10502: Use genericRow from the caller to update lookup table results
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10502:
URL: https://github.com/apache/pinot/pull/10502
--
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 #10502: Use genericRow from the caller to update lookup table results
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10502:
URL: https://github.com/apache/pinot/pull/10502#issuecomment-1488616946
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10502?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 [#10502](https://codecov.io/gh/apache/pinot/pull/10502?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (71c9323) into [master](https://codecov.io/gh/apache/pinot/commit/0c6c54e9e555d6defb1abc498314dd4e7c445bcd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0c6c54e) will **decrease** coverage by `56.26%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10502 +/- ##
=============================================
- Coverage 70.21% 13.96% -56.26%
+ Complexity 6116 237 -5879
=============================================
Files 2090 2036 -54
Lines 112207 109733 -2474
Branches 17078 16783 -295
=============================================
- Hits 78784 15321 -63463
- Misses 27857 93159 +65302
+ Partials 5566 1253 -4313
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.96% <0.00%> (-0.01%)` | :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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10502?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ata/manager/offline/DimensionTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/10502?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvb2ZmbGluZS9EaW1lbnNpb25UYWJsZURhdGFNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-53.78%)` | :arrow_down: |
| [...data/manager/offline/FastLookupDimensionTable.java](https://codecov.io/gh/apache/pinot/pull/10502?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvb2ZmbGluZS9GYXN0TG9va3VwRGltZW5zaW9uVGFibGUuamF2YQ==) | `0.00% <0.00%> (-90.00%)` | :arrow_down: |
| [...manager/offline/MemoryOptimizedDimensionTable.java](https://codecov.io/gh/apache/pinot/pull/10502?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvb2ZmbGluZS9NZW1vcnlPcHRpbWl6ZWREaW1lbnNpb25UYWJsZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...or/transform/function/LookupTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10502?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vTG9va3VwVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-76.89%)` | :arrow_down: |
... and [1611 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10502/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
: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=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: 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] saurabhd336 commented on pull request #10502: Use genericRow from the caller to update lookup table results
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on PR #10502:
URL: https://github.com/apache/pinot/pull/10502#issuecomment-1488931145
Would having a `ThreadLocal<GenericRow> _reuse` inside FastLookupDimensionTable.java be a better approach? Since we won't have to make interface 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: 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