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/12/24 08:12:09 UTC
[GitHub] [pinot] saurabhd336 opened a new pull request, #10028: Reduce the number of segments to wait for convergence when rebalancing
saurabhd336 opened a new pull request, #10028:
URL: https://github.com/apache/pinot/pull/10028
Instructions:
1. The PR has to be tagged with at least one of the following labels (*):
1. `feature`
2. `bugfix`
3. `performance`
4. `ui`
5. `backward-incompat`
6. `release-notes` (**)
2. Remove these instructions before publishing the PR.
(*) Other labels to consider:
- `testing`
- `dependencies`
- `docker`
- `kubernetes`
- `observability`
- `security`
- `code-style`
- `extension-point`
- `refactor`
- `cleanup`
(**) Use `release-notes` label for scenarios like:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins 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] codecov-commenter commented on pull request #10028: Reduce the number of segments to wait for convergence when rebalancing
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #10028:
URL: https://github.com/apache/pinot/pull/10028#issuecomment-1364490958
# [Codecov](https://codecov.io/gh/apache/pinot/pull/10028?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 [#10028](https://codecov.io/gh/apache/pinot/pull/10028?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d9d3a4e) into [master](https://codecov.io/gh/apache/pinot/commit/880a5c779fc441124ea14013a52d966885a58074?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (880a5c7) will **decrease** coverage by `39.20%`.
> The diff coverage is `72.72%`.
> :exclamation: Current head d9d3a4e differs from pull request most recent head f7db876. Consider uploading reports for the commit f7db876 to get more accurate results
```diff
@@ Coverage Diff @@
## master #10028 +/- ##
=============================================
- Coverage 64.28% 25.07% -39.21%
+ Complexity 5669 44 -5625
=============================================
Files 1939 1982 +43
Lines 105033 107136 +2103
Branches 16041 16295 +254
=============================================
- Hits 67518 26868 -40650
- Misses 32631 77553 +44922
+ Partials 4884 2715 -2169
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `25.07% <72.72%> (?)` | |
| 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/10028?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ntroller/helix/core/rebalance/TableRebalancer.java](https://codecov.io/gh/apache/pinot/pull/10028/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYmFsYW5jZS9UYWJsZVJlYmFsYW5jZXIuamF2YQ==) | `54.17% <61.53%> (-19.87%)` | :arrow_down: |
| [...ore/assignment/segment/SegmentAssignmentUtils.java](https://codecov.io/gh/apache/pinot/pull/10028/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvc2VnbWVudC9TZWdtZW50QXNzaWdubWVudFV0aWxzLmphdmE=) | `42.02% <88.88%> (-55.45%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/spi/utils/LoopUtils.java](https://codecov.io/gh/apache/pinot/pull/10028/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvTG9vcFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/10028/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/10028/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/10028/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/10028/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/config/user/RoleType.java](https://codecov.io/gh/apache/pinot/pull/10028/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3VzZXIvUm9sZVR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/10028/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: |
| [...ava/org/apache/pinot/spi/stream/StreamMessage.java](https://codecov.io/gh/apache/pinot/pull/10028/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvc3RyZWFtL1N0cmVhbU1lc3NhZ2UuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1603 more](https://codecov.io/gh/apache/pinot/pull/10028/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] saurabhd336 commented on a diff in pull request #10028: Reduce the number of segments to wait for convergence when rebalancing
Posted by GitBox <gi...@apache.org>.
saurabhd336 commented on code in PR #10028:
URL: https://github.com/apache/pinot/pull/10028#discussion_r1056847378
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/segment/SegmentAssignmentUtils.java:
##########
@@ -272,6 +272,7 @@ public static Map<String, String> getInstanceStateMap(Collection<String> instanc
return instanceStateMap;
}
+
Review Comment:
Ack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #10028: Reduce the number of segments to wait for convergence when rebalancing
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged PR #10028:
URL: https://github.com/apache/pinot/pull/10028
--
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 #10028: Reduce the number of segments to wait for convergence when rebalancing
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10028:
URL: https://github.com/apache/pinot/pull/10028#discussion_r1056776854
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/segment/SegmentAssignmentUtils.java:
##########
@@ -272,6 +272,7 @@ public static Map<String, String> getInstanceStateMap(Collection<String> instanc
return instanceStateMap;
}
+
Review Comment:
(nit) remove
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/segment/SegmentAssignmentUtils.java:
##########
@@ -292,6 +293,21 @@ public static Map<String, Integer> getNumSegmentsToBeMovedPerInstance(Map<String
return numSegmentsToBeMovedPerInstance;
}
+ public static Set<String> getSegmentsToMove(Map<String, Map<String, String>> oldAssignment,
+ Map<String, Map<String, String>> newAssignment) {
+ Set<String> result = new HashSet<>();
+ for (Map.Entry<String, Map<String, String>> entry : newAssignment.entrySet()) {
+ String segmentName = entry.getKey();
+ Set<String> newInstancesAssigned = entry.getValue().keySet();
+ Set<String> oldInstancesAssigned = oldAssignment.get(segmentName).keySet();
+ if (!newInstancesAssigned.containsAll(oldInstancesAssigned) || !oldInstancesAssigned.containsAll(
+ newInstancesAssigned)) {
Review Comment:
We can simplify it and directly compare the assignment
```suggestion
if (!entry.getValue().equals(oldAssignment.get(segmentName)) {
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -304,11 +304,18 @@ public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanc
LOGGER.info("Rebalancing table: {} with minAvailableReplicas: {}, enableStrictReplicaGroup: {}, bestEfforts: {}",
tableNameWithType, minAvailableReplicas, enableStrictReplicaGroup, bestEfforts);
int expectedVersion = currentIdealState.getRecord().getVersion();
- while (true) {
- // Wait for ExternalView to converge before updating the next IdealState
- IdealState idealState;
+
+ do {
+ Map<String, Map<String, String>> nextAssignment =
+ getNextAssignment(currentAssignment, targetAssignment, minAvailableReplicas, enableStrictReplicaGroup);
+ LOGGER.info("Got the next assignment for table: {} with number of segments to be moved to each instance: {}",
+ tableNameWithType,
+ SegmentAssignmentUtils.getNumSegmentsToBeMovedPerInstance(currentAssignment, nextAssignment));
+
+ Set<String> segmentsToMove = SegmentAssignmentUtils.getSegmentsToMove(currentAssignment, nextAssignment);
Review Comment:
`segmentsToMove` can be calculated with current and target assignment. We can keep the current logic of calculating next assignment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] saurabhd336 commented on a diff in pull request #10028: Reduce the number of segments to wait for convergence when rebalancing
Posted by GitBox <gi...@apache.org>.
saurabhd336 commented on code in PR #10028:
URL: https://github.com/apache/pinot/pull/10028#discussion_r1056847503
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/segment/SegmentAssignmentUtils.java:
##########
@@ -292,6 +293,21 @@ public static Map<String, Integer> getNumSegmentsToBeMovedPerInstance(Map<String
return numSegmentsToBeMovedPerInstance;
}
+ public static Set<String> getSegmentsToMove(Map<String, Map<String, String>> oldAssignment,
+ Map<String, Map<String, String>> newAssignment) {
+ Set<String> result = new HashSet<>();
+ for (Map.Entry<String, Map<String, String>> entry : newAssignment.entrySet()) {
+ String segmentName = entry.getKey();
+ Set<String> newInstancesAssigned = entry.getValue().keySet();
+ Set<String> oldInstancesAssigned = oldAssignment.get(segmentName).keySet();
+ if (!newInstancesAssigned.containsAll(oldInstancesAssigned) || !oldInstancesAssigned.containsAll(
+ newInstancesAssigned)) {
Review Comment:
Ack
--
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