Prevent the table creation when the time column name config is invalid #6030
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
|
kishoreg
reviewed
Sep 17, 2020
| @@ -1226,6 +1231,27 @@ void validateTableTenantConfig(TableConfig tableConfig) { | |||
| } | |||
| } | |||
|
|
|||
| void validateTimeColumnConfig(TableConfig tableConfig, Schema schema) { | |||
There was a problem hiding this comment.
is this the right place? can we create a tableconfigvalidator @icefury71 you were also adding some validations right. lets move all validations related to tableconfig to one class
jackjlli
reviewed
Sep 17, 2020
Currently, there is no validation on the time column name from the table config when creating a realtime table. Invalid time configuration would cause the failure in commit stage because start/end metadata doesn't get added to the segment metadata. This adds the validation when creating the realtime table.
snleee
force-pushed
the
fix-realtime-table-time-column-config-issue
branch
from
b1f3309 to
b0c0f9d
Compare
npawar
reviewed
Sep 17, 2020
| @@ -504,6 +504,8 @@ private LLCRealtimeSegmentZKMetadata updateCommittingSegmentZKMetadata(String re | |||
| committingSegmentZKMetadata.setDownloadUrl(isPeerURL(committingSegmentDescriptor.getSegmentLocation()) | |||
| ? CommonConstants.Segment.METADATA_URI_FOR_PEER_DOWNLOAD : committingSegmentDescriptor.getSegmentLocation()); | |||
| committingSegmentZKMetadata.setCrc(Long.valueOf(segmentMetadata.getCrc())); | |||
| Preconditions.checkNotNull(segmentMetadata.getTimeInterval(), | |||
There was a problem hiding this comment.
can we log segment and table name here?
npawar
reviewed
Sep 17, 2020
| @@ -100,9 +101,13 @@ private static void validateValidationConfig(TableConfig tableConfig, @Nullable | |||
| } | |||
| // timeColumnName can be null in OFFLINE table | |||
| if (timeColumnName != null && schema != null) { | |||
| Preconditions.checkState(schema.getSpecForTimeColumn(timeColumnName) != null, | |||
| FieldSpec timeColumnFieldSpec = schema.getSpecForTimeColumn(timeColumnName); | |||
There was a problem hiding this comment.
There's a TableConfigUtilsTest which exhaustively tests validations done here. Can you add one for timeColumn being TIME/DATE_TIME only?
|
Closing the pull request since the current code correctly validates the time column that I would like to do. |
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.
Currently, there is no validation on the time column name from
the table config when creating a realtime table. Invalid time
configuration would cause the failure in commit stage because
start/end metadata doesn't get added to the segment metadata.
This adds the validation when creating the realtime table.