add validation for upsert tables #6149
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6149 +/- ##
==========================================
+ Coverage 66.44% 72.74% +6.29%
==========================================
Files 1075 1231 +156
Lines 54773 58072 +3299
Branches 8168 8572 +404
==========================================
+ Hits 36396 42245 +5849
+ Misses 15700 13037 -2663
- Partials 2677 2790 +113
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -230,6 +234,38 @@ private static void validateIngestionConfig(@Nullable IngestionConfig ingestionC | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Validates the upsert-related configurations | |||
| * | |||
There was a problem hiding this comment.
Please add some javadoc on the checks performed here
| if (schema.getPrimaryKeyColumns() == null || schema.getPrimaryKeyColumns().isEmpty()) { | ||
| throw new IllegalStateException("Upsert table must have primary key columns in the schema."); | ||
| } |
There was a problem hiding this comment.
| if (schema.getPrimaryKeyColumns() == null || schema.getPrimaryKeyColumns().isEmpty()) { | |
| throw new IllegalStateException("Upsert table must have primary key columns in the schema."); | |
| } | |
| Preconditions.checkState(CollectionUtils.isNotEmpty(schema.getPrimaryKeyColumns()), "Upsert table must have primary key columns in the schema"); |
There was a problem hiding this comment.
I saw some checks in this class use precondition, others use IllegalStateException. Shall we make it consistent? @icefury71
There was a problem hiding this comment.
Will do so in my next pass
| if (tableConfig.getRoutingConfig() == null || !tableConfig.getRoutingConfig().getInstanceSelectorType() | ||
| .equalsIgnoreCase(RoutingConfig.REPLICA_GROUP_INSTANCE_SELECTOR_TYPE)) { | ||
| throw new IllegalStateException("Upsert table must use replicaGroup as the routing config."); | ||
| } |
There was a problem hiding this comment.
This can potentially have NPE
| if (tableConfig.getRoutingConfig() == null || !tableConfig.getRoutingConfig().getInstanceSelectorType() | |
| .equalsIgnoreCase(RoutingConfig.REPLICA_GROUP_INSTANCE_SELECTOR_TYPE)) { | |
| throw new IllegalStateException("Upsert table must use replicaGroup as the routing config."); | |
| } | |
| Preconditions.checkState(tableConfig.getRoutingConfig() != null && RoutingConfig.REPLICA_GROUP_INSTANCE_SELECTOR_TYPE. equalsIgnoreCase(tableConfig.getRoutingConfig().getInstanceSelectorType()), "Upsert table must use replica-group based routing") |
There was a problem hiding this comment.
why? this seems the same rewrite.
There was a problem hiding this comment.
tableConfig.getRoutingConfig().getInstanceSelectorType() might be null
There was a problem hiding this comment.
then it's the same :)
| if (streamConfig.getConsumerTypes().size() != 1 | ||
| || streamConfig.getConsumerTypes().get(0) != StreamConfig.ConsumerType.LOWLEVEL) { | ||
| throw new IllegalStateException("Upsert table must use low-level streaming consumer type."); | ||
| } |
There was a problem hiding this comment.
| if (streamConfig.getConsumerTypes().size() != 1 | |
| || streamConfig.getConsumerTypes().get(0) != StreamConfig.ConsumerType.LOWLEVEL) { | |
| throw new IllegalStateException("Upsert table must use low-level streaming consumer type."); | |
| } | |
| Preconditions.checkState(streamConfig.hasLowLevelConsumerType() && !streamConfig.hasHighLevelConsumerType(), ""Upsert table must use low-level streaming consumer type"); |
| } | ||
| } | ||
| // check type type is realtime | ||
| if (tableConfig.getTableType() != TableType.REALTIME) { |
There was a problem hiding this comment.
Use precondition, also recommend reorder the checks in this order: upsert mode -> primary key -> table type -> consumer type -> routing type, which IMO is more intuitive
|
Can you add unit test for this as well please ? Look at TableConfigUtilsTest |
| } | ||
| } | ||
| // check type type is realtime | ||
| if (tableConfig.getTableType() != TableType.REALTIME) { |
There was a problem hiding this comment.
nit: do this in the beginning ?
|
@icefury71 @Jackie-Jiang thanks for the review. Comments addressed. |
| return; | ||
| } | ||
| // check table type is realtime | ||
| if (tableConfig.getTableType() != TableType.REALTIME) { |
There was a problem hiding this comment.
(nit) Use precondition for this as well
Description
Part of a series of PRs for #4261
Check this doc out for the new design