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 2022/12/01 01:20:46 UTC
[GitHub] [pinot] Jackie-Jiang opened a new pull request, #9885: Fix issues for realtime table reload
Jackie-Jiang opened a new pull request, #9885:
URL: https://github.com/apache/pinot/pull/9885
Fix #9761
Fix 2 issues:
1. When loading the just committed segment, we should pull the latest table config
2. Ignore the invalid freshness value from the empty consuming segment (before the first message being ingested)
--
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
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #9885: Fix issues for realtime table reload
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #9885:
URL: https://github.com/apache/pinot/pull/9885#discussion_r1042777559
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeTableDataManager.java:
##########
@@ -538,10 +538,14 @@ public void replaceHLSegment(SegmentZKMetadata segmentZKMetadata, IndexLoadingCo
* Replaces a committed LLC REALTIME segment.
*/
public void replaceLLSegment(String segmentName, IndexLoadingConfig indexLoadingConfig) {
+ File indexDir = new File(_indexDir, segmentName);
+ // Use the latest table config and schema to load the segment
+ TableConfig tableConfig = ZKMetadataProvider.getTableConfig(_propertyStore, _tableNameWithType);
+ Preconditions.checkState(tableConfig != null, "Failed to get table config for table: {}", _tableNameWithType);
+ Schema schema = ZKMetadataProvider.getTableSchema(_propertyStore, tableConfig);
+ indexLoadingConfig.updateTableConfigAndSchema(tableConfig, schema);
Review Comment:
If we check the implementation of `updateTableConfigAndSchema()`, we see that only additional indexing configs are added. For example:
1. If the user removes the inverted index on columnA and updates table config, the change would never be reflected here. That's because we only add new columns where the index is enabled.
2. The same is true for others like dictionary, etc.
I can propose a change to use the tableConfig as source of truth but just want to confirm that my understanding is right @Jackie-Jiang
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #9885: Fix issues for realtime table reload
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9885:
URL: https://github.com/apache/pinot/pull/9885#discussion_r1036602848
##########
pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java:
##########
@@ -210,40 +209,38 @@ private InstanceResponseBlock executeInternal(ServerQueryRequest queryRequest, E
}
// Gather stats for realtime consuming segments
+ // TODO: the freshness time should not be collected at query time because there is no guarantee that the consuming
+ // segment is queried (consuming segment might be pruned, or the server only contains relocated committed
+ // segments)
Review Comment:
CC @vvivekiyer ^
--
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
[GitHub] [pinot] Jackie-Jiang merged pull request #9885: Fix issues for realtime table reload
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged PR #9885:
URL: https://github.com/apache/pinot/pull/9885
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9885: Fix issues for realtime table reload
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9885:
URL: https://github.com/apache/pinot/pull/9885#discussion_r1042784136
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeTableDataManager.java:
##########
@@ -538,10 +538,14 @@ public void replaceHLSegment(SegmentZKMetadata segmentZKMetadata, IndexLoadingCo
* Replaces a committed LLC REALTIME segment.
*/
public void replaceLLSegment(String segmentName, IndexLoadingConfig indexLoadingConfig) {
+ File indexDir = new File(_indexDir, segmentName);
+ // Use the latest table config and schema to load the segment
+ TableConfig tableConfig = ZKMetadataProvider.getTableConfig(_propertyStore, _tableNameWithType);
+ Preconditions.checkState(tableConfig != null, "Failed to get table config for table: {}", _tableNameWithType);
+ Schema schema = ZKMetadataProvider.getTableSchema(_propertyStore, tableConfig);
+ indexLoadingConfig.updateTableConfigAndSchema(tableConfig, schema);
Review Comment:
Yes, index removal is not properly handled here.
I think the easy fix is to add `InstanceDataManagerConfig getInstanceDataManagerConfig()` into `IndexLoadingConfig`, and create a new `IndexLoadingConfig` here.
Currently `IndexLoadingConfig` takes `InstanceDataManagerConfig`, `TableConfig` and `Schema`, then re-create all configs again from them. Ideally we can remove it or make it a thin wrapper on top of these configs, which will prevent such problems, and make it much easier to manage
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #9885: Fix issues for realtime table reload
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9885:
URL: https://github.com/apache/pinot/pull/9885#issuecomment-1333051752
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9885?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#9885](https://codecov.io/gh/apache/pinot/pull/9885?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (47fcf3f) into [master](https://codecov.io/gh/apache/pinot/commit/041865a80f7a2359270571a2343049bb1f294fe5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (041865a) will **decrease** coverage by `34.07%`.
> The diff coverage is `57.14%`.
```diff
@@ Coverage Diff @@
## master #9885 +/- ##
=============================================
- Coverage 68.65% 34.57% -34.08%
+ Complexity 5049 190 -4859
=============================================
Files 1973 1973
Lines 106008 106007 -1
Branches 16060 16064 +4
=============================================
- Hits 72775 36655 -36120
- Misses 28110 66249 +38139
+ Partials 5123 3103 -2020
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.97% <57.14%> (-0.08%)` | :arrow_down: |
| unittests1 | `?` | |
| unittests2 | `15.80% <0.00%> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9885?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...local/segment/index/loader/IndexLoadingConfig.java](https://codecov.io/gh/apache/pinot/pull/9885/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2xvYWRlci9JbmRleExvYWRpbmdDb25maWcuamF2YQ==) | `0.00% <0.00%> (-81.35%)` | :arrow_down: |
| [...core/query/executor/ServerQueryExecutorV1Impl.java](https://codecov.io/gh/apache/pinot/pull/9885/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9leGVjdXRvci9TZXJ2ZXJRdWVyeUV4ZWN1dG9yVjFJbXBsLmphdmE=) | `74.76% <57.89%> (-6.00%)` | :arrow_down: |
| [...ata/manager/realtime/RealtimeTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/9885/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVUYWJsZURhdGFNYW5hZ2VyLmphdmE=) | `39.31% <83.33%> (-0.85%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/spi/utils/LoopUtils.java](https://codecov.io/gh/apache/pinot/pull/9885/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvTG9vcFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/9885/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/9885/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/9885/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/9885/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/user/RoleType.java](https://codecov.io/gh/apache/pinot/pull/9885/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3VzZXIvUm9sZVR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/9885/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1042 more](https://codecov.io/gh/apache/pinot/pull/9885/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [pinot] Jackie-Jiang commented on pull request #9885: Fix issues for realtime table reload
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #9885:
URL: https://github.com/apache/pinot/pull/9885#issuecomment-1333022911
> would be nice if we can add some unit-test
It is slightly hard to test the reload behavior in the unit test. Currently most of the reload test is done in the integration test.
--
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
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #9885: Fix issues for realtime table reload
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #9885:
URL: https://github.com/apache/pinot/pull/9885#discussion_r1042786535
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeTableDataManager.java:
##########
@@ -538,10 +538,14 @@ public void replaceHLSegment(SegmentZKMetadata segmentZKMetadata, IndexLoadingCo
* Replaces a committed LLC REALTIME segment.
*/
public void replaceLLSegment(String segmentName, IndexLoadingConfig indexLoadingConfig) {
+ File indexDir = new File(_indexDir, segmentName);
+ // Use the latest table config and schema to load the segment
+ TableConfig tableConfig = ZKMetadataProvider.getTableConfig(_propertyStore, _tableNameWithType);
+ Preconditions.checkState(tableConfig != null, "Failed to get table config for table: {}", _tableNameWithType);
+ Schema schema = ZKMetadataProvider.getTableSchema(_propertyStore, tableConfig);
+ indexLoadingConfig.updateTableConfigAndSchema(tableConfig, schema);
Review Comment:
Yeah, I like the idea of creating a new IndexLoadingConfig. I'll propose a fix for it.
--
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