You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "KKcorps (via GitHub)" <gi...@apache.org> on 2023/04/13 11:10:40 UTC
[GitHub] [pinot] KKcorps opened a new pull request, #10602: Add a pre-shutdown method to make deleting tables faster
KKcorps opened a new pull request, #10602:
URL: https://github.com/apache/pinot/pull/10602
Currently, when a table is deleted we wait for EV to converge and then trigger the shutdown.
However, this leads to performance bottlenecks primarily being that during EV convergence, segments are removed from upsert metadata and their valid doc ids snapshots are taken.
This can be avoided if we simply add a preShutDown method and stop the upsert metadata manager in that call. When upsert metadata manager is stopped, we simply skip the segment removal and directly return null.
The call to this method can be made before shutdown.
In future, we can also add other calls to this method.
--
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] KKcorps commented on pull request #10602: Add a pre-shutdown method to make deleting tables faster
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on PR #10602:
URL: https://github.com/apache/pinot/pull/10602#issuecomment-1519460056
> Can you check if we can directly call shut down? Calling shut down will prevent consuming segment from committing, which is desired
It works when I tried it but then we won't be waiting for EV to converge. It would lead to race condition issues mentioned in
https://github.com/apache/pinot/issues/8423
--
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
Re: [PR] Add a pre-shutdown method to make deleting tables faster [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang closed pull request #10602: Add a pre-shutdown method to make deleting tables faster
URL: https://github.com/apache/pinot/pull/10602
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10602: Add a pre-shutdown method to make deleting tables faster
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10602:
URL: https://github.com/apache/pinot/pull/10602#discussion_r1165995898
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java:
##########
@@ -235,6 +236,15 @@ public void deleteTable(String tableNameWithType)
throws Exception {
// Wait externalview to converge
long endTimeMs = System.currentTimeMillis() + _externalViewDroppedMaxWaitMs;
+ _tableDataManagerMap.compute(tableNameWithType, (k, v) -> {
Review Comment:
Don't use `compute()` here. `get()` should be enough. Currently this compute will remove the table data manager, preventing it being shut down when external view is removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #10602: Add a pre-shutdown method to make deleting tables faster
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #10602:
URL: https://github.com/apache/pinot/pull/10602#issuecomment-1522518227
> > Can you check if we can directly call shut down? Calling shut down will prevent consuming segment from committing, which is desired
>
> It works when I tried it but then we won't be waiting for EV to converge. It would lead to race condition issues mentioned in #8423
I don't think we will run into the same issue. In #8423, the race condition is triggered when table data manager is shut down without user deleting the table, and the new uploaded segment data gets deleted. The fix for that is to introduce the table delete message which ensures the table is deleted.
Since we know the table is guaranteed to be deleted, we should be able to just shut down the table data manager, wait for all segments to be cleaned up (EV disappear, or current state disappear which is even better), then remove it from the instance data manager. That is actually similar to the server shut down where we first shut down the data manager, then wait for segments to be OFFLINE
--
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 #10602: Add a pre-shutdown method to make deleting tables faster
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10602:
URL: https://github.com/apache/pinot/pull/10602#issuecomment-1506858024
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10602?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 [#10602](https://codecov.io/gh/apache/pinot/pull/10602?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f2c0604) into [master](https://codecov.io/gh/apache/pinot/commit/0cb456a3b03805dbba5b7b34db7c0d877c08c58d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0cb456a) will **decrease** coverage by `5.98%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10602 +/- ##
============================================
- Coverage 70.37% 64.39% -5.98%
+ Complexity 6495 6471 -24
============================================
Files 2106 2052 -54
Lines 113004 110524 -2480
Branches 17026 16729 -297
============================================
- Hits 79521 71174 -8347
- Misses 27917 34199 +6282
+ Partials 5566 5151 -415
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `67.97% <0.00%> (-0.01%)` | :arrow_down: |
| unittests2 | `13.91% <0.00%> (+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/10602?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/10602?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==) | `72.96% <0.00%> (-2.64%)` | :arrow_down: |
| [.../data/manager/offline/OfflineTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/10602?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvb2ZmbGluZS9PZmZsaW5lVGFibGVEYXRhTWFuYWdlci5qYXZh) | `80.00% <0.00%> (-20.00%)` | :arrow_down: |
| [...ata/manager/realtime/RealtimeTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/10602?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=) | `29.16% <0.00%> (-38.26%)` | :arrow_down: |
| [...t/segment/local/data/manager/TableDataManager.java](https://codecov.io/gh/apache/pinot/pull/10602?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9kYXRhL21hbmFnZXIvVGFibGVEYXRhTWFuYWdlci5qYXZh) | `0.00% <ø> (ø)` | |
... and [433 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10602/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add a pre-shutdown method to make deleting tables faster [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #10602:
URL: https://github.com/apache/pinot/pull/10602#issuecomment-1762162825
#11380 is picked instead
--
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