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:10:36 UTC
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #7071: Clear legacy configs when converting to new TableConfig.
Jackie-Jiang commented on a change in pull request #7071:
URL: https://github.com/apache/incubator-pinot/pull/7071#discussion_r653959152
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java
##########
@@ -263,6 +262,14 @@ public static void convertFromLegacyTableConfig(TableConfig tableConfig) {
ingestionConfig.setStreamIngestionConfig(streamIngestionConfig);
}
+ // Set the new config fields.
tableConfig.setIngestionConfig(ingestionConfig);
+
+ // Clear the deprecated ones.
+ indexingConfig.setStreamConfigs(null);
+ tableConfig.setIndexingConfig(indexingConfig);
Review comment:
This line is redundant
##########
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:
(nit) No need to do null check here
##########
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);
Review comment:
Set `ingestionConfig` only when one of `batchIngestionConfig` and `streamIngestionConfig` is not `null`
##########
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:
(nit)
```suggestion
if (CollectionUtils.isNotEmpty(streamConfigs)) {
```
##########
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:
It should be `MapUtils` lol
--
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