Add a check to enable size based threshold for realtime tables #12016
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 ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #12016 +/- ##
=============================================
- Coverage 61.69% 27.57% -34.12%
Complexity 207 207
=============================================
Files 2385 2385
Lines 129352 129357 +5
Branches 20029 20032 +3
=============================================
- Hits 79799 35669 -44130
- Misses 43731 90998 +47267
+ Partials 5822 2690 -3132
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
walterddr
reviewed
Nov 17, 2023
Comment on lines
290
to
297
| streamConfigMap.put(StreamConfigProperties.SEGMENT_FLUSH_THRESHOLD_ROWS, "1000000"); | ||
| streamConfigMap.put(StreamConfigProperties.SEGMENT_FLUSH_THRESHOLD_SEGMENT_SIZE, "100M"); | ||
| try { | ||
| new StreamConfig(tableName, streamConfigMap); | ||
| fail("Invalid config: flush threshold rows must be 0, when flush threshold size is set."); | ||
| } catch (Exception e) { | ||
| // Expected | ||
| } |
There was a problem hiding this comment.
i am not sure this is what the document intended to mean
what i read was:
You can pick the appropriate value for segment size and number of hours in the table config, and set the number of rows to zero. Note that you don't have to pick values exactly as given in each of these combinations (they are calculated guesses anyway).
it wouldn't throw exception if num rows is set to non-zero.
my concern is that this will cause issue during cluster upgrade. if any user have both configuration set table will be unable to load in an error state .
| @@ -157,7 +157,8 @@ public static void validate(TableConfig tableConfig, @Nullable Schema schema, @N | |||
| // Validate that StreamConfig can be created | |||
| streamConfig = new StreamConfig(tableConfig.getTableName(), streamConfigMap); | |||
| } catch (Exception e) { | |||
| throw new IllegalStateException("Could not create StreamConfig using the streamConfig map", e); | |||
| throw new IllegalStateException("Could not create StreamConfig using the streamConfig map: " + e.getMessage(), | |||
There was a problem hiding this comment.
Since we already included the exception, do not add message again
| @@ -172,7 +172,7 @@ public StreamConfig(String tableNameWithType, Map<String, String> streamConfigMa | |||
|
|
|||
| _flushThresholdRows = extractFlushThresholdRows(streamConfigMap); | |||
| _flushThresholdTimeMillis = extractFlushThresholdTimeMillis(streamConfigMap); | |||
| _flushThresholdSegmentSizeBytes = extractFlushThresholdSegmentSize(streamConfigMap); | |||
| _flushThresholdSegmentSizeBytes = extractFlushThresholdSegmentSize(streamConfigMap, _flushThresholdRows); | |||
There was a problem hiding this comment.
Do not add check within the constructor because it will break existing table configs. For new checks we want to enforce, add it into TableConfigUtils.validate()
…ment.flush.threshold.rows must be set to 0 for size base flush threshold updater to be active.
…reation only. Existing tables will continue to work.
soumitra-st
force-pushed
the
check-size-based-threshold
branch
from
9046668
to
725d400
Compare
Jackie-Jiang
approved these changes
Nov 18, 2023
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.
As per the Pinot document, to enable the segment size based threshold rows must be set to 0. Adding a check during table creation, if realtime.segment.flush.threshold.segment.size is set, realtime.segment.flush.threshold.rows must be set to 0.
Added a test, and also ran below curl command to ensure if the table config has invalid combination, then the API fails:
% curl -X POST http://localhost:9000/tables -H 'accept: application/json' -H 'Content-Type: application/json' -d @/Users/soumitra/pinot-tutorial/transcript/transcript-table-realtime.json
{"code":400,"error":"Invalid config: realtime.segment.flush.threshold.rows=50000, it must be set to 0 for size based segment threshold to work."}
backward-incompat