[PINOT-6701] Controller API to check storage quota before segment upload #3068
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #3068 +/- ##
============================================
+ Coverage 70.29% 70.34% +0.05%
Complexity 4 4
============================================
Files 935 936 +1
Lines 43471 43511 +40
Branches 5894 5901 +7
============================================
+ Hits 30556 30606 +50
+ Misses 10869 10845 -24
- Partials 2046 2060 +14
Continue to review full report at Codecov.
|
jackjlli
reviewed
Aug 17, 2018
| public void testUploadQuotaCheck() throws Exception { | ||
| try { | ||
| // upload 1G | ||
| sendGetRequest(_controllerBaseApiUrl + "/tables/mytable/checkQuotaForUpload?size=1000000000"); |
There was a problem hiding this comment.
Please add an Assert.fail(); statement after this line.
| @ApiResponse(code = 413, message = "Proposed upload size too large"), | ||
| @ApiResponse(code = 500, message = "Internal server error")}) | ||
| public SuccessResponse checkQuotaForUpload( | ||
| @ApiParam(value = "Table name without type", required = true, example = "myTable") |
There was a problem hiding this comment.
It'd be good to add some logic to check whether the input tableName contains suffix.
| ZKMetadataProvider.getOfflineTableConfig(_pinotHelixResourceManager.getPropertyStore(), offlineTableName); | ||
| if (offlineTableConfig == null) { | ||
| throw new ControllerApplicationException(LOGGER, | ||
| "Table: " + tableName + "not found", Response.Status.NOT_FOUND); |
There was a problem hiding this comment.
Add a space before "not found".
| if (!offlineTableConfig.getIndexingConfig().isCreateInvertedIndexDuringSegmentGeneration()) { | ||
| if (!offlineTableConfig.getIndexingConfig().getInvertedIndexColumns().isEmpty()) { | ||
| size = size + (long)(size * INDEX_RATIO); | ||
| LOGGER.info("Size (in bytes) estimate with indices included is ", size); |
There was a problem hiding this comment.
Please log the tableName here as well.
| } | ||
|
|
||
| if (tableSizeDetails.estimatedSizeInBytes + uploadSizeForAllReplicas > configured) { | ||
| String message = "Upload of " + DataSize.fromBytes(size) + " will exceed quota. Estimated append size " |
There was a problem hiding this comment.
I think it's best to use String.format() to form the String.
|
Closed this PR due to 6 months inactivity. Reopen if needed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Expose a controller API to check storage quota before segments are uploaded. The API expects, as a query parameter, the uncompressed size of all segments that are to be uploaded. The API accounts for the appropriate push-type (refresh vs append) and whether the segments to be uploaded have indices or not and returns whether the upload can go through or not.
Eventually, we will drop the quota checks that are done per segment and expect all push jobs to call this API before starting the upload. That will save a lot of unnecessary calls to the servers for get table sizes as well as prevent partial uploads if the size is exceeded mid way through the segments push.
Testing Done: added integration test that covers the append use-case upload. Plan to add more after initial review.