You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "jackjlli (via GitHub)" <gi...@apache.org> on 2024/04/09 00:18:29 UTC
[PR] Add validation check for forward index disabled when enabling columnar segment creation [pinot]
jackjlli opened a new pull request, #12838:
URL: https://github.com/apache/pinot/pull/12838
Now that column major segment builder is enabled by default in this PR (https://github.com/apache/pinot/pull/12770), the validation check should be added on cases where the forward index is disabled. Otherwise, the segment generation would fail and be paused. Thus, it'd be great to capture this early when the table config is added/updated.
This PR aims to add the validation check for forward index disabled when enabling columnar segment creation.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] Add validation check for forward index disabled when enabling columnar segment creation [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #12838:
URL: https://github.com/apache/pinot/pull/12838#issuecomment-2046090959
What I was asking is when we disable forward index, will the config be picked up for consuming segment? Will the consuming segment only create inverted index, but not forward index?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] Add validation check for forward index disabled when enabling columnar segment creation [pinot]
Posted by "jackjlli (via GitHub)" <gi...@apache.org>.
jackjlli commented on PR #12838:
URL: https://github.com/apache/pinot/pull/12838#issuecomment-2046113187
Right, I was about to paste you the same [link](https://github.com/apache/pinot/blob/master/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java#L1304). Since forward index is always enabled by default, I don't see a reason why it's explicitly set to be disabled for realtime tables. And since with `columnMajorSegmentBuilderEnabled` is on by default, any realtime segment completion will fail eventually, I think it makes more sense to fail it fast in order to bring the attention to the table owners/pinot admins. @Jackie-Jiang wdyt?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] Add validation check for forward index disabled when enabling columnar segment creation [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #12838:
URL: https://github.com/apache/pinot/pull/12838#issuecomment-2046098039
I don't think forward index can be disabled for mutable segment (see this [line](https://github.com/apache/pinot/blob/425eea9e80dd81c633481488a12c1c62cc7a2c91/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java#L1304)), not sure how it is handled right now
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] Add validation check for forward index disabled when enabling columnar segment creation [pinot]
Posted by "jackjlli (via GitHub)" <gi...@apache.org>.
jackjlli commented on PR #12838:
URL: https://github.com/apache/pinot/pull/12838#issuecomment-2046083662
> Do we allow disabling forward index for consuming segment? Can we add a test to verify if it works?
With the change in this current PR no we won't allow disabling forward index for consuming segment as it will fail on table creation. And `columnMajorSegmentBuilderEnabled` is set to true by default in the previous [PR](https://github.com/apache/pinot/pull/12770). And in fact, columnar segment generation won't work with consuming segment whose column has forward index disabled. Even without adding the validation check, the columnar segment generation would still fail (check [this line of code](https://github.com/apache/pinot/blob/master/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentColumnReader.java#L48)). The purpose of this PR is to fail fast instead of waiting for a segment commit request.
Regarding tests, I think the ones in `TableConfigUtilsTest` would be sufficient as in this PR the failure would be detected during table creation?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] Add validation check for forward index disabled when enabling columnar segment creation [pinot]
Posted by "jackjlli (via GitHub)" <gi...@apache.org>.
jackjlli commented on PR #12838:
URL: https://github.com/apache/pinot/pull/12838#issuecomment-2048413919
Ah that's good point! Adjusted the logic to only check for the table type in the latest push. @Jackie-Jiang PTAL. Thanks!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] Add validation check for forward index disabled when enabling columnar segment creation [pinot]
Posted by "jackjlli (via GitHub)" <gi...@apache.org>.
jackjlli commented on code in PR #12838:
URL: https://github.com/apache/pinot/pull/12838#discussion_r1567924010
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -171,7 +171,8 @@ public static void validate(TableConfig tableConfig, @Nullable Schema schema, @N
}
validateTierConfigList(tableConfig.getTierConfigsList());
validateIndexingConfig(tableConfig.getIndexingConfig(), schema);
- validateFieldConfigList(tableConfig.getFieldConfigList(), tableConfig.getIndexingConfig(), schema);
+ validateFieldConfigList(tableConfig.getFieldConfigList(), tableConfig.getIndexingConfig(), schema,
Review Comment:
Updated.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] Add validation check for forward index disabled when enabling columnar segment creation [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #12838:
URL: https://github.com/apache/pinot/pull/12838#issuecomment-2048378643
Yeah, so basically we can add a check that forward index cannot be disabled for real-time table. It should have nothing to do with enabling columnar segment creation.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] Add validation check for forward index disabled when enabling columnar segment creation [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12838:
URL: https://github.com/apache/pinot/pull/12838#discussion_r1563214632
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -171,7 +171,8 @@ public static void validate(TableConfig tableConfig, @Nullable Schema schema, @N
}
validateTierConfigList(tableConfig.getTierConfigsList());
validateIndexingConfig(tableConfig.getIndexingConfig(), schema);
- validateFieldConfigList(tableConfig.getFieldConfigList(), tableConfig.getIndexingConfig(), schema);
+ validateFieldConfigList(tableConfig.getFieldConfigList(), tableConfig.getIndexingConfig(), schema,
Review Comment:
I'd rather directly pass in `tableConfig` and `schema`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] Add validation check for forward index disabled when enabling columnar segment creation [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #12838:
URL: https://github.com/apache/pinot/pull/12838#issuecomment-2046124115
My point is that forward index cannot be disabled for consuming segment anyway, regardless of whether `columnMajorSegmentBuilderEnabled` is enabled or not. Have you tried disabling forward index for any REALTIME table? Does it work?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] Add validation check for forward index disabled when enabling columnar segment creation [pinot]
Posted by "jackjlli (via GitHub)" <gi...@apache.org>.
jackjlli commented on PR #12838:
URL: https://github.com/apache/pinot/pull/12838#issuecomment-2046224113
I did test out the cases when forward index is disabled for the realtime table. And here is the exception messages:
```
2024/04/09 16:31:42.220 INFO [FixedByteSVMutableForwardIndex] [HelixTaskExecutor-message_handle_thread_55] Allocating 200 bytes for: airlineStats__7__1__20240409T2331Z:$ts$DAY.sv.unsorted.fwd
2024/04/09 16:31:42.220 INFO [FixedByteSVMutableForwardIndex] [HelixTaskExecutor-message_handle_thread_55] Allocating 512 bytes for: airlineStats__7__1__20240409T2331Z:$ts$MONTH.dict
2024/04/09 16:31:42.220 INFO [FixedByteSVMutableForwardIndex] [HelixTaskExecutor-message_handle_thread_55] Allocating 200 bytes for: airlineStats__7__1__20240409T2331Z:$ts$MONTH.sv.unsorted.fwd
2024/04/09 16:31:42.220 INFO [FixedByteSVMutableForwardIndex] [HelixTaskExecutor-message_handle_thread_55] Allocating 512 bytes for: airlineStats__7__1__20240409T2331Z:$ts$WEEK.dict
2024/04/09 16:31:42.220 INFO [FixedByteSVMutableForwardIndex] [HelixTaskExecutor-message_handle_thread_55] Allocating 200 bytes for: airlineStats__7__1__20240409T2331Z:$ts$WEEK.sv.unsorted.fwd
2024/04/09 16:31:42.220 INFO [FixedByteSVMutableForwardIndex] [HelixTaskExecutor-message_handle_thread_55] Allocating 256 bytes for: airlineStats__7__1__20240409T2331Z:ActualElapsedTime.dict
2024/04/09 16:31:42.220 INFO [FixedByteSVMutableForwardIndex] [HelixTaskExecutor-message_handle_thread_55] Allocating 200 bytes for: airlineStats__7__1__20240409T2331Z:ActualElapsedTime.sv.unsorted.fwd
2024/04/09 16:31:42.221 INFO [FixedByteSVMutableForwardIndex] [HelixTaskExecutor-message_handle_thread_55] Allocating 256 bytes for: airlineStats__7__1__20240409T2331Z:AirTime.dict
...
2024/04/09 16:31:42.232 WARN [RealtimeSegmentDataManager_airlineStats__6__1__20240409T2331Z] [HelixTaskExecutor-message_handle_thread_54] Scheduling task to call controller to mark the segment as OFFLINE in Ideal State due to initialization error: 'Forward index is required'
2024/04/09 16:31:42.232 ERROR [216_7050 - SegmentOnlineOfflineStateModel] [HelixTaskExecutor-message_handle_thread_55] Caught exception in state transition OFFLINE -> CONSUMING for table: airlineStats_REALTIME, segment: airlineStats__7__1__20240409T2331Z
java.lang.IllegalArgumentException: Forward index is required
at org.apache.pinot.shaded.com.google.common.base.Preconditions.checkArgument(Preconditions.java:145) ~[pinot-all-1.2.0-SNAPSHOT-jar-with-dependencies.jar:1.2.0-SNAPSHOT-c7f5887d55981764646d873b836eaed384ed4339]
at org.apache.pinot.segment.local.indexsegment.mutable.MutableSegmentImpl$IndexContainer.<init>(MutableSegmentImpl.java:1304) ~[pinot-all-1.2.0-SNAPSHOT-jar-with-dependencies.jar:1.2.0-SNAPSHOT-c7f5887d55981764646d873b836eaed384ed4339]
at org.apache.pinot.segment.local.indexsegment.mutable.MutableSegmentImpl.<init>(MutableSegmentImpl.java:364) ~[pinot-all-1.2.0-SNAPSHOT-jar-with-dependencies.jar:1.2.0-SNAPSHOT-c7f5887d55981764646d873b836eaed384ed4339]
at org.apache.pinot.core.data.manager.realtime.RealtimeSegmentDataManager.<init>(RealtimeSegmentDataManager.java:1529) ~[pinot-all-1.2.0-SNAPSHOT-jar-with-dependencies.jar:1.2.0-SNAPSHOT-c7f5887d55981764646d873b836eaed384ed4339]
at org.apache.pinot.core.data.manager.realtime.RealtimeTableDataManager.addSegment(RealtimeTableDataManager.java:462) ~[pinot-all-1.2.0-SNAPSHOT-jar-with-dependencies.jar:1.2.0-SNAPSHOT-c7f5887d55981764646d873b836eaed384ed4339]
at org.apache.pinot.server.starter.helix.HelixInstanceDataManager.addRealtimeSegment(HelixInstanceDataManager.java:241) ~[pinot-all-1.2.0-SNAPSHOT-jar-with-dependencies.jar:1.2.0-SNAPSHOT-c7f5887d55981764646d873b836eaed384ed4339]
at org.apache.pinot.server.starter.helix.SegmentOnlineOfflineStateModelFactory$SegmentOnlineOfflineStateModel.onBecomeConsumingFromOffline(SegmentOnlineOfflineStateModelFactory.java:91) ~[pinot-all-1.2.0-SNAPSHOT-jar-with-dependencies.jar:1.2.0-SNAPSHOT-c7f5887d55981764646d873b836eaed384ed4339]
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) ~[?:?]
at java.base/java.lang.reflect.Method.invoke(Method.java:580) ~[?:?]
at org.apache.helix.messaging.handling.HelixStateTransitionHandler.invoke(HelixStateTransitionHandler.java:350) ~[pinot-all-1.2.0-SNAPSHOT-jar-with-dependencies.jar:1.2.0-SNAPSHOT-c7f5887d55981764646d873b836eaed384ed4339]
at org.apache.helix.messaging.handling.HelixStateTransitionHandler.handleMessage(HelixStateTransitionHandler.java:278) ~[pinot-all-1.2.0-SNAPSHOT-jar-with-dependencies.jar:1.2.0-SNAPSHOT-c7f5887d55981764646d873b836eaed384ed4339]
at org.apache.helix.messaging.handling.HelixTask.call(HelixTask.java:97) ~[pinot-all-1.2.0-SNAPSHOT-jar-with-dependencies.jar:1.2.0-SNAPSHOT-c7f5887d55981764646d873b836eaed384ed4339]
at org.apache.helix.messaging.handling.HelixTask.call(HelixTask.java:49) ~[pinot-all-1.2.0-SNAPSHOT-jar-with-dependencies.jar:1.2.0-SNAPSHOT-c7f5887d55981764646d873b836eaed384ed4339]
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317) ~[?:?]
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) ~[?:?]
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) ~[?:?]
at java.base/java.lang.Thread.run(Thread.java:1583) [?:?]
```
updated the fieldConfig to the realtime table config:
```
@@ -55,6 +57,19 @@
"MONTH"
]
}
+ },
+ {
+ "name": "AirTime",
+ "encodingType": "DICTIONARY",
+ "indexType": "INVERTED",
+ "indexTypes": [
+ "INVERTED"
+ ],
+ "indexes": null,
+ "properties": {
+ "forwardIndexDisabled": "true"
+ },
+ "tierOverwrites": null
}
],
"metadata": {
```
So I think it still makes sense to fail fast on table config update.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] Add validation check for forward index disabled when enabling columnar segment creation [pinot]
Posted by "jackjlli (via GitHub)" <gi...@apache.org>.
jackjlli merged PR #12838:
URL: https://github.com/apache/pinot/pull/12838
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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