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 2020/05/01 20:43:55 UTC
[GitHub] [incubator-pinot] Jackie-Jiang opened a new pull request #5325: In SegmentPurger, use table config to generate the segment
Jackie-Jiang opened a new pull request #5325:
URL: https://github.com/apache/incubator-pinot/pull/5325
All the indexing and partition info are stored in the table config.
Without the table config, purged segment will lose the indexing and partition info, which could cause performance issue.
In the following pr, we will enforce all segment creation to provide table config and schema to guarantee the consistent behavior.
----------------------------------------------------------------
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.
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] [incubator-pinot] codecov-io commented on pull request #5325: In SegmentPurger, use table config to generate the segment
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #5325:
URL: https://github.com/apache/incubator-pinot/pull/5325#issuecomment-622601133
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5325?src=pr&el=h1) Report
> Merging [#5325](https://codecov.io/gh/apache/incubator-pinot/pull/5325?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/997853d6cd7dcd2c974072424bf003e015d61a92&el=desc) will **decrease** coverage by `9.22%`.
> The diff coverage is `40.42%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/5325?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #5325 +/- ##
==========================================
- Coverage 66.07% 56.85% -9.23%
==========================================
Files 1072 1072
Lines 54562 54552 -10
Branches 8139 8137 -2
==========================================
- Hits 36052 31015 -5037
- Misses 15865 21105 +5240
+ Partials 2645 2432 -213
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5325?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...rg/apache/pinot/core/minion/RawIndexConverter.java](https://codecov.io/gh/apache/incubator-pinot/pull/5325/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9taW5pb24vUmF3SW5kZXhDb252ZXJ0ZXIuamF2YQ==) | `0.00% <0.00%> (-62.50%)` | :arrow_down: |
| [...in/java/org/apache/pinot/minion/MinionStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/5325/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vTWluaW9uU3RhcnRlci5qYXZh) | `0.00% <0.00%> (-86.25%)` | :arrow_down: |
| [...inot/minion/events/DefaultMinionEventObserver.java](https://codecov.io/gh/apache/incubator-pinot/pull/5325/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXZlbnRzL0RlZmF1bHRNaW5pb25FdmVudE9ic2VydmVyLmphdmE=) | `0.00% <ø> (-60.00%)` | :arrow_down: |
| [...ot/minion/events/EventObserverFactoryRegistry.java](https://codecov.io/gh/apache/incubator-pinot/pull/5325/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXZlbnRzL0V2ZW50T2JzZXJ2ZXJGYWN0b3J5UmVnaXN0cnkuamF2YQ==) | `0.00% <ø> (-100.00%)` | :arrow_down: |
| [.../executor/BaseSingleSegmentConversionExecutor.java](https://codecov.io/gh/apache/incubator-pinot/pull/5325/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhlY3V0b3IvQmFzZVNpbmdsZVNlZ21lbnRDb252ZXJzaW9uRXhlY3V0b3IuamF2YQ==) | `4.54% <0.00%> (-87.13%)` | :arrow_down: |
| [...minion/executor/ConvertToRawIndexTaskExecutor.java](https://codecov.io/gh/apache/incubator-pinot/pull/5325/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhlY3V0b3IvQ29udmVydFRvUmF3SW5kZXhUYXNrRXhlY3V0b3IuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...indexsegment/generator/SegmentGeneratorConfig.java](https://codecov.io/gh/apache/incubator-pinot/pull/5325/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9pbmRleHNlZ21lbnQvZ2VuZXJhdG9yL1NlZ21lbnRHZW5lcmF0b3JDb25maWcuamF2YQ==) | `58.38% <40.00%> (-3.51%)` | :arrow_down: |
| [...pache/pinot/minion/executor/PurgeTaskExecutor.java](https://codecov.io/gh/apache/incubator-pinot/pull/5325/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhlY3V0b3IvUHVyZ2VUYXNrRXhlY3V0b3IuamF2YQ==) | `69.23% <60.00%> (-0.34%)` | :arrow_down: |
| [...org/apache/pinot/core/minion/SegmentConverter.java](https://codecov.io/gh/apache/incubator-pinot/pull/5325/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9taW5pb24vU2VnbWVudENvbnZlcnRlci5qYXZh) | `79.74% <100.00%> (+1.43%)` | :arrow_up: |
| [...va/org/apache/pinot/core/minion/SegmentPurger.java](https://codecov.io/gh/apache/incubator-pinot/pull/5325/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9taW5pb24vU2VnbWVudFB1cmdlci5qYXZh) | `76.92% <100.00%> (-1.97%)` | :arrow_down: |
| ... and [328 more](https://codecov.io/gh/apache/incubator-pinot/pull/5325/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5325?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5325?src=pr&el=footer). Last update [997853d...4db2ecd](https://codecov.io/gh/apache/incubator-pinot/pull/5325?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-pinot] Jackie-Jiang commented on pull request #5325: In SegmentPurger, use table config to generate the segment
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #5325:
URL: https://github.com/apache/incubator-pinot/pull/5325#issuecomment-622587478
> ```
> testConvert(org.apache.pinot.minion.executor.PurgeTaskExecutorTest) Time elapsed: 0.034 sec <<< FAILURE!
> java.lang.NullPointerException
> at org.apache.pinot.common.metadata.ZKMetadataProvider.getTableConfig(ZKMetadataProvider.java:211)
> ```
Yeah, need to modify the test to plug in the property store
----------------------------------------------------------------
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.
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] [incubator-pinot] snleee commented on pull request #5325: In SegmentPurger, use table config to generate the segment
Posted by GitBox <gi...@apache.org>.
snleee commented on pull request #5325:
URL: https://github.com/apache/incubator-pinot/pull/5325#issuecomment-622586974
```
testConvert(org.apache.pinot.minion.executor.PurgeTaskExecutorTest) Time elapsed: 0.034 sec <<< FAILURE!
java.lang.NullPointerException
at org.apache.pinot.common.metadata.ZKMetadataProvider.getTableConfig(ZKMetadataProvider.java:211)
```
----------------------------------------------------------------
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.
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] [incubator-pinot] snleee commented on a change in pull request #5325: In SegmentPurger, use table config to generate the segment
Posted by GitBox <gi...@apache.org>.
snleee commented on a change in pull request #5325:
URL: https://github.com/apache/incubator-pinot/pull/5325#discussion_r418751259
##########
File path: pinot-minion/src/main/java/org/apache/pinot/minion/MinionContext.java
##########
@@ -39,7 +41,7 @@ public static MinionContext getInstance() {
private File _dataDir;
private MinionMetrics _minionMetrics;
- private String _minionVersion;
+ private ZkHelixPropertyStore<ZNRecord> _helixPropertyStore;
Review comment:
+1 for this!
I thought of fetching table config calling controller API. This approach is much simpler :)
----------------------------------------------------------------
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.
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