Add a dry-run summary mode for TableRebalance which only returns a summary of the dry-run results - #15050
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #15050 +/- ##
============================================
+ Coverage 61.75% 63.42% +1.66%
- Complexity 207 1481 +1274
============================================
Files 2436 2745 +309
Lines 133233 154271 +21038
Branches 20636 23803 +3167
============================================
+ Hits 82274 97839 +15565
- Misses 44911 49041 +4130
- Partials 6048 7391 +1343
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
2449cc2 to
aa3fa91
Compare
A few comments on the summary. Feel free to just take those that make sense for this first iteration:
|
447752e to
35ba42c
Compare
|
Updated summary: Adding a server: Removing a server:
@npawar responses regarding the above are below:
|
| long totalDataPerServerInBytes = totalEstimatedDataToBeMovedInBytes / numServersGettingDataAdded; | ||
| // TODO: Pick a good threshold to calculate estimated time to rebalance. For now assume 100 MB/s data download | ||
| // + process rate | ||
| estimatedTimeToRebalanceInSec = ((double) totalDataPerServerInBytes) / (100.0D * 1024.0D * 1024.0D); |
There was a problem hiding this comment.
I think maybe we skip this field, as it's not easy to estimate this accurately because the cluster setup can vary a lot. The summary contains the total amount of segments and data bytes a server may get now, so the user can estimate the duration for their envs accordingly.
There was a problem hiding this comment.
yeah, this is a tricky field for sure. I've removed it for now, but added a TODO for adding it back once we have a better understanding of a good estimate. This field was an ask for this summary PR.
| RebalanceSummaryResult summaryResult = null; | ||
| if (summary) { | ||
| summaryResult = calculateDryRunSummary(currentAssignment, targetAssignment, tableNameWithType, rebalanceJobId); | ||
| } |
There was a problem hiding this comment.
how about calculating the summary after this whole if-block, so we save the cost from calculating it when the table is already balanced?
if (segmentAssignmentUnchanged) {
..
}
There was a problem hiding this comment.
I think it might still be useful to see the summary in case the table being balanced is not expected? wdyt?
I haven't moved it yet, but let me know if you still think it's not worth calculating the summary for this scenario and I can move it down and adjust the tests.
There was a problem hiding this comment.
make sense. Perhaps comment this for this if-branch so that others know the purpose and keep it here.
points 3,4,5 look great. |
… and servers getting new segments
Got it, updated the PR to add your suggestion for 1. Sample summary: Add server: Remove server: Also note that I've removed the time estimates for now as recommended by @klsince as it is hard to come up with a good estimate (different environments may have different speeds). We can perhaps come up with some heuristic based on some real rebalances if that makes sense in the future. |
| RebalanceSummaryResult summaryResult = null; | ||
| if (summary) { | ||
| summaryResult = calculateDryRunSummary(currentAssignment, targetAssignment, tableNameWithType, rebalanceJobId); | ||
| } |
There was a problem hiding this comment.
make sense. Perhaps comment this for this if-branch so that others know the purpose and keep it here.
…mmary of the dry-run results (apache#15050) * Add a dry-run summary mode for TableRebalance which only returns a summary of the dry-run results * Address review - add server level lists of added, removed, unchanged, and servers getting new segments
This PR adds a
summaryoption to the TableRebalance API which is meant to be used withdryRun. Ifsummaryis set totruea summary of the dry-run is returned rather than the full dry-run. This summary gives some stats about changes that will occur during the rebalance, such as:Today the dry-run output can be very large, and can be difficult to make sense of in terms of the changes occurring. It can also be difficult to display the full output. For now
summaryis not appended todryRunwithoutsummaryenabled, but this can be added if it makes sense to add a summary even for the usual result return.Note: This PR will have conflicts with #15029 and will need to be rebased once that is merged.
Sample JSON summary [NOTE - please see later comments for updated summary based on review comments]:
cc @Jackie-Jiang @klsince @deepthi912 @npawar