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 2021/06/18 21:21:50 UTC
[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #7071: Clear legacy configs when converting to new TableConfig.
mayankshriv commented on a change in pull request #7071:
URL: https://github.com/apache/incubator-pinot/pull/7071#discussion_r654105233
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java
##########
@@ -235,34 +233,56 @@ public static void convertFromLegacyTableConfig(TableConfig tableConfig) {
(ingestionConfig != null) ? ingestionConfig.getBatchIngestionConfig() : null;
SegmentsValidationAndRetentionConfig validationConfig = tableConfig.getValidationConfig();
+ String segmentPushType = validationConfig.getSegmentPushType();
+ String segmentPushFrequency = validationConfig.getSegmentPushFrequency();
+
if (batchIngestionConfig == null) {
- batchIngestionConfig = new BatchIngestionConfig(null, validationConfig.getSegmentPushType(),
- validationConfig.getSegmentPushFrequency());
+ // Only create the config if any of the deprecated config is not null.
+ if (segmentPushType != null || segmentPushFrequency != null) {
+ batchIngestionConfig = new BatchIngestionConfig(null, segmentPushType, segmentPushFrequency);
+ }
} else {
// This should not happen typically, but since we are in repair mode, might as well cover this corner case.
if (batchIngestionConfig.getSegmentIngestionType() == null) {
- batchIngestionConfig.setSegmentIngestionType(validationConfig.getSegmentPushType());
+ batchIngestionConfig.setSegmentIngestionType(segmentPushType);
}
if (batchIngestionConfig.getSegmentIngestionFrequency() == null) {
- batchIngestionConfig.setSegmentIngestionFrequency(validationConfig.getSegmentPushFrequency());
+ batchIngestionConfig.setSegmentIngestionFrequency(segmentPushFrequency);
}
}
StreamIngestionConfig streamIngestionConfig =
(ingestionConfig != null) ? ingestionConfig.getStreamIngestionConfig() : null;
+ IndexingConfig indexingConfig = tableConfig.getIndexingConfig();
+
if (streamIngestionConfig == null) {
- streamIngestionConfig =
- new StreamIngestionConfig(Collections.singletonList(tableConfig.getIndexingConfig().getStreamConfigs()));
+ Map<String, String> streamConfigs = indexingConfig.getStreamConfigs();
+
+ // Only set the new config if the deprecated one is set.
+ if (streamConfigs != null && !streamConfigs.isEmpty()) {
+ streamIngestionConfig = new StreamIngestionConfig(Collections.singletonList(streamConfigs));
+ }
}
if (ingestionConfig == null) {
ingestionConfig = new IngestionConfig(batchIngestionConfig, streamIngestionConfig, null, null, null);
} else {
- ingestionConfig.setBatchIngestionConfig(batchIngestionConfig);
- ingestionConfig.setStreamIngestionConfig(streamIngestionConfig);
+ if (batchIngestionConfig != null) {
+ ingestionConfig.setBatchIngestionConfig(batchIngestionConfig);
+ }
+
+ if (streamIngestionConfig != null) {
+ ingestionConfig.setStreamIngestionConfig(streamIngestionConfig);
+ }
Review comment:
I mean we added an `if` for each line of the code after review, one more doesn't hurt ;-).
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java
##########
@@ -235,34 +233,56 @@ public static void convertFromLegacyTableConfig(TableConfig tableConfig) {
(ingestionConfig != null) ? ingestionConfig.getBatchIngestionConfig() : null;
SegmentsValidationAndRetentionConfig validationConfig = tableConfig.getValidationConfig();
+ String segmentPushType = validationConfig.getSegmentPushType();
+ String segmentPushFrequency = validationConfig.getSegmentPushFrequency();
+
if (batchIngestionConfig == null) {
- batchIngestionConfig = new BatchIngestionConfig(null, validationConfig.getSegmentPushType(),
- validationConfig.getSegmentPushFrequency());
+ // Only create the config if any of the deprecated config is not null.
+ if (segmentPushType != null || segmentPushFrequency != null) {
+ batchIngestionConfig = new BatchIngestionConfig(null, segmentPushType, segmentPushFrequency);
+ }
} else {
// This should not happen typically, but since we are in repair mode, might as well cover this corner case.
if (batchIngestionConfig.getSegmentIngestionType() == null) {
- batchIngestionConfig.setSegmentIngestionType(validationConfig.getSegmentPushType());
+ batchIngestionConfig.setSegmentIngestionType(segmentPushType);
}
if (batchIngestionConfig.getSegmentIngestionFrequency() == null) {
- batchIngestionConfig.setSegmentIngestionFrequency(validationConfig.getSegmentPushFrequency());
+ batchIngestionConfig.setSegmentIngestionFrequency(segmentPushFrequency);
}
}
StreamIngestionConfig streamIngestionConfig =
(ingestionConfig != null) ? ingestionConfig.getStreamIngestionConfig() : null;
+ IndexingConfig indexingConfig = tableConfig.getIndexingConfig();
+
if (streamIngestionConfig == null) {
- streamIngestionConfig =
- new StreamIngestionConfig(Collections.singletonList(tableConfig.getIndexingConfig().getStreamConfigs()));
+ Map<String, String> streamConfigs = indexingConfig.getStreamConfigs();
+
+ // Only set the new config if the deprecated one is set.
+ if (streamConfigs != null && !streamConfigs.isEmpty()) {
Review comment:
Apparently, the suggested api does not take a Map.
--
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.
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