Skip task validation during table creation with schema #14683
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #14683 +/- ##
============================================
+ Coverage 61.75% 64.07% +2.31%
- Complexity 207 1607 +1400
============================================
Files 2436 2706 +270
Lines 133233 149164 +15931
Branches 20636 22864 +2228
============================================
+ Hits 82274 95572 +13298
- Misses 44911 46593 +1682
- Partials 6048 6999 +951
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
We should also handle this case in /tableConfigs/validate endpoint. But problem there is we cannot revalidate it as schema is never registered.
IMO let's just add the new interface method with schema parameter and deprecate the existing one as its only introduced last month so hopefully the usage is limited
| // Skip validate task type for table creation as the schema is not yet registered. | ||
| validateConfig(tableConfigs, databaseName, "TASK"); | ||
| } else { | ||
| validateConfig(tableConfigs, databaseName, typesToSkip); |
There was a problem hiding this comment.
We should append ",TASK" here to typesToSkip?
Looks like typesToSkip string is not parsed properly in validateTaskConfigs and assumed that only one type is passed while the endpoint expects comma separated list of types.
There was a problem hiding this comment.
right, there is only full string match inside, I feel some parsing logic is wrong there.
|
|
||
| if (typesToSkip == null) { | ||
| // Validate config again with schema for validations requires schema. | ||
| validateConfig(tableConfigs, databaseName, null); |
There was a problem hiding this comment.
Do we need to revert the schema creation upon validation failure as it will fail the request?
There was a problem hiding this comment.
yes, the catch clause will revert.
There was a problem hiding this comment.
Ah sorry missed that
I'm with you, I think let me make the interface change to allow an optional schema. The validation logic could fetch it from zk if it's null. |
xiangfu0
force-pushed
the
skip-table-validation-for-table-creation
branch
2 times, most recently
from
f4bc0fe to
5fce882
Compare
xiangfu0
force-pushed
the
skip-table-validation-for-table-creation
branch
2 times, most recently
from
4036269 to
c59305c
Compare
xiangfu0
force-pushed
the
skip-table-validation-for-table-creation
branch
from
c59305c to
f14dc59
Compare
How to reproduce:
Create a realtime table along with schema using the pinot-admin AddTable command. The table config should contain the task:
RealtimeToOfflineSegmentsTask.Reason:
For the table creation with schema, the task config validation might fail due to some tasks validation requires schema which is null when calling from the zookeeper.
The fix is to skip the task validation before the schema upload then validate it again.
The ultimate fix is to change the TaskGenerator interface to put both schema and tableConfig in it.