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/11/10 01:58:15 UTC
[GitHub] [pinot] GSharayu opened a new pull request, #9774: Fix the null pointer exception for segment assignment strategy in reassigning tiers (Table rebalance)
GSharayu opened a new pull request, #9774:
URL: https://github.com/apache/pinot/pull/9774
The segment assignment strategy was not getting set during rebalance tiers.
As mentioned in comment: https://github.com/apache/pinot/pull/9309#discussion_r1018496195
This code handles the null pointer exception and inits the assignment strategy with default config if not set
Reference PR [Introduce Segment AssignmentStrategy Interface](https://github.com/apache/pinot/pull/9309)
#9309
--
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] npawar commented on pull request #9774: Fix the null pointer exception for segment assignment strategy in reassigning tiers (Table rebalance)
Posted by GitBox <gi...@apache.org>.
npawar commented on PR #9774:
URL: https://github.com/apache/pinot/pull/9774#issuecomment-1310895331
> IIUC, this seems what I was assuming to happen: to move COMPLETED segments to new tier if they matches the selection criteria. And leave CONSUMING and other non-matching COMPLETED segments to the default tenant configured for the REALTIME table (DefaultTenant_REALTIME by default).
this discussion is not very clear to me, but is this is the conclusion, then lgtm
```
IIUC, this seems what I was assuming to happen: to move COMPLETED segments to new tier if they matches the selection criteria. And leave CONSUMING and other non-matching COMPLETED segments to the default tenant configured for the REALTIME table (DefaultTenant_REALTIME by default).
```
--
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 #9774: Fix the null pointer exception for segment assignment strategy in reassigning tiers (Table rebalance)
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9774:
URL: https://github.com/apache/pinot/pull/9774#issuecomment-1309707793
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9774?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 [#9774](https://codecov.io/gh/apache/pinot/pull/9774?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (923e44f) into [master](https://codecov.io/gh/apache/pinot/commit/2c38d292ea85d86a166addea9c8e85b11654217f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2c38d29) will **decrease** coverage by `0.02%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #9774 +/- ##
============================================
- Coverage 70.15% 70.12% -0.03%
- Complexity 4976 4977 +1
============================================
Files 1955 1955
Lines 104880 104883 +3
Branches 15874 15875 +1
============================================
- Hits 73576 73547 -29
- Misses 26164 26194 +30
- Partials 5140 5142 +2
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `25.30% <0.00%> (-0.12%)` | :arrow_down: |
| integration2 | `24.58% <0.00%> (+<0.01%)` | :arrow_up: |
| unittests1 | `67.65% <ø> (+<0.01%)` | :arrow_up: |
| unittests2 | `15.68% <0.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9774?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...core/assignment/segment/BaseSegmentAssignment.java](https://codecov.io/gh/apache/pinot/pull/9774/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvc2VnbWVudC9CYXNlU2VnbWVudEFzc2lnbm1lbnQuamF2YQ==) | `91.48% <0.00%> (-6.24%)` | :arrow_down: |
| [...ntroller/helix/core/minion/TaskMetricsEmitter.java](https://codecov.io/gh/apache/pinot/pull/9774/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9UYXNrTWV0cmljc0VtaXR0ZXIuamF2YQ==) | `34.88% <0.00%> (-51.17%)` | :arrow_down: |
| [...n/java/org/apache/pinot/common/utils/URIUtils.java](https://codecov.io/gh/apache/pinot/pull/9774/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvVVJJVXRpbHMuamF2YQ==) | `66.66% <0.00%> (-7.41%)` | :arrow_down: |
| [...transform/function/IsNotNullTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/9774/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==) | `65.51% <0.00%> (-6.90%)` | :arrow_down: |
| [...or/transform/function/IsNullTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/9774/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vSXNOdWxsVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `75.86% <0.00%> (-6.90%)` | :arrow_down: |
| [...nction/DistinctCountBitmapAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/9774/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) | `47.15% <0.00%> (-6.22%)` | :arrow_down: |
| [...elix/core/periodictask/ControllerPeriodicTask.java](https://codecov.io/gh/apache/pinot/pull/9774/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3BlcmlvZGljdGFzay9Db250cm9sbGVyUGVyaW9kaWNUYXNrLmphdmE=) | `66.07% <0.00%> (-5.36%)` | :arrow_down: |
| [...lix/core/minion/PinotHelixTaskResourceManager.java](https://codecov.io/gh/apache/pinot/pull/9774/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9QaW5vdEhlbGl4VGFza1Jlc291cmNlTWFuYWdlci5qYXZh) | `40.70% <0.00%> (-2.77%)` | :arrow_down: |
| [...inot/core/operator/filter/FilterOperatorUtils.java](https://codecov.io/gh/apache/pinot/pull/9774/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvRmlsdGVyT3BlcmF0b3JVdGlscy5qYXZh) | `87.50% <0.00%> (-2.50%)` | :arrow_down: |
| [...roller/helix/core/relocation/SegmentRelocator.java](https://codecov.io/gh/apache/pinot/pull/9774/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlbG9jYXRpb24vU2VnbWVudFJlbG9jYXRvci5qYXZh) | `63.57% <0.00%> (-1.43%)` | :arrow_down: |
| ... and [15 more](https://codecov.io/gh/apache/pinot/pull/9774/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) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] snleee commented on pull request #9774: Fix the null pointer exception for segment assignment strategy in reassigning tiers (Table rebalance)
Posted by GitBox <gi...@apache.org>.
snleee commented on PR #9774:
URL: https://github.com/apache/pinot/pull/9774#issuecomment-1309821451
To summarize the comment:
1. Let's make sure that we guarantee `segmentAssignmentStrategy` to be non-null in `RealtimeSegmentAssignment`
2. We should decide the rebalance behavior for the edge case above.
--
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 diff in pull request #9774: Fix the null pointer exception for segment assignment strategy in reassigning tiers (Table rebalance)
Posted by GitBox <gi...@apache.org>.
snleee commented on code in PR #9774:
URL: https://github.com/apache/pinot/pull/9774#discussion_r1018655342
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/segment/BaseSegmentAssignment.java:
##########
@@ -126,6 +127,15 @@ protected Pair<List<Map<String, Map<String, String>>>, Map<String, Map<String, S
_logger.info("Rebalancing tier: {} for table: {} with bootstrap: {}, instance partitions: {}", tierName,
_tableNameWithType, bootstrap, tierInstancePartitions);
+
+ // Set the default assignment strategy in case if it not set already
+ // Assuming all tiers use the same assignment strategy
+ if (segmentAssignmentStrategy == null) {
Review Comment:
Given that `segmentAssignmentStrategy` is not annotated as `@Nullable`, we can assume that `segmentAssignmentStrategy` will never be `null` in the perspective of `BaseSegmentAssignment`. IMO, we should handle this null issue in the caller of this function, which is `RealtimeSegmentAssignment`.
In `RealtimeSegmentAssignment`, we correctly handle for `OFFLINE/COMPLETED`; however, I think that the current code can pass `null` for `rebalanceTiers()` function.
We should update the following part `RealtimeSegmentAssignment:L193`:
```
SegmentAssignmentStrategy segmentAssignmentStrategy = null;
if (completedInstancePartitions != null) {
// Gets Segment assignment strategy for instance partitions
segmentAssignmentStrategy = SegmentAssignmentStrategyFactory
.getSegmentAssignmentStrategy(_helixManager, _tableConfig, InstancePartitionsType.COMPLETED.toString(),
completedInstancePartitions);
}
```
--
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] klsince commented on pull request #9774: Fix the null pointer exception for segment assignment strategy in reassigning tiers (Table rebalance)
Posted by GitBox <gi...@apache.org>.
klsince commented on PR #9774:
URL: https://github.com/apache/pinot/pull/9774#issuecomment-1310689036
Thanks for digging into this issue, @GSharayu @snleee !
> ... we can enforce that COMPLETED instance partition needs to be configured if Tier is used...
> ... We can improve the rebalance behavior when we have the clear idea if we really need to configure tiers without COMPLETED tenant.
I didn't know there is need to configure a COMPLETED tenant. I see tenants are usually named like DefaultTenant_OFFLINE or DefaultTenant_REALTIME for different table types, as well as in the local test I did. Is the `DefaultTenant_REALTIME` used as CONSUMING and COMPLETED tenants for RT table by default? Feel free to point me to docs about those if any.
> 2. Move all COMPLETED segments to Tiers after rebalance
IIUC, this seems what I was assuming to happen: to move COMPLETED segments to new tier if they matches the selection criteria. And leave CONSUMING and other non-matching COMPLETED segments to the default tenant configured for the REALTIME table (DefaultTenant_REALTIME by default).
--
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] GSharayu closed pull request #9774: Fix the null pointer exception for segment assignment strategy in reassigning tiers (Table rebalance)
Posted by GitBox <gi...@apache.org>.
GSharayu closed pull request #9774: Fix the null pointer exception for segment assignment strategy in reassigning tiers (Table rebalance)
URL: https://github.com/apache/pinot/pull/9774
--
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] GSharayu commented on pull request #9774: Fix the null pointer exception for segment assignment strategy in reassigning tiers (Table rebalance)
Posted by GitBox <gi...@apache.org>.
GSharayu commented on PR #9774:
URL: https://github.com/apache/pinot/pull/9774#issuecomment-1309663347
@snleee Please review
--
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