-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Marks end criteria reached for the segment if the Index cannot consume more rows #14479
Changes from 12 commits
f03043a
09d43d7
121d23a
06523b6
f47cb08
da8d691
2cff520
55bf033
4549e5c
36c5f0d
c2f78cb
0acb3f6
96e8663
ea48ba0
58370ca
fbc9c93
8722efc
a1467b5
27c4443
a2b95b6
6c3df28
c8f5ed2
ba59830
9f13ff1
b1143d5
33d40a5
be8ab39
3a185dd
d3a6ea2
ba07fc3
69b7e32
c60f1bd
5511e1f
ffa984d
783f8c0
f9e4b9c
3e1dde2
ed32e3d
e57d726
24243bd
541a7d9
9f67ce4
65fcaab
e6c1f43
dbaaf5c
d15a3fd
6960b5f
eb3662f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -309,6 +309,7 @@ public void deleteSegmentFile() { | |
private String _stopReason = null; | ||
private final Semaphore _segBuildSemaphore; | ||
private final boolean _isOffHeap; | ||
private final boolean _thresholdForNumOfColValuesEnabled; | ||
/** | ||
* Whether null handling is enabled by default. This value is only used if | ||
* {@link Schema#isEnableColumnBasedNullHandling()} is false. | ||
|
@@ -362,6 +363,13 @@ private boolean endCriteriaReached() { | |
_numRowsConsumed, _numRowsIndexed); | ||
_stopReason = SegmentCompletionProtocol.REASON_FORCE_COMMIT_MESSAGE_RECEIVED; | ||
return true; | ||
} else if (_thresholdForNumOfColValuesEnabled && _realtimeSegment.isNumOfColValuesAboveThreshold()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, so it is fairly easy to stop consumption and commit |
||
_segmentLogger.info( | ||
"Stopping consumption as num of values for a column is above threshold - numRowsConsumed={} " | ||
+ "numRowsIndexed={}", | ||
_numRowsConsumed, _numRowsIndexed); | ||
_stopReason = SegmentCompletionProtocol.REASON_NUM_OF_COL_VALUES_ABOVE_THRESHOLD; | ||
return true; | ||
} | ||
return false; | ||
|
||
|
@@ -1233,6 +1241,7 @@ private static class ConsumptionStopIndicator { | |
final Logger _logger; | ||
final ServerSegmentCompletionProtocolHandler _protocolHandler; | ||
final String _reason; | ||
|
||
private ConsumptionStopIndicator(StreamPartitionMsgOffset offset, String segmentName, String instanceId, | ||
ServerSegmentCompletionProtocolHandler protocolHandler, String reason, Logger logger) { | ||
_offset = offset; | ||
|
@@ -1529,6 +1538,7 @@ public RealtimeSegmentDataManager(SegmentZKMetadata segmentZKMetadata, TableConf | |
|
||
_isOffHeap = indexLoadingConfig.isRealtimeOffHeapAllocation(); | ||
_defaultNullHandlingEnabled = indexingConfig.isNullHandlingEnabled(); | ||
_thresholdForNumOfColValuesEnabled = tableConfig.getValidationConfig().isThresholdForNumOfColValuesEnabled(); | ||
|
||
// Start new realtime segment | ||
String consumerDir = realtimeTableDataManager.getConsumerDir(); | ||
|
@@ -1552,7 +1562,8 @@ public RealtimeSegmentDataManager(SegmentZKMetadata segmentZKMetadata, TableConf | |
.setUpsertDropOutOfOrderRecord(tableConfig.isDropOutOfOrderRecord()) | ||
.setPartitionDedupMetadataManager(partitionDedupMetadataManager) | ||
.setDedupTimeColumn(tableConfig.getDedupTimeColumn()) | ||
.setFieldConfigList(tableConfig.getFieldConfigList()); | ||
.setFieldConfigList(tableConfig.getFieldConfigList()) | ||
.setThresholdForNumOfColValuesEnabled(_thresholdForNumOfColValuesEnabled); | ||
|
||
// Create message decoder | ||
Set<String> fieldsToRead = IngestionUtils.getFieldsForRecordExtractor(_tableConfig.getIngestionConfig(), _schema); | ||
|
@@ -1625,7 +1636,7 @@ public RealtimeSegmentDataManager(SegmentZKMetadata segmentZKMetadata, TableConf | |
"Failed to initialize segment data manager", e)); | ||
_segmentLogger.warn( | ||
"Scheduling task to call controller to mark the segment as OFFLINE in Ideal State due" | ||
+ " to initialization error: '{}'", | ||
+ " to initialization error: '{}'", | ||
e.getMessage()); | ||
// Since we are going to throw exception from this thread (helix execution thread), the externalview | ||
// entry for this segment will be ERROR. We allow time for Helix to make this transition, and then | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,6 +128,7 @@ public class MutableSegmentImpl implements MutableSegment { | |
private static final int EXPECTED_COMPRESSION = 1000; | ||
private static final int MIN_ROWS_TO_INDEX = 1000_000; // Min size of recordIdMap for updatable metrics. | ||
private static final int MIN_RECORD_ID_MAP_CACHE_SIZE = 10000; // Min overflow map size for updatable metrics. | ||
private final static int DEFAULT_THRESHOLD_FOR_NUM_OF_VALUES_PER_COLUMN = 2_000_000_000; | ||
|
||
private final Logger _logger; | ||
private final long _startTimeMillis = System.currentTimeMillis(); | ||
|
@@ -147,8 +148,10 @@ public class MutableSegmentImpl implements MutableSegment { | |
private final int _mainPartitionId; // partition id designated for this consuming segment | ||
private final boolean _defaultNullHandlingEnabled; | ||
private final File _consumerDir; | ||
private final boolean _thresholdForNumOfColValuesEnabled; | ||
|
||
private final Map<String, IndexContainer> _indexContainerMap = new HashMap<>(); | ||
private boolean _numOfColValuesLimitBreached = false; | ||
|
||
private final IdMap<FixedIntArray> _recordIdMap; | ||
|
||
|
@@ -225,6 +228,7 @@ public boolean isMutableSegment() { | |
_mainPartitionId = config.getPartitionId(); | ||
_defaultNullHandlingEnabled = config.isNullHandlingEnabled(); | ||
_consumerDir = new File(config.getConsumerDir()); | ||
_thresholdForNumOfColValuesEnabled = config.isThresholdForNumOfColValuesEnabled(); | ||
|
||
Collection<FieldSpec> allFieldSpecs = _schema.getAllFieldSpecs(); | ||
List<FieldSpec> physicalFieldSpecs = new ArrayList<>(allFieldSpecs.size()); | ||
|
@@ -797,6 +801,18 @@ private void addNewRow(int docId, GenericRow row) { | |
recordIndexingError(indexEntry.getKey(), e); | ||
} | ||
} | ||
|
||
if (_thresholdForNumOfColValuesEnabled) { | ||
int prevCount = indexContainer._valuesInfo.getNumValues(); | ||
long newCount = prevCount + 1L + values.length; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Total values itself is not enough. We should perform a per-index check (add an api to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mutable index is unbounded right (No code enforced limit)? From the code I see Realtime Mutable index is always created with dictionary (even for MV VarByte col with noDict enabled in config). Hence from But we are more interested in the size of immutable index since that's where exception is being thrown. However Immutable index can be larger or even smaller than mutable index as implementation is completely diff. So while building mutable index we need to keep some state to estimate the approx size of the immutable version of the index (Like while building mutable fwd index, we need to keep an estimation of bitmap, numBitsPerValue, header size, etc).
So this might be adding too much complexity to every mutable index since now after every row consumption we need to update estimated size of the corresponding immutable index and it tight couples code with immutable index logic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Essentially what we want to ensure is that whatever accepted in mutable segment won't cause problem when the mutable segment is sealed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm. Okay, let me figure out if we can even do this on ingestion side considering we have 5 different implementations of immutable forward index for MV columns each having diff size limit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can keep threshold near 500 million for numOfValues which will be good enough for few immutable indexes but not for varByte fwd index having 4GB Limit (2GB wasted). However this will ensure we never encounter numOfValues overflow and index limit size reached exception. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For now we can figure out a conservative value that will never cause over 2GB buffer. Once we support larger index, we can increate this value There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. Will pick the task to keep the indexes unbounded post this. |
||
|
||
if (newCount > DEFAULT_THRESHOLD_FOR_NUM_OF_VALUES_PER_COLUMN) { | ||
_logger.warn("Number of total values for column {} is {} and has breached the threshold limit {}", | ||
column, newCount, DEFAULT_THRESHOLD_FOR_NUM_OF_VALUES_PER_COLUMN); | ||
_numOfColValuesLimitBreached = true; | ||
} | ||
} | ||
|
||
indexContainer._valuesInfo.updateMVNumValues(values.length); | ||
} | ||
} | ||
|
@@ -1229,6 +1245,10 @@ private boolean isAggregateMetricsEnabled() { | |
return _recordIdMap != null; | ||
} | ||
|
||
public boolean isNumOfColValuesAboveThreshold() { | ||
return _numOfColValuesLimitBreached; | ||
} | ||
|
||
// NOTE: Okay for single-writer | ||
@SuppressWarnings("NonAtomicOperationOnVolatileField") | ||
private static class ValuesInfo { | ||
|
@@ -1285,6 +1305,10 @@ void updateVarByteMVMaxRowLengthInBytes(Object entry, DataType dataType) { | |
throw new IllegalStateException("Invalid type=" + dataType); | ||
} | ||
} | ||
|
||
int getNumValues() { | ||
return _numValues; | ||
} | ||
} | ||
|
||
private static Map<String, Pair<String, ValueAggregator>> getMetricsAggregators(RealtimeSegmentConfig segmentConfig) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be always on. We may introduce a config to turn it off if we are not confident about this new logic, but if it is not very complicated we can remove this config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering new impln now where we are keeping conservative size estimation check as well, I guess it makes sense to enable this change only behind a flag?