You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/03/31 03:54:37 UTC
[GitHub] [pinot] jtao15 opened a new pull request #8422: Change TableDataManger deletion to message based
jtao15 opened a new pull request #8422:
URL: https://github.com/apache/pinot/pull/8422
This pr improves the TableDataManager thread safety issue (#8423) by ~~ ~~update `segmentDataManagerMap` with a place holder (dummy segmentDataManager) before downloading the segment, so `getNumSegment()` can give the correct segment count.~~ ~~
1. Remove the tableDataManager deletion mechanism when the number of segments drops to 0 to avoid race conditions.
2. Add TableDeletionMessage, the tableDataManager can only be removed upon receiving the TableDeletionMessage when the table is dropped.
--
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 edited a comment on pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#issuecomment-1081038136
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8422?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 [#8422](https://codecov.io/gh/apache/pinot/pull/8422?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e8127aa) into [master](https://codecov.io/gh/apache/pinot/commit/5b44c22b46824101381c71e712716e97dd960657?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5b44c22) will **increase** coverage by `0.00%`.
> The diff coverage is `92.18%`.
```diff
@@ Coverage Diff @@
## master #8422 +/- ##
=========================================
Coverage 70.84% 70.85%
+ Complexity 4278 4218 -60
=========================================
Files 1660 1661 +1
Lines 87035 87093 +58
Branches 13139 13142 +3
=========================================
+ Hits 61663 61712 +49
- Misses 21112 21125 +13
+ Partials 4260 4256 -4
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `28.69% <87.50%> (+0.02%)` | :arrow_up: |
| integration2 | `27.42% <84.37%> (+0.04%)` | :arrow_up: |
| unittests1 | `66.99% <10.00%> (-0.01%)` | :arrow_down: |
| unittests2 | `14.22% <64.70%> (+0.07%)` | :arrow_up: |
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/8422?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...server/starter/helix/HelixInstanceDataManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9IZWxpeEluc3RhbmNlRGF0YU1hbmFnZXIuamF2YQ==) | `78.64% <75.00%> (-0.80%)` | :arrow_down: |
| [...er/starter/helix/SegmentMessageHandlerFactory.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9TZWdtZW50TWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `80.00% <92.30%> (+6.31%)` | :arrow_up: |
| [...he/pinot/common/messages/TableDeletionMessage.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWVzc2FnZXMvVGFibGVEZWxldGlvbk1lc3NhZ2UuamF2YQ==) | `100.00% <100.00%> (ø)` | |
| [...a/org/apache/pinot/common/metrics/ServerMeter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJNZXRlci5qYXZh) | `100.00% <100.00%> (ø)` | |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `67.96% <100.00%> (+0.66%)` | :arrow_up: |
| [.../pinot/server/starter/helix/BaseServerStarter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | `57.46% <100.00%> (+0.12%)` | :arrow_up: |
| [...ontroller/api/resources/ServerTableSizeReader.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1NlcnZlclRhYmxlU2l6ZVJlYWRlci5qYXZh) | `83.33% <0.00%> (-16.67%)` | :arrow_down: |
| [...r/helix/SegmentOnlineOfflineStateModelFactory.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9TZWdtZW50T25saW5lT2ZmbGluZVN0YXRlTW9kZWxGYWN0b3J5LmphdmE=) | `58.49% <0.00%> (-4.72%)` | :arrow_down: |
| [...nction/DistinctCountBitmapAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9EaXN0aW5jdENvdW50Qml0bWFwQWdncmVnYXRpb25GdW5jdGlvbi5qYXZh) | `49.74% <0.00%> (-4.15%)` | :arrow_down: |
| [...core/startree/operator/StarTreeFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9vcGVyYXRvci9TdGFyVHJlZUZpbHRlck9wZXJhdG9yLmphdmE=) | `83.91% <0.00%> (-3.50%)` | :arrow_down: |
| ... and [23 more](https://codecov.io/gh/apache/pinot/pull/8422/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/pinot/pull/8422?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/pinot/pull/8422?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 [5b44c22...e8127aa](https://codecov.io/gh/apache/pinot/pull/8422?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: 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] jtao15 commented on pull request #8422: Fix the race condition that TableDataManager is removed during segment download because of incorrect segment count
Posted by GitBox <gi...@apache.org>.
jtao15 commented on pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#issuecomment-1081206855
> I feel this is not the correct fix. We should change the `computeIfAbsent()` to `compute()` to ensure the `addSegment()` is called within the `compute()` block.
This will make the addSegment() to be blocking? This may not be ideal because it will slow down the segment download on servers. Is it possible to do this async? We are considering two approaches:
1. The current approach in this pr which tries to increase the segment count before `addSegment()`, and decrease the count if `addSegment()` fails in the middle.
2. Another approach is to shutdown the tableDataManager in the background thread only if it's not active for a while. We can add `lastModifiedTime` whenever segmentDataManagerMap is updated and check both # of segments and `lastModifiedTime` to decide if shutdown is needed.
--
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 edited a comment on pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#issuecomment-1081038136
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8422?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 [#8422](https://codecov.io/gh/apache/pinot/pull/8422?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e8127aa) into [master](https://codecov.io/gh/apache/pinot/commit/5b44c22b46824101381c71e712716e97dd960657?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5b44c22) will **decrease** coverage by `1.03%`.
> The diff coverage is `92.18%`.
```diff
@@ Coverage Diff @@
## master #8422 +/- ##
============================================
- Coverage 70.84% 69.81% -1.04%
+ Complexity 4278 4218 -60
============================================
Files 1660 1661 +1
Lines 87035 87093 +58
Branches 13139 13142 +3
============================================
- Hits 61663 60805 -858
- Misses 21112 22047 +935
+ Partials 4260 4241 -19
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `28.69% <87.50%> (+0.02%)` | :arrow_up: |
| integration2 | `?` | |
| unittests1 | `66.99% <10.00%> (-0.01%)` | :arrow_down: |
| unittests2 | `14.22% <64.70%> (+0.07%)` | :arrow_up: |
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/8422?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...server/starter/helix/HelixInstanceDataManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9IZWxpeEluc3RhbmNlRGF0YU1hbmFnZXIuamF2YQ==) | `78.12% <75.00%> (-1.32%)` | :arrow_down: |
| [...er/starter/helix/SegmentMessageHandlerFactory.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9TZWdtZW50TWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `80.00% <92.30%> (+6.31%)` | :arrow_up: |
| [...he/pinot/common/messages/TableDeletionMessage.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWVzc2FnZXMvVGFibGVEZWxldGlvbk1lc3NhZ2UuamF2YQ==) | `100.00% <100.00%> (ø)` | |
| [...a/org/apache/pinot/common/metrics/ServerMeter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJNZXRlci5qYXZh) | `100.00% <100.00%> (ø)` | |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `67.76% <100.00%> (+0.46%)` | :arrow_up: |
| [.../pinot/server/starter/helix/BaseServerStarter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | `51.54% <100.00%> (-5.80%)` | :arrow_down: |
| [...apache/pinot/common/helix/ExtraInstanceConfig.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vaGVsaXgvRXh0cmFJbnN0YW5jZUNvbmZpZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...t/core/plan/StreamingInstanceResponsePlanNode.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL1N0cmVhbWluZ0luc3RhbmNlUmVzcG9uc2VQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ore/operator/streaming/StreamingResponseUtils.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nUmVzcG9uc2VVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ager/realtime/PeerSchemeSplitSegmentCommitter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUGVlclNjaGVtZVNwbGl0U2VnbWVudENvbW1pdHRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [103 more](https://codecov.io/gh/apache/pinot/pull/8422/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/pinot/pull/8422?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/pinot/pull/8422?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 [5b44c22...e8127aa](https://codecov.io/gh/apache/pinot/pull/8422?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: 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 edited a comment on pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#issuecomment-1081038136
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8422?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 [#8422](https://codecov.io/gh/apache/pinot/pull/8422?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6889dc2) into [master](https://codecov.io/gh/apache/pinot/commit/e2053f6776911dcce5f1ef1e32ed35063ca10bea?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e2053f6) will **decrease** coverage by `56.54%`.
> The diff coverage is `64.70%`.
```diff
@@ Coverage Diff @@
## master #8422 +/- ##
=============================================
- Coverage 70.73% 14.18% -56.55%
+ Complexity 4282 84 -4198
=============================================
Files 1662 1618 -44
Lines 87227 85378 -1849
Branches 13205 13004 -201
=============================================
- Hits 61703 12115 -49588
- Misses 21238 72348 +51110
+ Partials 4286 915 -3371
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.18% <64.70%> (+0.05%)` | :arrow_up: |
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/8422?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/pinot/common/messages/TableDeletionMessage.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWVzc2FnZXMvVGFibGVEZWxldGlvbk1lc3NhZ2UuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...a/org/apache/pinot/common/metrics/ServerMeter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJNZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `64.65% <91.66%> (-2.65%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1321 more](https://codecov.io/gh/apache/pinot/pull/8422/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/pinot/pull/8422?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/pinot/pull/8422?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 [e2053f6...6889dc2](https://codecov.io/gh/apache/pinot/pull/8422?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: 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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839056385
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SegmentMessageHandlerFactory.java
##########
@@ -144,6 +147,28 @@ public HelixTaskResult handleMessage()
}
}
+ private class TableDeletionMessageHandler extends DefaultMessageHandler {
+ TableDeletionMessageHandler(TableDeletionMessage tableDeletionMessage, ServerMetrics metrics,
+ NotificationContext context) {
+ super(tableDeletionMessage, metrics, context);
+ }
+
+ @Override
+ public HelixTaskResult handleMessage()
+ throws InterruptedException {
+ HelixTaskResult helixTaskResult = new HelixTaskResult();
+ _logger.info("Handling message: {}", _message);
+ try {
+ _instanceDataManager.deleteTable(_tableNameWithType);
+ helixTaskResult.setSuccess(true);
+ } catch (Exception e) {
+ _metrics.addMeteredTableValue(_tableNameWithType, ServerMeter.DELETE_TABLE_FAILURES, 1);
+ Utils.rethrowException(e);
Review comment:
Yes, it will print something like `ERROR [HelixTask] [HelixTaskExecutor-message_handle_thread] [pinot-server] [] Exception while executing a message...`
--
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] snleee commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
snleee commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r838064064
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java
##########
@@ -184,18 +184,26 @@ private TableDataManager createTableDataManager(String tableNameWithType, TableC
return tableDataManager;
}
+ @Override
+ public void deleteTable(String tableNameWithType) {
+ _tableDataManagerMap.compute(tableNameWithType, (k, v) -> {
+ if (v != null) {
+ v.shutDown();
Review comment:
What happens to the physical segment files on the server after we call `shutdown()`?
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SegmentMessageHandlerFactory.java
##########
@@ -144,6 +147,23 @@ public HelixTaskResult handleMessage()
}
}
+ private class TableDeletionMessageHandler extends DefaultMessageHandler {
+ TableDeletionMessageHandler(TableDeletionMessage tableDeletionMessage, ServerMetrics metrics,
+ NotificationContext context) {
+ super(tableDeletionMessage, metrics, context);
+ }
+
+ @Override
+ public HelixTaskResult handleMessage()
+ throws InterruptedException {
+ HelixTaskResult helixTaskResult = new HelixTaskResult();
+ _logger.info("Handling message: {}", _message);
+ _instanceDataManager.deleteTable(_tableNameWithType);
Review comment:
Can we emit the metrics on delete table failure? I think that we should at least emit the metric to indicate failure to cover the following case:
- we send the message to the server to drop table A from this line
- the server-side somehow fails to execute this message (but this doesn't get caught on the controller side because the message gets executed asynchronously)
- we remove other resources for this table and finish the table deletion.
- recreate table A
If the above case happens, the server will probably reuse the table data manager from the old table.
--
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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839055818
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1800,6 +1805,10 @@ public void deleteRealtimeTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager, realtimeTableName);
LOGGER.info("Deleting table {}: Removed from broker resource", realtimeTableName);
+ // Drop the table on servers
+ deleteTableOnServer(realtimeTableName);
+ LOGGER.info("Deleting table {}: Sent table deletion message to servers", realtimeTableName);
Review comment:
Ah, make sense. Removed this line.
--
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] snleee commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
snleee commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r838865953
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -408,7 +413,41 @@ public void testUploadSegmentRefreshOnly()
assertEquals(segmentsZKMetadata.size(), 1);
assertNotEquals(segmentsZKMetadata.get(0).getRefreshTime(), Long.MIN_VALUE);
}
+ waitForNumOfSegmentsBecomeOnline(offlineTableName, 1);
Review comment:
What happens if we remove this?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerMeter.java
##########
@@ -33,6 +33,7 @@
QUERY_EXECUTION_EXCEPTIONS("exceptions", false),
HELIX_ZOOKEEPER_RECONNECTS("reconnects", true),
DELETED_SEGMENT_COUNT("segments", false),
+ DELETE_TABLE_FAILURES("tables", true),
Review comment:
are we emitting the metrics per table? In that case, does this needs to be `false`? For instance, refresh failure sets this to be `false`. Can you double-check on this?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1757,6 +1758,10 @@ public void deleteOfflineTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager, offlineTableName);
LOGGER.info("Deleting table {}: Removed from broker resource", offlineTableName);
+ // Drop the table on servers
+ deleteTableOnServer(offlineTableName);
Review comment:
I see that we don't send the message if `externalview` is null from `deleteTableOnServer()`. If we send without filtering, the helix side will throw the exception? Or, you mention that we explicitly throw the exception?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -3285,10 +3324,11 @@ private void waitForSegmentsBecomeOnline(String tableNameWithType, Set<String> s
tableNameWithType, segmentsToCheck));
}
- private Set<String> getOnlineSegmentsFromExternalView(String tableNameWithType) {
+ public Set<String> getOnlineSegmentsFromExternalView(String tableNameWithType) {
ExternalView externalView = getTableExternalView(tableNameWithType);
- Preconditions
- .checkState(externalView != null, String.format("External view is null for table (%s)", tableNameWithType));
+ if (externalView == null) {
Review comment:
Have you checked all the usage of `getOnlineSegmentsFromExternalView`? This will change the behavior (returning null instead of throwing the exception).
--
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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839086828
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerMeter.java
##########
@@ -33,6 +33,7 @@
QUERY_EXECUTION_EXCEPTIONS("exceptions", false),
HELIX_ZOOKEEPER_RECONNECTS("reconnects", true),
DELETED_SEGMENT_COUNT("segments", false),
+ DELETE_TABLE_FAILURES("tables", true),
Review comment:
Updated to `false`
--
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] snleee commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
snleee commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r838863260
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1757,6 +1758,10 @@ public void deleteOfflineTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager, offlineTableName);
LOGGER.info("Deleting table {}: Removed from broker resource", offlineTableName);
+ // Drop the table on servers
+ deleteTableOnServer(offlineTableName);
Review comment:
I see that we don't send the message if `externalview` is null from `deleteTableOnServer()`. If we send without filtering, the helix side will throw the exception? Or, you mention that we explicitly throw the exception?
I think that one main difference between waiting on the controller vs server is that we will not delete table resource (which is a safety check for avoiding race confition in table re-create) if we wait for externalview to converge on the controller side.
If we offload the waiting to happen on the serverside, we now have another race condition when the external view gets updated with some significant delay. In that case, we will delete the table config on the following code block but the server side may not have done deleting and waiting for the external view to converge. If the new table creation request comes in, we will have some issue.
```
// Remove table config
// this should always be the last step for deletion to avoid race condition in table re-create.
ZKMetadataProvider.removeResourceConfigFromPropertyStore(_propertyStore, offlineTableName);
```
--
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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r838957733
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -2053,6 +2062,36 @@ public boolean updateZkMetadata(String tableNameWithType, SegmentZKMetadata segm
return ZKMetadataProvider.setSegmentZKMetadata(_propertyStore, tableNameWithType, segmentZKMetadata);
}
+ /**
+ * Delete the table on servers
+ */
+ private void deleteTableOnServer(String tableNameWithType) {
+ LOGGER.info("Sending delete message for table: {}", tableNameWithType);
+
+ Criteria recipientCriteria = new Criteria();
Review comment:
Added in the comment for the function
--
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 edited a comment on pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#issuecomment-1081038136
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8422?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 [#8422](https://codecov.io/gh/apache/pinot/pull/8422?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3e68dec) into [master](https://codecov.io/gh/apache/pinot/commit/e2053f6776911dcce5f1ef1e32ed35063ca10bea?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e2053f6) will **decrease** coverage by `56.59%`.
> The diff coverage is `62.50%`.
> :exclamation: Current head 3e68dec differs from pull request most recent head 254dd3a. Consider uploading reports for the commit 254dd3a to get more accurate results
```diff
@@ Coverage Diff @@
## master #8422 +/- ##
=============================================
- Coverage 70.73% 14.13% -56.60%
+ Complexity 4282 84 -4198
=============================================
Files 1662 1618 -44
Lines 87227 85376 -1851
Branches 13205 13004 -201
=============================================
- Hits 61703 12072 -49631
- Misses 21238 72390 +51152
+ Partials 4286 914 -3372
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.13% <62.50%> (+<0.01%)` | :arrow_up: |
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/8422?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/pinot/common/messages/TableDeletionMessage.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWVzc2FnZXMvVGFibGVEZWxldGlvbk1lc3NhZ2UuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...a/org/apache/pinot/common/metrics/ServerMeter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJNZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `64.28% <90.90%> (-3.02%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1323 more](https://codecov.io/gh/apache/pinot/pull/8422/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/pinot/pull/8422?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/pinot/pull/8422?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 [e2053f6...254dd3a](https://codecov.io/gh/apache/pinot/pull/8422?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: 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] snleee merged pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
snleee merged pull request #8422:
URL: https://github.com/apache/pinot/pull/8422
--
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] jackjlli commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839007738
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -2053,6 +2062,35 @@ public boolean updateZkMetadata(String tableNameWithType, SegmentZKMetadata segm
return ZKMetadataProvider.setSegmentZKMetadata(_propertyStore, tableNameWithType, segmentZKMetadata);
}
+ /**
+ * Delete the table on servers by sending table deletion message
+ */
+ private void deleteTableOnServer(String tableNameWithType) {
+ LOGGER.info("Sending delete table message for table: {}", tableNameWithType);
+ Criteria recipientCriteria = new Criteria();
+ recipientCriteria.setRecipientInstanceType(InstanceType.PARTICIPANT);
+ recipientCriteria.setInstanceName("%");
+ recipientCriteria.setResource(tableNameWithType);
+ recipientCriteria.setSessionSpecific(true);
+ TableDeletionMessage tableDeletionMessage = new TableDeletionMessage(tableNameWithType);
+ ClusterMessagingService messagingService = _helixZkManager.getMessagingService();
+
+ // Externalview can be null for newly created table, skip sending the message
+ if (_helixZkManager.getHelixDataAccessor()
+ .getProperty(_helixZkManager.getHelixDataAccessor().keyBuilder().externalView(tableNameWithType)) == null) {
+ LOGGER.warn("No delete table message sent for newly created table: {}", tableNameWithType);
Review comment:
`No delete table message sent for newly created table: {} as the exiernalview is null.`
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1757,6 +1758,10 @@ public void deleteOfflineTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager, offlineTableName);
LOGGER.info("Deleting table {}: Removed from broker resource", offlineTableName);
+ // Drop the table on servers
+ deleteTableOnServer(offlineTableName);
+ LOGGER.info("Deleting table {}: Sent table deletion message to servers", offlineTableName);
Review comment:
Same here.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1800,6 +1805,10 @@ public void deleteRealtimeTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager, realtimeTableName);
LOGGER.info("Deleting table {}: Removed from broker resource", realtimeTableName);
+ // Drop the table on servers
+ deleteTableOnServer(realtimeTableName);
+ LOGGER.info("Deleting table {}: Sent table deletion message to servers", realtimeTableName);
Review comment:
We may not need to print this message as there is a similar message printed inside `deleteTableOnServer` method already. Plus, even inside the `deleteTableOnServer` method there is still some scenarios that the message doesn't need to be sent. So I'd suggest skipping this log here.
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SegmentMessageHandlerFactory.java
##########
@@ -144,6 +147,28 @@ public HelixTaskResult handleMessage()
}
}
+ private class TableDeletionMessageHandler extends DefaultMessageHandler {
+ TableDeletionMessageHandler(TableDeletionMessage tableDeletionMessage, ServerMetrics metrics,
+ NotificationContext context) {
+ super(tableDeletionMessage, metrics, context);
+ }
+
+ @Override
+ public HelixTaskResult handleMessage()
+ throws InterruptedException {
+ HelixTaskResult helixTaskResult = new HelixTaskResult();
+ _logger.info("Handling message: {}", _message);
+ try {
+ _instanceDataManager.deleteTable(_tableNameWithType);
+ helixTaskResult.setSuccess(true);
+ } catch (Exception e) {
+ _metrics.addMeteredTableValue(_tableNameWithType, ServerMeter.DELETE_TABLE_FAILURES, 1);
+ Utils.rethrowException(e);
Review comment:
Will it print out the exception in the log?
--
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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839022387
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -408,7 +413,41 @@ public void testUploadSegmentRefreshOnly()
assertEquals(segmentsZKMetadata.size(), 1);
assertNotEquals(segmentsZKMetadata.get(0).getRefreshTime(), Long.MIN_VALUE);
}
+ waitForNumOfSegmentsBecomeOnline(offlineTableName, 1);
Review comment:
One way to handle the null case is to get all instances for the tenant, and send the message to them.
--
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 edited a comment on pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#issuecomment-1081038136
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8422?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 [#8422](https://codecov.io/gh/apache/pinot/pull/8422?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e8127aa) into [master](https://codecov.io/gh/apache/pinot/commit/5b44c22b46824101381c71e712716e97dd960657?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5b44c22) will **decrease** coverage by `34.73%`.
> The diff coverage is `92.18%`.
```diff
@@ Coverage Diff @@
## master #8422 +/- ##
=============================================
- Coverage 70.84% 36.11% -34.74%
+ Complexity 4278 84 -4194
=============================================
Files 1660 1661 +1
Lines 87035 87093 +58
Branches 13139 13142 +3
=============================================
- Hits 61663 31450 -30213
- Misses 21112 53123 +32011
+ Partials 4260 2520 -1740
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `28.69% <87.50%> (+0.02%)` | :arrow_up: |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.22% <64.70%> (+0.07%)` | :arrow_up: |
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/8422?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...server/starter/helix/HelixInstanceDataManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9IZWxpeEluc3RhbmNlRGF0YU1hbmFnZXIuamF2YQ==) | `78.12% <75.00%> (-1.32%)` | :arrow_down: |
| [...er/starter/helix/SegmentMessageHandlerFactory.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9TZWdtZW50TWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `80.00% <92.30%> (+6.31%)` | :arrow_up: |
| [...he/pinot/common/messages/TableDeletionMessage.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWVzc2FnZXMvVGFibGVEZWxldGlvbk1lc3NhZ2UuamF2YQ==) | `100.00% <100.00%> (ø)` | |
| [...a/org/apache/pinot/common/metrics/ServerMeter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJNZXRlci5qYXZh) | `100.00% <100.00%> (ø)` | |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `67.76% <100.00%> (+0.46%)` | :arrow_up: |
| [.../pinot/server/starter/helix/BaseServerStarter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | `51.54% <100.00%> (-5.80%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1007 more](https://codecov.io/gh/apache/pinot/pull/8422/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/pinot/pull/8422?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/pinot/pull/8422?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 [5b44c22...e8127aa](https://codecov.io/gh/apache/pinot/pull/8422?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: 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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839018545
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SegmentMessageHandlerFactory.java
##########
@@ -58,6 +59,8 @@ public MessageHandler createHandler(Message message, NotificationContext context
return new SegmentRefreshMessageHandler(new SegmentRefreshMessage(message), _metrics, context);
case SegmentReloadMessage.RELOAD_SEGMENT_MSG_SUB_TYPE:
return new SegmentReloadMessageHandler(new SegmentReloadMessage(message), _metrics, context);
+ case TableDeletionMessage.DELETE_TABLE_MSG_SUB_TYPE:
+ return new TableDeletionMessageHandler(new TableDeletionMessage(message), _metrics, context);
Review comment:
The message object is in the type of `Message` (checked by itelliJ debugger). So it should not be casted.
--
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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839056884
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SegmentMessageHandlerFactory.java
##########
@@ -144,6 +147,28 @@ public HelixTaskResult handleMessage()
}
}
+ private class TableDeletionMessageHandler extends DefaultMessageHandler {
+ TableDeletionMessageHandler(TableDeletionMessage tableDeletionMessage, ServerMetrics metrics,
+ NotificationContext context) {
+ super(tableDeletionMessage, metrics, context);
+ }
+
+ @Override
+ public HelixTaskResult handleMessage()
+ throws InterruptedException {
+ HelixTaskResult helixTaskResult = new HelixTaskResult();
+ _logger.info("Handling message: {}", _message);
Review comment:
Good catch, it will print some helix internal states which is not quite informative. Changed to `_logger.info("Handling table deletion message");`
--
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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839131473
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java
##########
@@ -73,6 +75,9 @@
@ThreadSafe
public class HelixInstanceDataManager implements InstanceDataManager {
private static final Logger LOGGER = LoggerFactory.getLogger(HelixInstanceDataManager.class);
+ // TODO: Make this configurable
+ private static final long EXTERNAL_VIEW_DROPPED_MAX_WAIT_MS = 20 * 60_000L; // 20 minutes
Review comment:
It's 10mins, but will retry on failure.
--
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 change in pull request #8422: Fix the race condition that TableDataManager is removed during segment download because of incorrect segment count
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r836730319
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/data/manager/SegmentDataManager.java
##########
@@ -25,9 +25,16 @@
/**
* Base segment data manager to maintain reference count for the segment.
*/
-public abstract class SegmentDataManager {
+public class SegmentDataManager {
Review comment:
Please add a singleton `DummySegmentDataManager` class instead of reusing the same class. We should keep this class abstract
--
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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r838958405
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -408,7 +413,41 @@ public void testUploadSegmentRefreshOnly()
assertEquals(segmentsZKMetadata.size(), 1);
assertNotEquals(segmentsZKMetadata.get(0).getRefreshTime(), Long.MIN_VALUE);
}
+ waitForNumOfSegmentsBecomeOnline(offlineTableName, 1);
dropOfflineTable(SEGMENT_UPLOAD_TEST_TABLE);
+ testTableDataManagerCleanup(offlineTableName);
+ }
+
+ private void waitForNumOfSegmentsBecomeOnline(String tableNameWithType, int numSegments)
+ throws InterruptedException, TimeoutException {
+ long endTimeMs = System.currentTimeMillis() + EXTERNAL_VIEW_ONLINE_SEGMENTS_MAX_WAIT_MS;
+ do {
+ Set<String> onlineSegments = _helixResourceManager.getOnlineSegmentsFromExternalView(tableNameWithType);
+ if (onlineSegments.size() == numSegments) {
+ return;
+ }
+ Thread.sleep(EXTERNAL_VIEW_CHECK_INTERVAL_MS);
+ } while (System.currentTimeMillis() < endTimeMs);
+ throw new TimeoutException(String
+ .format("Time out while waiting segments become ONLINE. (tableNameWithType = %s)", tableNameWithType));
+ }
+
+ private void testTableDataManagerCleanup(String offlineTableName)
Review comment:
Good catch, refactored the function and put it in the parent class.
--
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] PrachiKhobragade commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
PrachiKhobragade commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839021507
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SegmentMessageHandlerFactory.java
##########
@@ -144,6 +147,28 @@ public HelixTaskResult handleMessage()
}
}
+ private class TableDeletionMessageHandler extends DefaultMessageHandler {
+ TableDeletionMessageHandler(TableDeletionMessage tableDeletionMessage, ServerMetrics metrics,
+ NotificationContext context) {
+ super(tableDeletionMessage, metrics, context);
+ }
+
+ @Override
+ public HelixTaskResult handleMessage()
+ throws InterruptedException {
+ HelixTaskResult helixTaskResult = new HelixTaskResult();
+ _logger.info("Handling message: {}", _message);
Review comment:
What should this print, won't it just print the hashcode or do we need the content?
--
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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r838383274
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SegmentMessageHandlerFactory.java
##########
@@ -144,6 +147,23 @@ public HelixTaskResult handleMessage()
}
}
+ private class TableDeletionMessageHandler extends DefaultMessageHandler {
+ TableDeletionMessageHandler(TableDeletionMessage tableDeletionMessage, ServerMetrics metrics,
+ NotificationContext context) {
+ super(tableDeletionMessage, metrics, context);
+ }
+
+ @Override
+ public HelixTaskResult handleMessage()
+ throws InterruptedException {
+ HelixTaskResult helixTaskResult = new HelixTaskResult();
+ _logger.info("Handling message: {}", _message);
+ _instanceDataManager.deleteTable(_tableNameWithType);
Review comment:
Good point, added the server metrics `DELETE_TABLE_FAILURES`.
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java
##########
@@ -184,18 +184,26 @@ private TableDataManager createTableDataManager(String tableNameWithType, TableC
return tableDataManager;
}
+ @Override
+ public void deleteTable(String tableNameWithType) {
+ _tableDataManagerMap.compute(tableNameWithType, (k, v) -> {
+ if (v != null) {
+ v.shutDown();
Review comment:
The files on disk will be removed when the segment goes from `OFFLINE` to `DROPPED`, the clean up is based on table and segment name. We won’t have any issue if the tableDataManager is shutdown&removed before this state transition. But we will have the issue to go from `ONLINE` to `OFFLINE`, this is because segmentDataManger is destroyed via the tableDataManager for the state transition. As you mentioned in other comments, we need to make sure all segments are `OFFLINE/DROPPED` before removing the tableDataManager. Else we may have some resources unclosed.
--
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 edited a comment on pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#issuecomment-1081038136
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8422?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 [#8422](https://codecov.io/gh/apache/pinot/pull/8422?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e8127aa) into [master](https://codecov.io/gh/apache/pinot/commit/5b44c22b46824101381c71e712716e97dd960657?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5b44c22) will **decrease** coverage by `42.14%`.
> The diff coverage is `87.50%`.
```diff
@@ Coverage Diff @@
## master #8422 +/- ##
=============================================
- Coverage 70.84% 28.69% -42.15%
=============================================
Files 1660 1649 -11
Lines 87035 86739 -296
Branches 13139 13104 -35
=============================================
- Hits 61663 24894 -36769
- Misses 21112 59555 +38443
+ Partials 4260 2290 -1970
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `28.69% <87.50%> (+0.02%)` | :arrow_up: |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `?` | |
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/8422?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...server/starter/helix/HelixInstanceDataManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9IZWxpeEluc3RhbmNlRGF0YU1hbmFnZXIuamF2YQ==) | `78.12% <75.00%> (-1.32%)` | :arrow_down: |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `40.98% <87.50%> (-26.32%)` | :arrow_down: |
| [...er/starter/helix/SegmentMessageHandlerFactory.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9TZWdtZW50TWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `80.00% <92.30%> (+6.31%)` | :arrow_up: |
| [...he/pinot/common/messages/TableDeletionMessage.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWVzc2FnZXMvVGFibGVEZWxldGlvbk1lc3NhZ2UuamF2YQ==) | `100.00% <100.00%> (ø)` | |
| [...a/org/apache/pinot/common/metrics/ServerMeter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJNZXRlci5qYXZh) | `100.00% <100.00%> (ø)` | |
| [.../pinot/server/starter/helix/BaseServerStarter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | `51.54% <100.00%> (-5.80%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1197 more](https://codecov.io/gh/apache/pinot/pull/8422/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/pinot/pull/8422?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/pinot/pull/8422?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 [5b44c22...e8127aa](https://codecov.io/gh/apache/pinot/pull/8422?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: 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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839056123
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -2053,6 +2062,35 @@ public boolean updateZkMetadata(String tableNameWithType, SegmentZKMetadata segm
return ZKMetadataProvider.setSegmentZKMetadata(_propertyStore, tableNameWithType, segmentZKMetadata);
}
+ /**
+ * Delete the table on servers by sending table deletion message
+ */
+ private void deleteTableOnServer(String tableNameWithType) {
+ LOGGER.info("Sending delete table message for table: {}", tableNameWithType);
+ Criteria recipientCriteria = new Criteria();
+ recipientCriteria.setRecipientInstanceType(InstanceType.PARTICIPANT);
+ recipientCriteria.setInstanceName("%");
+ recipientCriteria.setResource(tableNameWithType);
+ recipientCriteria.setSessionSpecific(true);
+ TableDeletionMessage tableDeletionMessage = new TableDeletionMessage(tableNameWithType);
+ ClusterMessagingService messagingService = _helixZkManager.getMessagingService();
+
+ // Externalview can be null for newly created table, skip sending the message
+ if (_helixZkManager.getHelixDataAccessor()
+ .getProperty(_helixZkManager.getHelixDataAccessor().keyBuilder().externalView(tableNameWithType)) == null) {
+ LOGGER.warn("No delete table message sent for newly created table: {}", tableNameWithType);
Review comment:
Updated
--
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] snleee commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
snleee commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r838863260
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1757,6 +1758,10 @@ public void deleteOfflineTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager, offlineTableName);
LOGGER.info("Deleting table {}: Removed from broker resource", offlineTableName);
+ // Drop the table on servers
+ deleteTableOnServer(offlineTableName);
Review comment:
I see that we don't send the message if `externalview` is null from `deleteTableOnServer()`. If we send without filtering, the helix side will throw the exception? Or, you mention that we explicitly throw the exception?
I think that one main difference between waiting on the controller vs server is that we will not delete table resource (which is a safety check for avoiding race condition in table re-create) if we wait for externalview to converge on the controller side.
If we offload the waiting to happen on the server side, we now have another race condition when the external view gets updated with some significant delay. In that case, we will delete the table config on the following code block but the server side may not have done deleting and waiting for the external view to converge. If the new table creation request comes in, we will have some issues.
```
// Remove table config
// this should always be the last step for deletion to avoid race condition in table re-create.
ZKMetadataProvider.removeResourceConfigFromPropertyStore(_propertyStore, offlineTableName);
```
--
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] snleee commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
snleee commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r838100092
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1757,6 +1758,10 @@ public void deleteOfflineTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager, offlineTableName);
LOGGER.info("Deleting table {}: Removed from broker resource", offlineTableName);
+ // Drop the table on servers
+ deleteTableOnServer(offlineTableName);
Review comment:
As we discussed offline, we need to change the order to avoid some resources not getting cleaned up correctly.
- delete segments (this gets invoked by cleaning up the idealstate, which is triggered by dropResource() -> please refer PinotHelixResourceManager:deleteOfflineTable()
- wait until the external view is correctly cleaned up
- send the message to drop the table data manger on the server
--
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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r838958003
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/RealtimeClusterIntegrationTest.java
##########
@@ -154,6 +156,7 @@ public void testInstanceShutdown()
public void tearDown()
throws Exception {
dropRealtimeTable(getTableName());
+ testTableDataManagerCleanup();
Review comment:
Renamed
--
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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839008960
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -3285,10 +3324,11 @@ private void waitForSegmentsBecomeOnline(String tableNameWithType, Set<String> s
tableNameWithType, segmentsToCheck));
}
- private Set<String> getOnlineSegmentsFromExternalView(String tableNameWithType) {
+ public Set<String> getOnlineSegmentsFromExternalView(String tableNameWithType) {
ExternalView externalView = getTableExternalView(tableNameWithType);
- Preconditions
- .checkState(externalView != null, String.format("External view is null for table (%s)", tableNameWithType));
+ if (externalView == null) {
Review comment:
We use `getOnlineSegmentsFromExternalView()` in `updateSegmentLineageEntryToReverted()` to check if `segmentsFrom` for target lineage are all online. The `externalview` should be null only for newly created table, so it make sense to return empty set.
--
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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839010495
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1757,6 +1758,10 @@ public void deleteOfflineTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager, offlineTableName);
LOGGER.info("Deleting table {}: Removed from broker resource", offlineTableName);
+ // Drop the table on servers
+ deleteTableOnServer(offlineTableName);
Review comment:
Helix will throw the exception if `externalview` is null, and the message will not be sent.
--
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] jtao15 commented on pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#issuecomment-1082493001
Updated the pr to remove TableDataManager only when the table is removed based on offline discussions.
--
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] jtao15 commented on pull request #8422: Fix the race condition that TableDataManager is removed during segment download because of incorrect segment count
Posted by GitBox <gi...@apache.org>.
jtao15 commented on pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#issuecomment-1081008430
@jadami10 @jackjlli
--
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] PrachiKhobragade commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
PrachiKhobragade commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r838994387
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SegmentMessageHandlerFactory.java
##########
@@ -58,6 +59,8 @@ public MessageHandler createHandler(Message message, NotificationContext context
return new SegmentRefreshMessageHandler(new SegmentRefreshMessage(message), _metrics, context);
case SegmentReloadMessage.RELOAD_SEGMENT_MSG_SUB_TYPE:
return new SegmentReloadMessageHandler(new SegmentReloadMessage(message), _metrics, context);
+ case TableDeletionMessage.DELETE_TABLE_MSG_SUB_TYPE:
+ return new TableDeletionMessageHandler(new TableDeletionMessage(message), _metrics, context);
Review comment:
Does this need a new TableDeletionMessage or the current message can be just cast to 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: 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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839131605
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1757,6 +1758,16 @@ public void deleteOfflineTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager, offlineTableName);
LOGGER.info("Deleting table {}: Removed from broker resource", offlineTableName);
+ // Drop the table on servers
+ // TODO: Wait for externalview to converge on controllers instead of servers. This is because if externalview
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: 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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839040897
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1757,6 +1758,10 @@ public void deleteOfflineTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager, offlineTableName);
LOGGER.info("Deleting table {}: Removed from broker resource", offlineTableName);
+ // Drop the table on servers
+ deleteTableOnServer(offlineTableName);
Review comment:
Good point. I think one way to solve the race condition is to check if the externalview is null when we create the table. If it's not null, we can simply reject the request. Then we will guarantee the new table will not use the old tableDataManager. How do you think?
--
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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r838384259
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1757,6 +1758,10 @@ public void deleteOfflineTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager, offlineTableName);
LOGGER.info("Deleting table {}: Removed from broker resource", offlineTableName);
+ // Drop the table on servers
+ deleteTableOnServer(offlineTableName);
Review comment:
After taking a deeper look, helix messages will be sent to the instances in `Externalview` or `Idealstate`. Helix will throw errors if the `externalview/idealstate` is null. So we have to send the message before cleaning up the `idealstate`. I added the logic on server side to delete the `tableDataManager` only after `externalview `is cleaned up.
--
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] jtao15 commented on pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#issuecomment-1082909540
> Please add the testing case to cover this change.
Added integration tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839019506
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -408,7 +413,41 @@ public void testUploadSegmentRefreshOnly()
assertEquals(segmentsZKMetadata.size(), 1);
assertNotEquals(segmentsZKMetadata.get(0).getRefreshTime(), Long.MIN_VALUE);
}
+ waitForNumOfSegmentsBecomeOnline(offlineTableName, 1);
Review comment:
For newly created table, it's possible the `externalview` is null, and the message will not be sent. Hmm, we may have the issue to drop a new table then.
--
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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839056884
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SegmentMessageHandlerFactory.java
##########
@@ -144,6 +147,28 @@ public HelixTaskResult handleMessage()
}
}
+ private class TableDeletionMessageHandler extends DefaultMessageHandler {
+ TableDeletionMessageHandler(TableDeletionMessage tableDeletionMessage, ServerMetrics metrics,
+ NotificationContext context) {
+ super(tableDeletionMessage, metrics, context);
+ }
+
+ @Override
+ public HelixTaskResult handleMessage()
+ throws InterruptedException {
+ HelixTaskResult helixTaskResult = new HelixTaskResult();
+ _logger.info("Handling message: {}", _message);
Review comment:
Good catch, it will print some helix internal states which is not quite meaningful. Changed to `_logger.info("Handling table deletion message");`
--
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] jtao15 closed pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 closed pull request #8422:
URL: https://github.com/apache/pinot/pull/8422
--
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 edited a comment on pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#issuecomment-1081038136
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8422?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 [#8422](https://codecov.io/gh/apache/pinot/pull/8422?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2a49295) into [master](https://codecov.io/gh/apache/pinot/commit/e2053f6776911dcce5f1ef1e32ed35063ca10bea?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e2053f6) will **decrease** coverage by `44.77%`.
> The diff coverage is `83.87%`.
> :exclamation: Current head 2a49295 differs from pull request most recent head 55abe0b. Consider uploading reports for the commit 55abe0b to get more accurate results
```diff
@@ Coverage Diff @@
## master #8422 +/- ##
=============================================
- Coverage 70.73% 25.96% -44.78%
=============================================
Files 1662 1651 -11
Lines 87227 86929 -298
Branches 13205 13170 -35
=============================================
- Hits 61703 22568 -39135
- Misses 21238 62196 +40958
+ Partials 4286 2165 -2121
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `25.96% <83.87%> (-0.06%)` | :arrow_down: |
| unittests1 | `?` | |
| unittests2 | `?` | |
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/8422?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...server/starter/helix/HelixInstanceDataManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9IZWxpeEluc3RhbmNlRGF0YU1hbmFnZXIuamF2YQ==) | `78.64% <75.00%> (-0.80%)` | :arrow_down: |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `32.72% <77.27%> (-34.58%)` | :arrow_down: |
| [...er/starter/helix/SegmentMessageHandlerFactory.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9TZWdtZW50TWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `64.28% <92.30%> (-9.40%)` | :arrow_down: |
| [...he/pinot/common/messages/TableDeletionMessage.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWVzc2FnZXMvVGFibGVEZWxldGlvbk1lc3NhZ2UuamF2YQ==) | `100.00% <100.00%> (ø)` | |
| [...a/org/apache/pinot/common/metrics/ServerMeter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJNZXRlci5qYXZh) | `100.00% <100.00%> (ø)` | |
| [.../pinot/server/starter/helix/BaseServerStarter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9CYXNlU2VydmVyU3RhcnRlci5qYXZh) | `51.54% <100.00%> (-7.78%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1221 more](https://codecov.io/gh/apache/pinot/pull/8422/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/pinot/pull/8422?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/pinot/pull/8422?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 [e2053f6...55abe0b](https://codecov.io/gh/apache/pinot/pull/8422?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: 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 edited a comment on pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#issuecomment-1081038136
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8422?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 [#8422](https://codecov.io/gh/apache/pinot/pull/8422?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6889dc2) into [master](https://codecov.io/gh/apache/pinot/commit/e2053f6776911dcce5f1ef1e32ed35063ca10bea?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e2053f6) will **decrease** coverage by `6.50%`.
> The diff coverage is `67.64%`.
```diff
@@ Coverage Diff @@
## master #8422 +/- ##
============================================
- Coverage 70.73% 64.22% -6.51%
+ Complexity 4282 4281 -1
============================================
Files 1662 1618 -44
Lines 87227 85378 -1849
Branches 13205 13004 -201
============================================
- Hits 61703 54837 -6866
- Misses 21238 26553 +5315
+ Partials 4286 3988 -298
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `67.03% <10.00%> (-0.02%)` | :arrow_down: |
| unittests2 | `14.18% <64.70%> (+0.05%)` | :arrow_up: |
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/8422?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/pinot/common/messages/TableDeletionMessage.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWVzc2FnZXMvVGFibGVEZWxldGlvbk1lc3NhZ2UuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `64.65% <91.66%> (-2.65%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/ServerMeter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJNZXRlci5qYXZh) | `98.14% <100.00%> (-1.86%)` | :arrow_down: |
| [...va/org/apache/pinot/core/routing/RoutingTable.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yb3V0aW5nL1JvdXRpbmdUYWJsZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/common/config/NettyConfig.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL05ldHR5Q29uZmlnLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...apache/pinot/common/helix/ExtraInstanceConfig.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vaGVsaXgvRXh0cmFJbnN0YW5jZUNvbmZpZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [367 more](https://codecov.io/gh/apache/pinot/pull/8422/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/pinot/pull/8422?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/pinot/pull/8422?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 [e2053f6...6889dc2](https://codecov.io/gh/apache/pinot/pull/8422?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: 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] jtao15 commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839090215
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1757,6 +1758,10 @@ public void deleteOfflineTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager, offlineTableName);
LOGGER.info("Deleting table {}: Removed from broker resource", offlineTableName);
+ // Drop the table on servers
+ deleteTableOnServer(offlineTableName);
Review comment:
As discussed offline, it would be good to make the table deletion api idempotent, and all the server states and resources should be cleaned up before returning 200. Since the race condition only happens in rare cases (slow externalview converge + table recreation). Added the `TODO` to wait on the controller side.
--
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] jackjlli commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r838836727
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -2053,6 +2062,36 @@ public boolean updateZkMetadata(String tableNameWithType, SegmentZKMetadata segm
return ZKMetadataProvider.setSegmentZKMetadata(_propertyStore, tableNameWithType, segmentZKMetadata);
}
+ /**
+ * Delete the table on servers
+ */
+ private void deleteTableOnServer(String tableNameWithType) {
+ LOGGER.info("Sending delete message for table: {}", tableNameWithType);
+
+ Criteria recipientCriteria = new Criteria();
Review comment:
Add the comment above saying that:
```
// Send table deletion message to servers
```
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -408,7 +413,41 @@ public void testUploadSegmentRefreshOnly()
assertEquals(segmentsZKMetadata.size(), 1);
assertNotEquals(segmentsZKMetadata.get(0).getRefreshTime(), Long.MIN_VALUE);
}
+ waitForNumOfSegmentsBecomeOnline(offlineTableName, 1);
dropOfflineTable(SEGMENT_UPLOAD_TEST_TABLE);
+ testTableDataManagerCleanup(offlineTableName);
+ }
+
+ private void waitForNumOfSegmentsBecomeOnline(String tableNameWithType, int numSegments)
+ throws InterruptedException, TimeoutException {
+ long endTimeMs = System.currentTimeMillis() + EXTERNAL_VIEW_ONLINE_SEGMENTS_MAX_WAIT_MS;
+ do {
+ Set<String> onlineSegments = _helixResourceManager.getOnlineSegmentsFromExternalView(tableNameWithType);
+ if (onlineSegments.size() == numSegments) {
+ return;
+ }
+ Thread.sleep(EXTERNAL_VIEW_CHECK_INTERVAL_MS);
+ } while (System.currentTimeMillis() < endTimeMs);
+ throw new TimeoutException(String
+ .format("Time out while waiting segments become ONLINE. (tableNameWithType = %s)", tableNameWithType));
+ }
+
+ private void testTableDataManagerCleanup(String offlineTableName)
Review comment:
Is it possible to put this method to `BaseClusterIntegrationTestSet` since both offline and realtime cluster integration tests have this method respectively?
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/RealtimeClusterIntegrationTest.java
##########
@@ -154,6 +156,7 @@ public void testInstanceShutdown()
public void tearDown()
throws Exception {
dropRealtimeTable(getTableName());
+ testTableDataManagerCleanup();
Review comment:
Rename it to `cleanupTestTableDataManager`?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -3285,10 +3324,11 @@ private void waitForSegmentsBecomeOnline(String tableNameWithType, Set<String> s
tableNameWithType, segmentsToCheck));
}
- private Set<String> getOnlineSegmentsFromExternalView(String tableNameWithType) {
+ public Set<String> getOnlineSegmentsFromExternalView(String tableNameWithType) {
ExternalView externalView = getTableExternalView(tableNameWithType);
- Preconditions
- .checkState(externalView != null, String.format("External view is null for table (%s)", tableNameWithType));
+ if (externalView == null) {
Review comment:
+1 on this. It's a valid scenario that externalview might be null.
--
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] snleee commented on a change in pull request #8422: Change TableDataManger deletion to message based
Posted by GitBox <gi...@apache.org>.
snleee commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839119100
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1757,6 +1758,16 @@ public void deleteOfflineTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager, offlineTableName);
LOGGER.info("Deleting table {}: Removed from broker resource", offlineTableName);
+ // Drop the table on servers
+ // TODO: Wait for externalview to converge on controllers instead of servers. This is because if externalview
+ // gets updated with significant delay. We may have the race condition for table recreation that
Review comment:
`delay. We -> delay, we`
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1757,6 +1758,16 @@ public void deleteOfflineTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager, offlineTableName);
LOGGER.info("Deleting table {}: Removed from broker resource", offlineTableName);
+ // Drop the table on servers
+ // TODO: Wait for externalview to converge on controllers instead of servers. This is because if externalview
Review comment:
Can you also mention that we should make this API to be blocking and we also need to make this to be idempotent?
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java
##########
@@ -73,6 +75,9 @@
@ThreadSafe
public class HelixInstanceDataManager implements InstanceDataManager {
private static final Logger LOGGER = LoggerFactory.getLogger(HelixInstanceDataManager.class);
+ // TODO: Make this configurable
+ private static final long EXTERNAL_VIEW_DROPPED_MAX_WAIT_MS = 20 * 60_000L; // 20 minutes
Review comment:
What is our default value for the merge and rollup on the segment replacement protocol?
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -408,7 +413,41 @@ public void testUploadSegmentRefreshOnly()
assertEquals(segmentsZKMetadata.size(), 1);
assertNotEquals(segmentsZKMetadata.get(0).getRefreshTime(), Long.MIN_VALUE);
}
+ waitForNumOfSegmentsBecomeOnline(offlineTableName, 1);
Review comment:
If make the drop table blocking, we probably don't need to wait (drop table will make sure to wait to converge before deleting if there's a difference between idealstate & external view).
Let's add `TODO` comment as well to remove this once we make delete API to be blocking
--
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 #8422: Fix the race condition that TableDataManager is removed during segment download because of incorrect segment count
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#issuecomment-1081038136
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8422?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 [#8422](https://codecov.io/gh/apache/pinot/pull/8422?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7c47843) into [master](https://codecov.io/gh/apache/pinot/commit/0d6b5da7c6872229501faf6cfc27481f35f52584?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0d6b5da) will **decrease** coverage by `6.76%`.
> The diff coverage is `17.64%`.
```diff
@@ Coverage Diff @@
## master #8422 +/- ##
============================================
- Coverage 70.83% 64.07% -6.77%
+ Complexity 4283 4281 -2
============================================
Files 1656 1611 -45
Lines 86690 84820 -1870
Branches 13076 12875 -201
============================================
- Hits 61407 54345 -7062
- Misses 21044 26535 +5491
+ Partials 4239 3940 -299
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `66.97% <17.64%> (-0.07%)` | :arrow_down: |
| unittests2 | `14.13% <0.00%> (-0.03%)` | :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/8422?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../pinot/core/data/manager/BaseTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvQmFzZVRhYmxlRGF0YU1hbmFnZXIuamF2YQ==) | `83.40% <0.00%> (-3.41%)` | :arrow_down: |
| [...ata/manager/realtime/RealtimeTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVUYWJsZURhdGFNYW5hZ2VyLmphdmE=) | `11.64% <0.00%> (-56.63%)` | :arrow_down: |
| [...segment/local/data/manager/SegmentDataManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9kYXRhL21hbmFnZXIvU2VnbWVudERhdGFNYW5hZ2VyLmphdmE=) | `70.58% <50.00%> (-12.75%)` | :arrow_down: |
| [...va/org/apache/pinot/core/routing/RoutingTable.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yb3V0aW5nL1JvdXRpbmdUYWJsZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/common/config/NettyConfig.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL05ldHR5Q29uZmlnLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...apache/pinot/common/helix/ExtraInstanceConfig.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vaGVsaXgvRXh0cmFJbnN0YW5jZUNvbmZpZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [386 more](https://codecov.io/gh/apache/pinot/pull/8422/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/pinot/pull/8422?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/pinot/pull/8422?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 [0d6b5da...7c47843](https://codecov.io/gh/apache/pinot/pull/8422?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: 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 edited a comment on pull request #8422: Fix the race condition that TableDataManager is removed during segment download because of incorrect segment count
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#issuecomment-1081038136
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8422?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 [#8422](https://codecov.io/gh/apache/pinot/pull/8422?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7c47843) into [master](https://codecov.io/gh/apache/pinot/commit/0d6b5da7c6872229501faf6cfc27481f35f52584?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0d6b5da) will **decrease** coverage by `0.05%`.
> The diff coverage is `56.75%`.
```diff
@@ Coverage Diff @@
## master #8422 +/- ##
============================================
- Coverage 70.83% 70.78% -0.06%
+ Complexity 4283 4281 -2
============================================
Files 1656 1656
Lines 86690 86714 +24
Branches 13076 13078 +2
============================================
- Hits 61407 61378 -29
- Misses 21044 21089 +45
- Partials 4239 4247 +8
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `28.68% <48.64%> (-0.03%)` | :arrow_down: |
| integration2 | `27.37% <48.64%> (+0.03%)` | :arrow_up: |
| unittests1 | `66.97% <17.64%> (-0.07%)` | :arrow_down: |
| unittests2 | `14.13% <0.00%> (-0.03%)` | :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/8422?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/realtime/RealtimeTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVUYWJsZURhdGFNYW5hZ2VyLmphdmE=) | `68.27% <0.00%> (ø)` | |
| [.../pinot/core/data/manager/BaseTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvQmFzZVRhYmxlRGF0YU1hbmFnZXIuamF2YQ==) | `85.47% <40.00%> (-1.34%)` | :arrow_down: |
| [...segment/local/data/manager/SegmentDataManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9kYXRhL21hbmFnZXIvU2VnbWVudERhdGFNYW5hZ2VyLmphdmE=) | `70.58% <50.00%> (-12.75%)` | :arrow_down: |
| [...server/starter/helix/HelixInstanceDataManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9IZWxpeEluc3RhbmNlRGF0YU1hbmFnZXIuamF2YQ==) | `78.23% <70.00%> (-1.21%)` | :arrow_down: |
| [...data/manager/realtime/DefaultSegmentCommitter.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvRGVmYXVsdFNlZ21lbnRDb21taXR0ZXIuamF2YQ==) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [...er/api/resources/LLCSegmentCompletionHandlers.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL0xMQ1NlZ21lbnRDb21wbGV0aW9uSGFuZGxlcnMuamF2YQ==) | `43.56% <0.00%> (-18.82%)` | :arrow_down: |
| [...nt/local/startree/v2/store/StarTreeDataSource.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9zdG9yZS9TdGFyVHJlZURhdGFTb3VyY2UuamF2YQ==) | `40.00% <0.00%> (-13.34%)` | :arrow_down: |
| [...data/manager/realtime/SegmentCommitterFactory.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvU2VnbWVudENvbW1pdHRlckZhY3RvcnkuamF2YQ==) | `88.23% <0.00%> (-11.77%)` | :arrow_down: |
| [...ore/query/scheduler/resources/ResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zY2hlZHVsZXIvcmVzb3VyY2VzL1Jlc291cmNlTWFuYWdlci5qYXZh) | `85.71% <0.00%> (-10.72%)` | :arrow_down: |
| [...transform/function/IsNotNullTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/8422/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vSXNOb3ROdWxsVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `67.85% <0.00%> (-7.15%)` | :arrow_down: |
| ... and [29 more](https://codecov.io/gh/apache/pinot/pull/8422/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/pinot/pull/8422?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/pinot/pull/8422?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 [0d6b5da...7c47843](https://codecov.io/gh/apache/pinot/pull/8422?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: 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