You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "PrachiKhobragade (via GitHub)" <gi...@apache.org> on 2023/06/20 03:39:28 UTC
[GitHub] [pinot] PrachiKhobragade opened a new pull request, #10946: Add a new SegmentPurger interface method to get record purger based on table configs and schema
PrachiKhobragade opened a new pull request, #10946:
URL: https://github.com/apache/pinot/pull/10946
This change adds a new default method to get the RecordPurger which also takes table and task configs and table 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
[GitHub] [pinot] PrachiKhobragade commented on a diff in pull request #10946: Add a new SegmentPurger interface method to get record purger based on table configs and schema
Posted by "PrachiKhobragade (via GitHub)" <gi...@apache.org>.
PrachiKhobragade commented on code in PR #10946:
URL: https://github.com/apache/pinot/pull/10946#discussion_r1236439672
##########
pinot-core/src/main/java/org/apache/pinot/core/minion/SegmentPurger.java:
##########
@@ -230,6 +231,10 @@ public interface RecordPurgerFactory {
* Get the {@link RecordPurger} for the given table.
*/
RecordPurger getRecordPurger(String rawTableName);
+
+ default RecordPurger getRecordPurger(PinotTaskConfig taskConfig, TableConfig tableConfig, Schema tableSchema) {
Review Comment:
done
--
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 #10946: Add a new SegmentPurger interface method to get record purger based on table configs and schema
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10946:
URL: https://github.com/apache/pinot/pull/10946#issuecomment-1598090140
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10946?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10946](https://app.codecov.io/gh/apache/pinot/pull/10946?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (eee66f5) into [master](https://app.codecov.io/gh/apache/pinot/commit/8b2fb03d8532afd092c0211990e61f156fb45b73?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8b2fb03) will **increase** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10946 +/- ##
=========================================
Coverage 0.11% 0.11%
=========================================
Files 2188 2134 -54
Lines 117754 115258 -2496
Branches 17795 17493 -302
=========================================
Hits 137 137
+ Misses 117597 115101 -2496
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `?` | |
| integration1temurin17 | `?` | |
| integration1temurin20 | `?` | |
| integration2temurin11 | `?` | |
| integration2temurin17 | `?` | |
| integration2temurin20 | `?` | |
| unittests1temurin11 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `0.11% <0.00%> (ø)` | |
| unittests2temurin17 | `?` | |
| unittests2temurin20 | `0.11% <0.00%> (ø)` | |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10946?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/core/minion/SegmentPurger.java](https://app.codecov.io/gh/apache/pinot/pull/10946?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9taW5pb24vU2VnbWVudFB1cmdlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...t/plugin/minion/tasks/purge/PurgeTaskExecutor.java](https://app.codecov.io/gh/apache/pinot/pull/10946?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvcHVyZ2UvUHVyZ2VUYXNrRXhlY3V0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
... and [54 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10946/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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=apache)
--
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] navina commented on a diff in pull request #10946: Add a new SegmentPurger interface method to get record purger based on table configs and schema
Posted by "navina (via GitHub)" <gi...@apache.org>.
navina commented on code in PR #10946:
URL: https://github.com/apache/pinot/pull/10946#discussion_r1236397175
##########
pinot-core/src/main/java/org/apache/pinot/core/minion/SegmentPurger.java:
##########
@@ -230,6 +231,10 @@ public interface RecordPurgerFactory {
* Get the {@link RecordPurger} for the given table.
*/
RecordPurger getRecordPurger(String rawTableName);
+
+ default RecordPurger getRecordPurger(PinotTaskConfig taskConfig, TableConfig tableConfig, Schema tableSchema) {
Review Comment:
nit: add javadocs to public apis
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/purge/PurgeTaskExecutor.java:
##########
@@ -46,14 +46,14 @@ protected SegmentConversionResult convert(PinotTaskConfig pinotTaskConfig, File
String rawTableName = TableNameBuilder.extractRawTableName(tableNameWithType);
SegmentPurger.RecordPurgerFactory recordPurgerFactory = MINION_CONTEXT.getRecordPurgerFactory();
+ TableConfig tableConfig = getTableConfig(tableNameWithType);
+ Schema schema = getSchema(tableNameWithType);
SegmentPurger.RecordPurger recordPurger =
- recordPurgerFactory != null ? recordPurgerFactory.getRecordPurger(rawTableName) : null;
+ getRecordPurger(pinotTaskConfig, recordPurgerFactory, tableConfig, schema);
Review Comment:
+1 to Sajjad's comment about ternary operator reads better.
--
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] PrachiKhobragade commented on a diff in pull request #10946: Add a new SegmentPurger interface method to get record purger based on table configs and schema
Posted by "PrachiKhobragade (via GitHub)" <gi...@apache.org>.
PrachiKhobragade commented on code in PR #10946:
URL: https://github.com/apache/pinot/pull/10946#discussion_r1235799724
##########
pinot-core/src/main/java/org/apache/pinot/core/minion/SegmentPurger.java:
##########
@@ -230,6 +230,11 @@ public interface RecordPurgerFactory {
* Get the {@link RecordPurger} for the given table.
*/
RecordPurger getRecordPurger(String rawTableName);
+
+ default RecordPurger getRecordPurger(String rawTableName, PinotTaskConfig taskConfig, TableConfig tableConfig,
Review Comment:
Removed
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/purge/PurgeTaskExecutor.java:
##########
@@ -77,4 +77,14 @@ protected SegmentZKMetadataCustomMapModifier getSegmentZKMetadataCustomMapModifi
.singletonMap(MinionConstants.PurgeTask.TASK_TYPE + MinionConstants.TASK_TIME_SUFFIX,
String.valueOf(System.currentTimeMillis())));
}
+
+ private static SegmentPurger.RecordPurger getRecordPurger(PinotTaskConfig pinotTaskConfig, String rawTableName,
+ SegmentPurger.RecordPurgerFactory recordPurgerFactory, TableConfig tableConfig, Schema schema) {
+ if (recordPurgerFactory == null) {
+ return null;
+ }
+ SegmentPurger.RecordPurger recordPurger =
+ recordPurgerFactory.getRecordPurger(rawTableName, pinotTaskConfig, tableConfig, schema);
+ return recordPurger != null ? recordPurger : recordPurgerFactory.getRecordPurger(rawTableName);
Review Comment:
Modified
--
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] sajjad-moradi commented on a diff in pull request #10946: Add a new SegmentPurger interface method to get record purger based on table configs and schema
Posted by "sajjad-moradi (via GitHub)" <gi...@apache.org>.
sajjad-moradi commented on code in PR #10946:
URL: https://github.com/apache/pinot/pull/10946#discussion_r1236018596
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/purge/PurgeTaskExecutor.java:
##########
@@ -46,14 +46,14 @@ protected SegmentConversionResult convert(PinotTaskConfig pinotTaskConfig, File
String rawTableName = TableNameBuilder.extractRawTableName(tableNameWithType);
SegmentPurger.RecordPurgerFactory recordPurgerFactory = MINION_CONTEXT.getRecordPurgerFactory();
+ TableConfig tableConfig = getTableConfig(tableNameWithType);
+ Schema schema = getSchema(tableNameWithType);
SegmentPurger.RecordPurger recordPurger =
- recordPurgerFactory != null ? recordPurgerFactory.getRecordPurger(rawTableName) : null;
+ getRecordPurger(pinotTaskConfig, recordPurgerFactory, tableConfig, schema);
Review Comment:
Nit: Maybe the ternary operator reads better, and it's in sync with recordModifer initialization?
--
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] jtao15 commented on a diff in pull request #10946: Add a new SegmentPurger interface method to get record purger based on table configs and schema
Posted by "jtao15 (via GitHub)" <gi...@apache.org>.
jtao15 commented on code in PR #10946:
URL: https://github.com/apache/pinot/pull/10946#discussion_r1235629041
##########
pinot-core/src/main/java/org/apache/pinot/core/minion/SegmentPurger.java:
##########
@@ -230,6 +230,11 @@ public interface RecordPurgerFactory {
* Get the {@link RecordPurger} for the given table.
*/
RecordPurger getRecordPurger(String rawTableName);
+
+ default RecordPurger getRecordPurger(String rawTableName, PinotTaskConfig taskConfig, TableConfig tableConfig,
Review Comment:
We don't need `rawTableName` because it can be retrieved easily from the `tableConfig`?
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/purge/PurgeTaskExecutor.java:
##########
@@ -77,4 +77,14 @@ protected SegmentZKMetadataCustomMapModifier getSegmentZKMetadataCustomMapModifi
.singletonMap(MinionConstants.PurgeTask.TASK_TYPE + MinionConstants.TASK_TIME_SUFFIX,
String.valueOf(System.currentTimeMillis())));
}
+
+ private static SegmentPurger.RecordPurger getRecordPurger(PinotTaskConfig pinotTaskConfig, String rawTableName,
+ SegmentPurger.RecordPurgerFactory recordPurgerFactory, TableConfig tableConfig, Schema schema) {
+ if (recordPurgerFactory == null) {
+ return null;
+ }
+ SegmentPurger.RecordPurger recordPurger =
+ recordPurgerFactory.getRecordPurger(rawTableName, pinotTaskConfig, tableConfig, schema);
+ return recordPurger != null ? recordPurger : recordPurgerFactory.getRecordPurger(rawTableName);
Review Comment:
Is it better to just call `getRecordPurger(rawTableName)` in `getRecordPurger(rawTableName, pinotTaskConfig, tableConfig, 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
[GitHub] [pinot] PrachiKhobragade commented on a diff in pull request #10946: Add a new SegmentPurger interface method to get record purger based on table configs and schema
Posted by "PrachiKhobragade (via GitHub)" <gi...@apache.org>.
PrachiKhobragade commented on code in PR #10946:
URL: https://github.com/apache/pinot/pull/10946#discussion_r1236441550
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/purge/PurgeTaskExecutor.java:
##########
@@ -46,14 +46,14 @@ protected SegmentConversionResult convert(PinotTaskConfig pinotTaskConfig, File
String rawTableName = TableNameBuilder.extractRawTableName(tableNameWithType);
SegmentPurger.RecordPurgerFactory recordPurgerFactory = MINION_CONTEXT.getRecordPurgerFactory();
+ TableConfig tableConfig = getTableConfig(tableNameWithType);
+ Schema schema = getSchema(tableNameWithType);
SegmentPurger.RecordPurger recordPurger =
- recordPurgerFactory != null ? recordPurgerFactory.getRecordPurger(rawTableName) : null;
+ getRecordPurger(pinotTaskConfig, recordPurgerFactory, tableConfig, schema);
Review Comment:
Removed the method and kept the ternary operator
--
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] sajjad-moradi merged pull request #10946: Add a new SegmentPurger interface method to get record purger based on table configs and schema
Posted by "sajjad-moradi (via GitHub)" <gi...@apache.org>.
sajjad-moradi merged PR #10946:
URL: https://github.com/apache/pinot/pull/10946
--
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