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 2021/07/17 19:41:35 UTC
[GitHub] [incubator-pinot] suddendust opened a new pull request #7173: [7156] Human Readable Controller Configs
suddendust opened a new pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173
## Description
#7156
This PR deprecates certain controller configurations in favour of new configs that accept human-readable period strings.
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes
Does this PR otherwise need attention when creating release notes?
* [x] Yes
## Release Notes
The following configurations have been updated. The ones to the left of the `-> ` have been deprecated. Each deprecated configuration has been replaced with the corresponding configuration to the right of the `-> `. If both configs are present, the new one is picked. If both configs are present but the new config uses an incorrect representation of period string, an exception is thrown (it does not fallback to the old configuration).
- `controller.retention.frequencyInSeconds` -> `controller.retention.frequencyPeriod`
- `controller.offline.segment.interval.checker.frequencyInSeconds` -> `controller.offline.segment.interval.checker.frequencyPeriod`
- `controller.realtime.segment.validation.frequencyInSeconds` -> `controller.realtime.segment.validation.frequencyPeriod`
- `controller.realtime.segment.validation.initialDelayInSeconds` -> `controller.realtime.segment.validation.initialDelayPeriod`
- `controller.broker.resource.validation.frequencyInSeconds` -> `controller.broker.resource.validation.frequencyPeriod`
- `controller.broker.resource.validation.initialDelayInSeconds` -> `controller.broker.resource.validation.initialDelayPeriod`
- `controller.statuschecker.frequencyInSeconds` -> `controller.statuschecker.frequencyPeriod`
- `controller.statuschecker.waitForPushTimeInSeconds` -> `controller.statuschecker.waitForPushTimePeriod`
- `controller.task.frequencyInSeconds` -> `controller.task.frequencyPeriod`
- `controller.minion.instances.cleanup.task.frequencyInSeconds` -> `controller.minion.instances.cleanup.task.frequencyPeriod`
- `controller.minion.instances.cleanup.task.initialDelaySeconds` -> `controller.minion.instances.cleanup.task.initialDelayPeriod`
- `controller.minion.instances.cleanup.task.minOfflineTimeBeforeDeletionSeconds` -> `controller.minion.instances.cleanup.task.minOfflineTimeBeforeDeletionPeriod`
- `controller.minion.task.metrics.emitter.frequencyInSeconds` -> `controller.minion.task.metrics.emitter.frequencyPeriod`
- `controller.segment.relocator.frequencyInSeconds` -> `controller.segment.relocator.frequencyPeriod`
- `controller.segment.level.validation.intervalInSeconds` -> `controller.segment.level.validation.intervalPeriod`
- `controller.statusChecker.initialDelayInSeconds` -> `controller.statusChecker.initialDelayPeriod`
- `controller.retentionManager.initialDelayInSeconds` -> `controller.retentionManager.initialDelayPeriod`
- `controller.offlineSegmentIntervalChecker.initialDelayInSeconds` -> `controller.offlineSegmentIntervalChecker.initialDelayPeriod`
- `controller.segmentRelocator.initialDelayInSeconds` -> `controller.segmentRelocator.initialDelayPeriod`
Some examples of updated configurations:
```
controller.segmentRelocator.initialDelayPeriod=20s
#controller.segment.level.validation.intervalPeriod=5m
```
## Documentation
Final PR for docs update will be raised once this PR is approved.
--
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] [incubator-pinot] suddendust closed pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust closed pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173
--
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] [incubator-pinot] suddendust commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r674535352
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/UpsertTableSegmentUploadIntegrationTest.java
##########
@@ -152,8 +152,8 @@ protected void startController()
Map<String, Object> controllerConfig = getDefaultControllerConfiguration();
// Perform realtime segment validation every second with 1 second initial delay.
controllerConfig
- .put(ControllerConf.ControllerPeriodicTasksConf.REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS, 1);
- controllerConfig.put(ControllerConf.ControllerPeriodicTasksConf.SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS, 1);
+ .put(ControllerConf.ControllerPeriodicTasksConf.DEPRECATED_REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS, 1);
Review comment:
Yes @jackjlli we need to stop using these deprecated configs in our tests. Can I raise another PR for this? I think they are used at too many places and changing it in this PR will add another round of review. Can raise it immediately after this one.
--
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] [incubator-pinot] suddendust commented on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-882723615
Thanks for the review everyone, I will incorporate the suggested changes.
--
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] [incubator-pinot] mcvsubbu commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r674170688
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -74,60 +76,115 @@
}
public static class ControllerPeriodicTasksConf {
+
// frequency configs
+ @Deprecated
public static final String RETENTION_MANAGER_FREQUENCY_IN_SECONDS = "controller.retention.frequencyInSeconds";
+ public static final String RETENTION_MANAGER_FREQUENCY_PERIOD = "controller.retention.frequencyPeriod";
@Deprecated
// The ValidationManager has been split up into 3 separate tasks, each having their own frequency config settings
public static final String DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS =
"controller.validation.frequencyInSeconds";
+ @Deprecated
public static final String OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS =
"controller.offline.segment.interval.checker.frequencyInSeconds";
+ public static final String OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD =
+ "controller.offline.segment.interval.checker.frequencyPeriod";
+ @Deprecated
public static final String REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS =
"controller.realtime.segment.validation.frequencyInSeconds";
+ public static final String REALTIME_SEGMENT_VALIDATION_FREQUENCY_PERIOD =
+ "controller.realtime.segment.validation.frequencyPeriod";
+ @Deprecated
public static final String REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_IN_SECONDS =
"controller.realtime.segment.validation.initialDelayInSeconds";
+ public static final String REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_PERIOD =
+ "controller.realtime.segment.validation.initialDelayPeriod";
+ @Deprecated
public static final String BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS =
"controller.broker.resource.validation.frequencyInSeconds";
+ public static final String BROKER_RESOURCE_VALIDATION_FREQUENCY_PERIOD =
+ "controller.broker.resource.validation.frequencyPeriod";
+ @Deprecated
public static final String BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_IN_SECONDS =
"controller.broker.resource.validation.initialDelayInSeconds";
+ public static final String BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_PERIOD =
+ "controller.broker.resource.validation.initialDelayPeriod";
+ @Deprecated
public static final String STATUS_CHECKER_FREQUENCY_IN_SECONDS = "controller.statuschecker.frequencyInSeconds";
+ public static final String STATUS_CHECKER_FREQUENCY_PERIOD = "controller.statuschecker.frequencyPeriod";
+ @Deprecated
public static final String STATUS_CHECKER_WAIT_FOR_PUSH_TIME_IN_SECONDS =
"controller.statuschecker.waitForPushTimeInSeconds";
+ public static final String STATUS_CHECKER_WAIT_FOR_PUSH_TIME_PERIOD =
Review comment:
You are right. I was mixed up with another config perhaps. Thanks for the diligence
--
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] [incubator-pinot] jackjlli commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r673493791
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -74,60 +76,115 @@
}
public static class ControllerPeriodicTasksConf {
+
// frequency configs
+ @Deprecated
public static final String RETENTION_MANAGER_FREQUENCY_IN_SECONDS = "controller.retention.frequencyInSeconds";
Review comment:
I like the idea of adding `DEPRECATED_` prefix to the ones that are about to deprecated in the next release. Maybe we should do that in this PR as well.
--
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] [incubator-pinot] codecov-commenter edited a comment on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-883325800
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?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 [#7173](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b910b27) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/6d0ab91de409fec8ddbf816c2348c8b95a37e393?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6d0ab91) will **decrease** coverage by `8.25%`.
> The diff coverage is `65.67%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7173 +/- ##
============================================
- Coverage 73.49% 65.23% -8.26%
Complexity 92 92
============================================
Files 1500 1506 +6
Lines 73758 73834 +76
Branches 10637 10652 +15
============================================
- Hits 54209 48168 -6041
- Misses 16005 22261 +6256
+ Partials 3544 3405 -139
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `?` | |
| unittests | `65.23% <65.67%> (-0.04%)` | :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/incubator-pinot/pull/7173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `53.35% <65.67%> (+0.49%)` | :arrow_up: |
| [...va/org/apache/pinot/common/minion/Granularity.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWluaW9uL0dyYW51bGFyaXR5LmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...t/core/startree/plan/StarTreeDocIdSetPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlRG9jSWRTZXRQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ot/common/restlet/resources/TableMetadataInfo.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzdGxldC9yZXNvdXJjZXMvVGFibGVNZXRhZGF0YUluZm8uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../core/startree/plan/StarTreeTransformPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlVHJhbnNmb3JtUGxhbk5vZGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [381 more](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6d0ab91...b910b27](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-pinot] jackjlli commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r672465207
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -341,8 +406,9 @@ public String getDataDir() {
}
public int getSegmentCommitTimeoutSeconds() {
Review comment:
I feel that these method names also need to be changed, or have a new method here and mark the old one as deprecated.
The behavior should be that the new one should be used if both are specified or only the new one be specified. If not, use the old one.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -428,15 +496,20 @@ public void setRetentionControllerFrequencyInSeconds(int retentionFrequencyInSec
}
/**
- * Returns the config value for controller.offline.segment.interval.checker.frequencyInSeconds if it exists.
- * If it doesn't exist, returns the segment level validation interval. This is done in order to retain the current behavior,
- * wherein the offline validation tasks were done at segment level validation interval frequency
- * The default value is the new DEFAULT_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS
+ * Returns the config value for controller.offline.segment.interval.checker.frequencyInSeconds if
Review comment:
We should check the new one first and then the old one, right?
--
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] [incubator-pinot] mcvsubbu commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r672444492
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1; // Disabled
private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS = 60 * 60; // 1 Hour.
+ private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS =
+ 60 * 60; // 1 Hour.
private static final int DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS = 60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS = "server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD = "server.request.timeoutPeriod";
+
+ @Deprecated
private static final String SEGMENT_COMMIT_TIMEOUT_SECONDS = "controller.realtime.segment.commit.timeoutSeconds";
+ private static final String SEGMENT_COMMIT_TIMEOUT_PERIOD = "controller.realtime.segment.commit.timeoutPeriod";
Review comment:
This one should not be changed
##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java
##########
@@ -0,0 +1,158 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.controller.ControllerConf.ControllerPeriodicTasksConf.*;
+
+
+public class ControllerConfTest {
+
+ private static final List<String> UNIT_CONFIGS =
+ List.of(RETENTION_MANAGER_FREQUENCY_IN_SECONDS, OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS,
+ OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS, REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS,
+ REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_IN_SECONDS, BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS,
+ BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_IN_SECONDS, STATUS_CHECKER_FREQUENCY_IN_SECONDS,
+ STATUS_CHECKER_WAIT_FOR_PUSH_TIME_IN_SECONDS, TASK_MANAGER_FREQUENCY_IN_SECONDS,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS, MINION_INSTANCES_CLEANUP_TASK_INITIAL_DELAY_SECONDS,
+ MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_SECONDS,
+ TASK_METRICS_EMITTER_FREQUENCY_IN_SECONDS, SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS,
+ SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS, STATUS_CHECKER_INITIAL_DELAY_IN_SECONDS,
+ OFFLINE_SEGMENT_INTERVAL_CHECKER_INITIAL_DELAY_IN_SECONDS,
+ DEPRECATED_REALTIME_SEGMENT_RELOCATION_INITIAL_DELAY_IN_SECONDS, SEGMENT_RELOCATOR_INITIAL_DELAY_IN_SECONDS);
+
+ private static final List<String> PERIOD_CONFIGS =
+ List.of(RETENTION_MANAGER_FREQUENCY_PERIOD, OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD,
+ REALTIME_SEGMENT_VALIDATION_FREQUENCY_PERIOD, REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_PERIOD,
+ BROKER_RESOURCE_VALIDATION_FREQUENCY_PERIOD, BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_PERIOD,
+ STATUS_CHECKER_FREQUENCY_PERIOD, STATUS_CHECKER_WAIT_FOR_PUSH_TIME_PERIOD, TASK_MANAGER_FREQUENCY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_PERIOD, MINION_INSTANCES_CLEANUP_TASK_INITIAL_DELAY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_PERIOD, TASK_METRICS_EMITTER_FREQUENCY_PERIOD,
+ SEGMENT_RELOCATOR_FREQUENCY_PERIOD, SEGMENT_LEVEL_VALIDATION_INTERVAL_PERIOD,
+ STATUS_CHECKER_INITIAL_DELAY_PERIOD, OFFLINE_SEGMENT_INTERVAL_CHECKER_INITIAL_DELAY_PERIOD,
+ SEGMENT_RELOCATOR_INITIAL_DELAY_PERIOD);
+
+ private static final int DURATION_IN_SECONDS = 20;
+ private static final String DURATION_IN_PERIOD_VALID = "2m";
+ private static final int PERIOD_DURATION_IN_SECONDS = 120;
+ private static final String DURATION_IN_PERIOD_INVALID = "2m2m";
+
+ /**
+ * When both type of configs are present, then the human-readable period config overrides the unit config
+ */
+ @Test
+ public void periodConfigOverridesUnitConfig() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ UNIT_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_SECONDS));
+ PERIOD_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_PERIOD_VALID));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, PERIOD_DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When only unit configs are supplied, then the correct converted value is returned.
+ */
+ @Test
+ public void suppliedUnitConfigsShouldReturnCorrectValues() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ UNIT_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_SECONDS));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When only period configs are supplied, then the correct converted value is returned.
+ */
+ @Test
+ public void suppliedPeriodConfigsShouldReturnCorrectValues() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ PERIOD_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_PERIOD_VALID));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, PERIOD_DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When valid unit config and invalid period config are specified, then an {@link IllegalArgumentException} is thrown (valid unit
+ * config does not override invalid period config)
+ */
+ @Test(expectedExceptions = IllegalArgumentException.class)
Review comment:
Expected exceptions are good, but I prefer fielding specific exceptions at the specific points where we expect them. That makes it easy to add more code to the test, or re-factor things, etc. and not worry about the test passing if we accidentally get IllegalArgumentException some place else.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1; // Disabled
private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS = 60 * 60; // 1 Hour.
+ private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS =
+ 60 * 60; // 1 Hour.
private static final int DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS = 60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS = "server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD = "server.request.timeoutPeriod";
Review comment:
This one should not be changed. We don't expect people to configure this value in minutes or hours.
--
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] [incubator-pinot] suddendust edited a comment on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust edited a comment on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-882012741
No setters have been added for the newly added configs as configs are meant to be immutable and these methods are already deprecated for existing configs.
Also, I cannot add labels for some reason. This PR needs the "release notes" label.
--
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] [incubator-pinot] suddendust commented on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-882723615
--
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] [incubator-pinot] mcvsubbu commented on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-885745279
Marking this for release notes, as well as backward incompat (since one config has been removed. This config was deprecated in 0.5.0 version)
--
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] [incubator-pinot] codecov-commenter edited a comment on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-883325800
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?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 [#7173](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (eeb896e) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/6d0ab91de409fec8ddbf816c2348c8b95a37e393?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6d0ab91) will **decrease** coverage by `8.24%`.
> The diff coverage is `67.79%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7173 +/- ##
============================================
- Coverage 73.49% 65.25% -8.25%
Complexity 92 92
============================================
Files 1500 1506 +6
Lines 73758 73826 +68
Branches 10637 10650 +13
============================================
- Hits 54209 48173 -6036
- Misses 16005 22247 +6242
+ Partials 3544 3406 -138
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `?` | |
| unittests | `65.25% <67.79%> (-0.02%)` | :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/incubator-pinot/pull/7173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `53.46% <67.79%> (+0.60%)` | :arrow_up: |
| [...va/org/apache/pinot/common/minion/Granularity.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWluaW9uL0dyYW51bGFyaXR5LmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...t/core/startree/plan/StarTreeDocIdSetPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlRG9jSWRTZXRQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ot/common/restlet/resources/TableMetadataInfo.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzdGxldC9yZXNvdXJjZXMvVGFibGVNZXRhZGF0YUluZm8uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../core/startree/plan/StarTreeTransformPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlVHJhbnNmb3JtUGxhbk5vZGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [382 more](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6d0ab91...eeb896e](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-pinot] suddendust commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r675282440
##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java
##########
@@ -0,0 +1,193 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Random;
+import java.util.concurrent.TimeUnit;
+import org.apache.commons.lang3.RandomStringUtils;
+import org.apache.pinot.spi.utils.TimeUtils;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.controller.ControllerConf.ControllerPeriodicTasksConf.*;
+
+
+public class ControllerConfTest {
+
+ private static final List<String> DEPRECATED_CONFIGS = Arrays
+ .asList(DEPRECATED_RETENTION_MANAGER_FREQUENCY_IN_SECONDS,
+ DEPRECATED_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS,
+ DEPRECATED_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS,
+ DEPRECATED_REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS,
+ DEPRECATED_BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS, DEPRECATED_STATUS_CHECKER_FREQUENCY_IN_SECONDS,
+ DEPRECATED_TASK_MANAGER_FREQUENCY_IN_SECONDS, DEPRECATED_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS,
+ DEPRECATED_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_SECONDS,
+ DEPRECATED_TASK_METRICS_EMITTER_FREQUENCY_IN_SECONDS, DEPRECATED_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS,
+ DEPRECATED_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS,
+ DEPRECATED_REALTIME_SEGMENT_RELOCATION_INITIAL_DELAY_IN_SECONDS,
+ DEPRECATED_STATUS_CHECKER_WAIT_FOR_PUSH_TIME_IN_SECONDS);
+
+ private static final List<String> NEW_CONFIGS = Arrays
+ .asList(RETENTION_MANAGER_FREQUENCY_PERIOD, OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD,
+ REALTIME_SEGMENT_VALIDATION_FREQUENCY_PERIOD, BROKER_RESOURCE_VALIDATION_FREQUENCY_PERIOD,
+ STATUS_CHECKER_FREQUENCY_PERIOD, TASK_MANAGER_FREQUENCY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_PERIOD, TASK_METRICS_EMITTER_FREQUENCY_PERIOD,
+ SEGMENT_RELOCATOR_FREQUENCY_PERIOD, SEGMENT_LEVEL_VALIDATION_INTERVAL_PERIOD,
+ STATUS_CHECKER_WAIT_FOR_PUSH_TIME_PERIOD);
+
+ private static final Random RAND = new Random();
+
+ /**
+ * When config contains: 1. Both deprecated config and the corresponding new config. 2. All new
+ * configurations are valid. 3. Some deprecated configurations are invalid. then new configs
+ * override deprecated configs (invalid deprecated configs do not throw exceptions when
+ * corresponding valid new configs are supplied as well)
+ */
+ @Test
+ public void validNewConfigOverridesCorrespondingValidOrInvalidOldConfigOnRead() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ int durationInSeconds = getRandomDurationInSeconds();
+ DEPRECATED_CONFIGS.forEach(config -> controllerConfig.put(config, durationInSeconds));
+ //put some invalid deprecated configs
+ controllerConfig.put(DEPRECATED_RETENTION_MANAGER_FREQUENCY_IN_SECONDS, getRandomString());
+ controllerConfig.put(DEPRECATED_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS, getRandomString());
+ //override all deprecated configs with valid new configs
+ String period = getRandomPeriodInMinutes();
+ NEW_CONFIGS.forEach(config -> controllerConfig.put(config, period));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, TimeUnit.SECONDS.convert(TimeUtils.convertPeriodToMillis(period), TimeUnit.MILLISECONDS));
+ }
+
+ /**
+ * When config contains: 1. Both deprecated config and the corresponding new config. 2. All
+ * deprecated configurations are valid. 3. Some new configurations are invalid. then exceptions
+ * are thrown when invalid new configurations are read (there is no fall-back to the corresponding
+ * valid deprecated configuration). For all valid new configurations, they override the
+ * corresponding deprecated configuration.
+ */
+ @Test
+ public void invalidNewConfigShouldThrowExceptionOnReadWithoutFallbackToCorrespondingValidDeprecatedConfig() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ int durationInSeconds = getRandomDurationInSeconds();
+ //all deprecated configs should be valid
+ DEPRECATED_CONFIGS.forEach(config -> controllerConfig.put(config, durationInSeconds));
+ String randomPeriodInMinutes = getRandomPeriodInMinutes();
Review comment:
Ah this is a good learning. Will incorporate this change.
--
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] [incubator-pinot] mcvsubbu commented on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-885087735
Other than one minor request of adding a comment, we are good to merge this.
--
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] [incubator-pinot] codecov-commenter edited a comment on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-883325800
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?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 [#7173](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (94ee738) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/6d0ab91de409fec8ddbf816c2348c8b95a37e393?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6d0ab91) will **decrease** coverage by `31.58%`.
> The diff coverage is `76.11%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7173 +/- ##
=============================================
- Coverage 73.49% 41.90% -31.59%
+ Complexity 92 7 -85
=============================================
Files 1500 1506 +6
Lines 73758 73834 +76
Branches 10637 10652 +15
=============================================
- Hits 54209 30942 -23267
- Misses 16005 40257 +24252
+ Partials 3544 2635 -909
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.90% <76.11%> (+0.01%)` | :arrow_up: |
| unittests | `?` | |
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/incubator-pinot/pull/7173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `50.19% <76.11%> (-2.67%)` | :arrow_down: |
| [...c/main/java/org/apache/pinot/common/tier/Tier.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/spi/config/table/TableType.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlVHlwZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/spi/data/DateTimeFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EYXRlVGltZUZpZWxkU3BlYy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pinot/spi/data/DimensionFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EaW1lbnNpb25GaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pinot/spi/data/readers/FileFormat.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9yZWFkZXJzL0ZpbGVGb3JtYXQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/common/minion/Granularity.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWluaW9uL0dyYW51bGFyaXR5LmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [956 more](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6d0ab91...94ee738](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-pinot] suddendust commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r672484276
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1; // Disabled
private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS = 60 * 60; // 1 Hour.
+ private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS =
+ 60 * 60; // 1 Hour.
private static final int DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS = 60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS = "server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD = "server.request.timeoutPeriod";
Review comment:
Yes makes sense. My thought process was that the configuration should move towards taking everything as human-readable strings and converting it to seconds (to keep it generic, basically if user wanted to keep the timeout as 2m, he should be able to).
##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java
##########
@@ -0,0 +1,158 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.controller.ControllerConf.ControllerPeriodicTasksConf.*;
+
+
+public class ControllerConfTest {
+
+ private static final List<String> UNIT_CONFIGS =
+ List.of(RETENTION_MANAGER_FREQUENCY_IN_SECONDS, OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS,
+ OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS, REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS,
+ REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_IN_SECONDS, BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS,
+ BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_IN_SECONDS, STATUS_CHECKER_FREQUENCY_IN_SECONDS,
+ STATUS_CHECKER_WAIT_FOR_PUSH_TIME_IN_SECONDS, TASK_MANAGER_FREQUENCY_IN_SECONDS,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS, MINION_INSTANCES_CLEANUP_TASK_INITIAL_DELAY_SECONDS,
+ MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_SECONDS,
+ TASK_METRICS_EMITTER_FREQUENCY_IN_SECONDS, SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS,
+ SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS, STATUS_CHECKER_INITIAL_DELAY_IN_SECONDS,
+ OFFLINE_SEGMENT_INTERVAL_CHECKER_INITIAL_DELAY_IN_SECONDS,
+ DEPRECATED_REALTIME_SEGMENT_RELOCATION_INITIAL_DELAY_IN_SECONDS, SEGMENT_RELOCATOR_INITIAL_DELAY_IN_SECONDS);
+
+ private static final List<String> PERIOD_CONFIGS =
+ List.of(RETENTION_MANAGER_FREQUENCY_PERIOD, OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD,
+ REALTIME_SEGMENT_VALIDATION_FREQUENCY_PERIOD, REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_PERIOD,
+ BROKER_RESOURCE_VALIDATION_FREQUENCY_PERIOD, BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_PERIOD,
+ STATUS_CHECKER_FREQUENCY_PERIOD, STATUS_CHECKER_WAIT_FOR_PUSH_TIME_PERIOD, TASK_MANAGER_FREQUENCY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_PERIOD, MINION_INSTANCES_CLEANUP_TASK_INITIAL_DELAY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_PERIOD, TASK_METRICS_EMITTER_FREQUENCY_PERIOD,
+ SEGMENT_RELOCATOR_FREQUENCY_PERIOD, SEGMENT_LEVEL_VALIDATION_INTERVAL_PERIOD,
+ STATUS_CHECKER_INITIAL_DELAY_PERIOD, OFFLINE_SEGMENT_INTERVAL_CHECKER_INITIAL_DELAY_PERIOD,
+ SEGMENT_RELOCATOR_INITIAL_DELAY_PERIOD);
+
+ private static final int DURATION_IN_SECONDS = 20;
+ private static final String DURATION_IN_PERIOD_VALID = "2m";
+ private static final int PERIOD_DURATION_IN_SECONDS = 120;
+ private static final String DURATION_IN_PERIOD_INVALID = "2m2m";
+
+ /**
+ * When both type of configs are present, then the human-readable period config overrides the unit config
+ */
+ @Test
+ public void periodConfigOverridesUnitConfig() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ UNIT_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_SECONDS));
+ PERIOD_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_PERIOD_VALID));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, PERIOD_DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When only unit configs are supplied, then the correct converted value is returned.
+ */
+ @Test
+ public void suppliedUnitConfigsShouldReturnCorrectValues() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ UNIT_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_SECONDS));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When only period configs are supplied, then the correct converted value is returned.
+ */
+ @Test
+ public void suppliedPeriodConfigsShouldReturnCorrectValues() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ PERIOD_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_PERIOD_VALID));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, PERIOD_DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When valid unit config and invalid period config are specified, then an {@link IllegalArgumentException} is thrown (valid unit
+ * config does not override invalid period config)
+ */
+ @Test(expectedExceptions = IllegalArgumentException.class)
Review comment:
That's a valid point @mcvsubbu. I just went with the simplest approach as the test was very trivial, and you pretty much can point where exactly the exception was thrown. Will change it.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1; // Disabled
private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS = 60 * 60; // 1 Hour.
+ private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS =
+ 60 * 60; // 1 Hour.
private static final int DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS = 60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS = "server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD = "server.request.timeoutPeriod";
Review comment:
Yes makes sense. My thought process was that the configuration should move towards taking everything as human-readable strings and converting it to whatever units it wants to (to keep it generic, basically if user wanted to keep the timeout as 2m, he should be able to. The config takes care to changing it to seconds). Will address this.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -341,8 +406,9 @@ public String getDataDir() {
}
public int getSegmentCommitTimeoutSeconds() {
Review comment:
"The behavior should be that the new one should be used if both are specified or only the new one be specified" - Isn't it what it is doing @jackjlli? Basically if the new config is absent, it falls-back to the older one. But method still does what it says - `getSegmentCommitTimeoutSeconds`.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -428,15 +496,20 @@ public void setRetentionControllerFrequencyInSeconds(int retentionFrequencyInSec
}
/**
- * Returns the config value for controller.offline.segment.interval.checker.frequencyInSeconds if it exists.
- * If it doesn't exist, returns the segment level validation interval. This is done in order to retain the current behavior,
- * wherein the offline validation tasks were done at segment level validation interval frequency
- * The default value is the new DEFAULT_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS
+ * Returns the config value for controller.offline.segment.interval.checker.frequencyInSeconds if
Review comment:
Missed this, will update the docs.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1; // Disabled
private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS = 60 * 60; // 1 Hour.
+ private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS =
+ 60 * 60; // 1 Hour.
private static final int DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS = 60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS = "server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD = "server.request.timeoutPeriod";
Review comment:
Yes makes sense. My thought process was that the configuration should move towards taking everything as human-readable strings and converting it to whatever units it wants to (to keep it generic, basically if user wanted to keep the timeout as 2m, he should be able to. The config takes care to changing of to seconds). Will address this.
--
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] [incubator-pinot] mcvsubbu commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r674332083
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -74,60 +76,115 @@
}
public static class ControllerPeriodicTasksConf {
+
// frequency configs
+ @Deprecated
public static final String RETENTION_MANAGER_FREQUENCY_IN_SECONDS = "controller.retention.frequencyInSeconds";
+ public static final String RETENTION_MANAGER_FREQUENCY_PERIOD = "controller.retention.frequencyPeriod";
@Deprecated
// The ValidationManager has been split up into 3 separate tasks, each having their own frequency config settings
public static final String DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS =
"controller.validation.frequencyInSeconds";
+ @Deprecated
Review comment:
I was suggesting to just remove `DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS`. It has been there since 0.5.0. I understand this can mean some more code to remove. If that is too much, then you can choose to add a TODO there, saying that this config has been deprecated as of 0.5.0, so must be removed. 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
[GitHub] [incubator-pinot] suddendust commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r672488420
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -428,15 +496,20 @@ public void setRetentionControllerFrequencyInSeconds(int retentionFrequencyInSec
}
/**
- * Returns the config value for controller.offline.segment.interval.checker.frequencyInSeconds if it exists.
- * If it doesn't exist, returns the segment level validation interval. This is done in order to retain the current behavior,
- * wherein the offline validation tasks were done at segment level validation interval frequency
- * The default value is the new DEFAULT_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS
+ * Returns the config value for controller.offline.segment.interval.checker.frequencyInSeconds if
Review comment:
Missed this, will update the docs.
--
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] [incubator-pinot] suddendust commented on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-882723615
Thanks for the review everyone, I will incorporate the suggested changes.
--
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] [incubator-pinot] suddendust commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r674272705
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -74,60 +76,115 @@
}
public static class ControllerPeriodicTasksConf {
+
// frequency configs
+ @Deprecated
public static final String RETENTION_MANAGER_FREQUENCY_IN_SECONDS = "controller.retention.frequencyInSeconds";
+ public static final String RETENTION_MANAGER_FREQUENCY_PERIOD = "controller.retention.frequencyPeriod";
@Deprecated
// The ValidationManager has been split up into 3 separate tasks, each having their own frequency config settings
public static final String DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS =
"controller.validation.frequencyInSeconds";
+ @Deprecated
Review comment:
All deprecated configs have been prefixed with `DEPRECATED.` I will create a new ticket to do this in other places.
--
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] [incubator-pinot] mcvsubbu commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r672444492
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1; // Disabled
private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS = 60 * 60; // 1 Hour.
+ private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS =
+ 60 * 60; // 1 Hour.
private static final int DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS = 60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS = "server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD = "server.request.timeoutPeriod";
+
+ @Deprecated
private static final String SEGMENT_COMMIT_TIMEOUT_SECONDS = "controller.realtime.segment.commit.timeoutSeconds";
+ private static final String SEGMENT_COMMIT_TIMEOUT_PERIOD = "controller.realtime.segment.commit.timeoutPeriod";
Review comment:
This one should not be changed
##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java
##########
@@ -0,0 +1,158 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.controller.ControllerConf.ControllerPeriodicTasksConf.*;
+
+
+public class ControllerConfTest {
+
+ private static final List<String> UNIT_CONFIGS =
+ List.of(RETENTION_MANAGER_FREQUENCY_IN_SECONDS, OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS,
+ OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS, REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS,
+ REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_IN_SECONDS, BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS,
+ BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_IN_SECONDS, STATUS_CHECKER_FREQUENCY_IN_SECONDS,
+ STATUS_CHECKER_WAIT_FOR_PUSH_TIME_IN_SECONDS, TASK_MANAGER_FREQUENCY_IN_SECONDS,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS, MINION_INSTANCES_CLEANUP_TASK_INITIAL_DELAY_SECONDS,
+ MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_SECONDS,
+ TASK_METRICS_EMITTER_FREQUENCY_IN_SECONDS, SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS,
+ SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS, STATUS_CHECKER_INITIAL_DELAY_IN_SECONDS,
+ OFFLINE_SEGMENT_INTERVAL_CHECKER_INITIAL_DELAY_IN_SECONDS,
+ DEPRECATED_REALTIME_SEGMENT_RELOCATION_INITIAL_DELAY_IN_SECONDS, SEGMENT_RELOCATOR_INITIAL_DELAY_IN_SECONDS);
+
+ private static final List<String> PERIOD_CONFIGS =
+ List.of(RETENTION_MANAGER_FREQUENCY_PERIOD, OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD,
+ REALTIME_SEGMENT_VALIDATION_FREQUENCY_PERIOD, REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_PERIOD,
+ BROKER_RESOURCE_VALIDATION_FREQUENCY_PERIOD, BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_PERIOD,
+ STATUS_CHECKER_FREQUENCY_PERIOD, STATUS_CHECKER_WAIT_FOR_PUSH_TIME_PERIOD, TASK_MANAGER_FREQUENCY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_PERIOD, MINION_INSTANCES_CLEANUP_TASK_INITIAL_DELAY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_PERIOD, TASK_METRICS_EMITTER_FREQUENCY_PERIOD,
+ SEGMENT_RELOCATOR_FREQUENCY_PERIOD, SEGMENT_LEVEL_VALIDATION_INTERVAL_PERIOD,
+ STATUS_CHECKER_INITIAL_DELAY_PERIOD, OFFLINE_SEGMENT_INTERVAL_CHECKER_INITIAL_DELAY_PERIOD,
+ SEGMENT_RELOCATOR_INITIAL_DELAY_PERIOD);
+
+ private static final int DURATION_IN_SECONDS = 20;
+ private static final String DURATION_IN_PERIOD_VALID = "2m";
+ private static final int PERIOD_DURATION_IN_SECONDS = 120;
+ private static final String DURATION_IN_PERIOD_INVALID = "2m2m";
+
+ /**
+ * When both type of configs are present, then the human-readable period config overrides the unit config
+ */
+ @Test
+ public void periodConfigOverridesUnitConfig() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ UNIT_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_SECONDS));
+ PERIOD_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_PERIOD_VALID));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, PERIOD_DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When only unit configs are supplied, then the correct converted value is returned.
+ */
+ @Test
+ public void suppliedUnitConfigsShouldReturnCorrectValues() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ UNIT_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_SECONDS));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When only period configs are supplied, then the correct converted value is returned.
+ */
+ @Test
+ public void suppliedPeriodConfigsShouldReturnCorrectValues() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ PERIOD_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_PERIOD_VALID));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, PERIOD_DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When valid unit config and invalid period config are specified, then an {@link IllegalArgumentException} is thrown (valid unit
+ * config does not override invalid period config)
+ */
+ @Test(expectedExceptions = IllegalArgumentException.class)
Review comment:
Expected exceptions are good, but I prefer fielding specific exceptions at the specific points where we expect them. That makes it easy to add more code to the test, or re-factor things, etc. and not worry about the test passing if we accidentally get IllegalArgumentException some place else.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1; // Disabled
private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS = 60 * 60; // 1 Hour.
+ private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS =
+ 60 * 60; // 1 Hour.
private static final int DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS = 60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS = "server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD = "server.request.timeoutPeriod";
Review comment:
This one should not be changed. We don't expect people to configure this value in minutes or hours.
--
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] [incubator-pinot] codecov-commenter edited a comment on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-883325800
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?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 [#7173](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b910b27) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/6d0ab91de409fec8ddbf816c2348c8b95a37e393?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6d0ab91) will **decrease** coverage by `0.00%`.
> The diff coverage is `77.61%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7173 +/- ##
============================================
- Coverage 73.49% 73.49% -0.01%
Complexity 92 92
============================================
Files 1500 1506 +6
Lines 73758 73834 +76
Branches 10637 10652 +15
============================================
+ Hits 54209 54264 +55
- Misses 16005 16035 +30
+ Partials 3544 3535 -9
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.98% <76.11%> (+0.09%)` | :arrow_up: |
| unittests | `65.23% <65.67%> (-0.04%)` | :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/incubator-pinot/pull/7173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `56.91% <77.61%> (+4.05%)` | :arrow_up: |
| [...va/org/apache/pinot/common/minion/Granularity.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWluaW9uL0dyYW51bGFyaXR5LmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...re/segment/processing/filter/NoOpRecordFilter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvZmlsdGVyL05vT3BSZWNvcmRGaWx0ZXIuamF2YQ==) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [.../processing/transformer/NoOpRecordTransformer.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvdHJhbnNmb3JtZXIvTm9PcFJlY29yZFRyYW5zZm9ybWVyLmphdmE=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...rocessing/transformer/RecordTransformerConfig.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvdHJhbnNmb3JtZXIvUmVjb3JkVHJhbnNmb3JtZXJDb25maWcuamF2YQ==) | `88.88% <0.00%> (-11.12%)` | :arrow_down: |
| [...ntroller/helix/core/minion/TaskMetricsEmitter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9UYXNrTWV0cmljc0VtaXR0ZXIuamF2YQ==) | `77.27% <0.00%> (-9.10%)` | :arrow_down: |
| [.../segment/processing/filter/RecordFilterConfig.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvZmlsdGVyL1JlY29yZEZpbHRlckNvbmZpZy5qYXZh) | `93.33% <0.00%> (-6.67%)` | :arrow_down: |
| [...mpl/dictionary/DoubleOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT2ZmSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `57.44% <0.00%> (-5.32%)` | :arrow_down: |
| [...core/query/executor/ServerQueryExecutorV1Impl.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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=) | `81.42% <0.00%> (-4.92%)` | :arrow_down: |
| [...lix/core/minion/PinotHelixTaskResourceManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9QaW5vdEhlbGl4VGFza1Jlc291cmNlTWFuYWdlci5qYXZh) | `72.00% <0.00%> (-4.67%)` | :arrow_down: |
| ... and [46 more](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6d0ab91...b910b27](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-pinot] mcvsubbu commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r675012518
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -568,17 +666,21 @@ public void setDeletedSegmentsRetentionInDays(int retentionInDays) {
}
public int getTaskManagerFrequencyInSeconds() {
- return getProperty(ControllerPeriodicTasksConf.TASK_MANAGER_FREQUENCY_IN_SECONDS,
- ControllerPeriodicTasksConf.DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS);
+ return Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.TASK_MANAGER_FREQUENCY_PERIOD))
Review comment:
ah, got 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
[GitHub] [incubator-pinot] suddendust commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r675297116
##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java
##########
@@ -0,0 +1,193 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Random;
+import java.util.concurrent.TimeUnit;
+import org.apache.commons.lang3.RandomStringUtils;
+import org.apache.pinot.spi.utils.TimeUtils;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.controller.ControllerConf.ControllerPeriodicTasksConf.*;
+
+
+public class ControllerConfTest {
+
+ private static final List<String> DEPRECATED_CONFIGS = Arrays
+ .asList(DEPRECATED_RETENTION_MANAGER_FREQUENCY_IN_SECONDS,
+ DEPRECATED_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS,
+ DEPRECATED_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS,
+ DEPRECATED_REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS,
+ DEPRECATED_BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS, DEPRECATED_STATUS_CHECKER_FREQUENCY_IN_SECONDS,
+ DEPRECATED_TASK_MANAGER_FREQUENCY_IN_SECONDS, DEPRECATED_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS,
+ DEPRECATED_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_SECONDS,
+ DEPRECATED_TASK_METRICS_EMITTER_FREQUENCY_IN_SECONDS, DEPRECATED_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS,
+ DEPRECATED_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS,
+ DEPRECATED_REALTIME_SEGMENT_RELOCATION_INITIAL_DELAY_IN_SECONDS,
+ DEPRECATED_STATUS_CHECKER_WAIT_FOR_PUSH_TIME_IN_SECONDS);
+
+ private static final List<String> NEW_CONFIGS = Arrays
+ .asList(RETENTION_MANAGER_FREQUENCY_PERIOD, OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD,
+ REALTIME_SEGMENT_VALIDATION_FREQUENCY_PERIOD, BROKER_RESOURCE_VALIDATION_FREQUENCY_PERIOD,
+ STATUS_CHECKER_FREQUENCY_PERIOD, TASK_MANAGER_FREQUENCY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_PERIOD, TASK_METRICS_EMITTER_FREQUENCY_PERIOD,
+ SEGMENT_RELOCATOR_FREQUENCY_PERIOD, SEGMENT_LEVEL_VALIDATION_INTERVAL_PERIOD,
+ STATUS_CHECKER_WAIT_FOR_PUSH_TIME_PERIOD);
+
+ private static final Random RAND = new Random();
+
+ /**
+ * When config contains: 1. Both deprecated config and the corresponding new config. 2. All new
+ * configurations are valid. 3. Some deprecated configurations are invalid. then new configs
+ * override deprecated configs (invalid deprecated configs do not throw exceptions when
+ * corresponding valid new configs are supplied as well)
+ */
+ @Test
+ public void validNewConfigOverridesCorrespondingValidOrInvalidOldConfigOnRead() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ int durationInSeconds = getRandomDurationInSeconds();
+ DEPRECATED_CONFIGS.forEach(config -> controllerConfig.put(config, durationInSeconds));
+ //put some invalid deprecated configs
+ controllerConfig.put(DEPRECATED_RETENTION_MANAGER_FREQUENCY_IN_SECONDS, getRandomString());
+ controllerConfig.put(DEPRECATED_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS, getRandomString());
+ //override all deprecated configs with valid new configs
+ String period = getRandomPeriodInMinutes();
+ NEW_CONFIGS.forEach(config -> controllerConfig.put(config, period));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, TimeUnit.SECONDS.convert(TimeUtils.convertPeriodToMillis(period), TimeUnit.MILLISECONDS));
+ }
+
+ /**
+ * When config contains: 1. Both deprecated config and the corresponding new config. 2. All
+ * deprecated configurations are valid. 3. Some new configurations are invalid. then exceptions
+ * are thrown when invalid new configurations are read (there is no fall-back to the corresponding
+ * valid deprecated configuration). For all valid new configurations, they override the
+ * corresponding deprecated configuration.
+ */
+ @Test
+ public void invalidNewConfigShouldThrowExceptionOnReadWithoutFallbackToCorrespondingValidDeprecatedConfig() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ int durationInSeconds = getRandomDurationInSeconds();
+ //all deprecated configs should be valid
+ DEPRECATED_CONFIGS.forEach(config -> controllerConfig.put(config, durationInSeconds));
+ String randomPeriodInMinutes = getRandomPeriodInMinutes();
Review comment:
While this definitely is needed for reproducing failing tests, in this case, simply printing the test configuration gives enough information to reproduce a failing test. So I have simply printing the config for any failing 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] [incubator-pinot] suddendust commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r673853047
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -74,60 +76,115 @@
}
public static class ControllerPeriodicTasksConf {
+
// frequency configs
+ @Deprecated
public static final String RETENTION_MANAGER_FREQUENCY_IN_SECONDS = "controller.retention.frequencyInSeconds";
+ public static final String RETENTION_MANAGER_FREQUENCY_PERIOD = "controller.retention.frequencyPeriod";
@Deprecated
// The ValidationManager has been split up into 3 separate tasks, each having their own frequency config settings
public static final String DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS =
"controller.validation.frequencyInSeconds";
+ @Deprecated
public static final String OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS =
"controller.offline.segment.interval.checker.frequencyInSeconds";
+ public static final String OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD =
+ "controller.offline.segment.interval.checker.frequencyPeriod";
+ @Deprecated
public static final String REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS =
"controller.realtime.segment.validation.frequencyInSeconds";
+ public static final String REALTIME_SEGMENT_VALIDATION_FREQUENCY_PERIOD =
+ "controller.realtime.segment.validation.frequencyPeriod";
+ @Deprecated
public static final String REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_IN_SECONDS =
"controller.realtime.segment.validation.initialDelayInSeconds";
+ public static final String REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_PERIOD =
+ "controller.realtime.segment.validation.initialDelayPeriod";
+ @Deprecated
public static final String BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS =
"controller.broker.resource.validation.frequencyInSeconds";
+ public static final String BROKER_RESOURCE_VALIDATION_FREQUENCY_PERIOD =
+ "controller.broker.resource.validation.frequencyPeriod";
+ @Deprecated
public static final String BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_IN_SECONDS =
"controller.broker.resource.validation.initialDelayInSeconds";
+ public static final String BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_PERIOD =
+ "controller.broker.resource.validation.initialDelayPeriod";
+ @Deprecated
public static final String STATUS_CHECKER_FREQUENCY_IN_SECONDS = "controller.statuschecker.frequencyInSeconds";
+ public static final String STATUS_CHECKER_FREQUENCY_PERIOD = "controller.statuschecker.frequencyPeriod";
+ @Deprecated
public static final String STATUS_CHECKER_WAIT_FOR_PUSH_TIME_IN_SECONDS =
"controller.statuschecker.waitForPushTimeInSeconds";
+ public static final String STATUS_CHECKER_WAIT_FOR_PUSH_TIME_PERIOD =
Review comment:
@mcvsubbu this config's default value in [docs](https://docs.pinot.apache.org/configuration-reference/controller#:~:text=controller.statuschecker.waitForPushTimeInSeconds) is 10 minutes. So can't we expect that the user might want to supply a different value in minutes again?
--
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] [incubator-pinot] suddendust commented on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-885389139
> Other than one minor request of adding a comment, we are good to merge this.
Addressed the changes.
--
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] [incubator-pinot] codecov-commenter commented on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-883325800
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?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 [#7173](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (43bb91e) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/6d0ab91de409fec8ddbf816c2348c8b95a37e393?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6d0ab91) will **decrease** coverage by `31.70%`.
> The diff coverage is `97.82%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7173 +/- ##
=============================================
- Coverage 73.49% 41.79% -31.71%
+ Complexity 92 7 -85
=============================================
Files 1500 1506 +6
Lines 73758 73875 +117
Branches 10637 10657 +20
=============================================
- Hits 54209 30875 -23334
- Misses 16005 40381 +24376
+ Partials 3544 2619 -925
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.79% <97.82%> (-0.10%)` | :arrow_down: |
| unittests | `?` | |
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/incubator-pinot/pull/7173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `54.81% <97.82%> (+1.95%)` | :arrow_up: |
| [...c/main/java/org/apache/pinot/common/tier/Tier.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/spi/config/table/TableType.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlVHlwZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/spi/data/DateTimeFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EYXRlVGltZUZpZWxkU3BlYy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pinot/spi/data/DimensionFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EaW1lbnNpb25GaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pinot/spi/data/readers/FileFormat.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9yZWFkZXJzL0ZpbGVGb3JtYXQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/common/minion/Granularity.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWluaW9uL0dyYW51bGFyaXR5LmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [954 more](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6d0ab91...43bb91e](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-pinot] jackjlli commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r674352132
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/UpsertTableSegmentUploadIntegrationTest.java
##########
@@ -152,8 +152,8 @@ protected void startController()
Map<String, Object> controllerConfig = getDefaultControllerConfiguration();
// Perform realtime segment validation every second with 1 second initial delay.
controllerConfig
- .put(ControllerConf.ControllerPeriodicTasksConf.REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS, 1);
- controllerConfig.put(ControllerConf.ControllerPeriodicTasksConf.SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS, 1);
+ .put(ControllerConf.ControllerPeriodicTasksConf.DEPRECATED_REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS, 1);
Review comment:
The deprecated configs should only be used in the getter/setter method for better cleanup purpose. Maybe we should start changing these config to the ones you introduced in this PR?
--
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] [incubator-pinot] jackjlli commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r672490885
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -341,8 +406,9 @@ public String getDataDir() {
}
public int getSegmentCommitTimeoutSeconds() {
Review comment:
Right, I wrote this one because I saw some of the java docs for the methods have been adjusted but it talks about the other way around. So mention it here.
--
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] [incubator-pinot] suddendust commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r674272088
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -568,17 +666,21 @@ public void setDeletedSegmentsRetentionInDays(int retentionInDays) {
}
public int getTaskManagerFrequencyInSeconds() {
- return getProperty(ControllerPeriodicTasksConf.TASK_MANAGER_FREQUENCY_IN_SECONDS,
- ControllerPeriodicTasksConf.DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS);
+ return Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.TASK_MANAGER_FREQUENCY_PERIOD))
Review comment:
Handled this in the latest commit and added a UT to catch this. This wasn't handled earlier.
--
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] [incubator-pinot] suddendust commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r672484276
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1; // Disabled
private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS = 60 * 60; // 1 Hour.
+ private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS =
+ 60 * 60; // 1 Hour.
private static final int DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS = 60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS = "server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD = "server.request.timeoutPeriod";
Review comment:
Yes makes sense. My thought process was that the configuration should move towards taking everything as human-readable strings and converting it to seconds (to keep it generic, basically if user wanted to keep the timeout as 2m, he should be able to).
--
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] [incubator-pinot] suddendust commented on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-883610973
> Please make sure that you change only those that actually need to be in minutes or hours. If something needs to be in seconds (or even lower) chances are that the default values will work, and hardly anyone will change it. So, ew don't need to add extra code to introduce new config and deprecate old one, etc. Not to mention someone setting a very high value and maybe causing other things to break.
Yes @mcvsubbu actually I was my thought process along the lines of keeping the configuration consistent - That is, even if we always expect a config in seconds, and the user say gives 2m as the config, we convert it to 120s and use it. But your points show this can be fragile. I will remove such configs.
--
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] [incubator-pinot] suddendust commented on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-883274839
Addressed earlier 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.
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] [incubator-pinot] mcvsubbu commented on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-883605329
Alsobe sure to document these in https://docs.pinot.apache.org/configuration-reference/controller
I guess the convention is to keep the old config and mark them as deprecated in the doc, and add the new config. Be sure to add an example in the "Description" column.
--
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] [incubator-pinot] mcvsubbu commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r673379306
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -568,17 +666,21 @@ public void setDeletedSegmentsRetentionInDays(int retentionInDays) {
}
public int getTaskManagerFrequencyInSeconds() {
- return getProperty(ControllerPeriodicTasksConf.TASK_MANAGER_FREQUENCY_IN_SECONDS,
- ControllerPeriodicTasksConf.DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS);
+ return Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.TASK_MANAGER_FREQUENCY_PERIOD))
Review comment:
Does this work if value is set to "-1" in the new config (that is the default value).
--
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] [incubator-pinot] suddendust commented on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-884492143
> also, in all the changes that you make, if you can prefix the configs with a comment like this:
> // Deprecated as of 0.8.0
> Then, it is easy for someone to remove it a few releases down the road.
>
> Other than these minor comments, LGTM
>
> Thanks for your contribution
Hey sure @mcvsubbu it is a pleasure to contribute to Pinot :) I will make these changes and raise another PR after updating the documentation. Thanks for your thorough review.
--
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] [incubator-pinot] suddendust commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r673853047
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -74,60 +76,115 @@
}
public static class ControllerPeriodicTasksConf {
+
// frequency configs
+ @Deprecated
public static final String RETENTION_MANAGER_FREQUENCY_IN_SECONDS = "controller.retention.frequencyInSeconds";
+ public static final String RETENTION_MANAGER_FREQUENCY_PERIOD = "controller.retention.frequencyPeriod";
@Deprecated
// The ValidationManager has been split up into 3 separate tasks, each having their own frequency config settings
public static final String DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS =
"controller.validation.frequencyInSeconds";
+ @Deprecated
public static final String OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS =
"controller.offline.segment.interval.checker.frequencyInSeconds";
+ public static final String OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD =
+ "controller.offline.segment.interval.checker.frequencyPeriod";
+ @Deprecated
public static final String REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS =
"controller.realtime.segment.validation.frequencyInSeconds";
+ public static final String REALTIME_SEGMENT_VALIDATION_FREQUENCY_PERIOD =
+ "controller.realtime.segment.validation.frequencyPeriod";
+ @Deprecated
public static final String REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_IN_SECONDS =
"controller.realtime.segment.validation.initialDelayInSeconds";
+ public static final String REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_PERIOD =
+ "controller.realtime.segment.validation.initialDelayPeriod";
+ @Deprecated
public static final String BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS =
"controller.broker.resource.validation.frequencyInSeconds";
+ public static final String BROKER_RESOURCE_VALIDATION_FREQUENCY_PERIOD =
+ "controller.broker.resource.validation.frequencyPeriod";
+ @Deprecated
public static final String BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_IN_SECONDS =
"controller.broker.resource.validation.initialDelayInSeconds";
+ public static final String BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_PERIOD =
+ "controller.broker.resource.validation.initialDelayPeriod";
+ @Deprecated
public static final String STATUS_CHECKER_FREQUENCY_IN_SECONDS = "controller.statuschecker.frequencyInSeconds";
+ public static final String STATUS_CHECKER_FREQUENCY_PERIOD = "controller.statuschecker.frequencyPeriod";
+ @Deprecated
public static final String STATUS_CHECKER_WAIT_FOR_PUSH_TIME_IN_SECONDS =
"controller.statuschecker.waitForPushTimeInSeconds";
+ public static final String STATUS_CHECKER_WAIT_FOR_PUSH_TIME_PERIOD =
Review comment:
@mcvsubbu this config's default value in the [docs](https://docs.pinot.apache.org/configuration-reference/controller#:~:text=controller.statuschecker.waitForPushTimeInSeconds) is 10 minutes. So can't we expect that the user might want to supply a different value in minutes again?
--
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] [incubator-pinot] mcvsubbu commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r673272036
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -74,60 +76,115 @@
}
public static class ControllerPeriodicTasksConf {
+
// frequency configs
+ @Deprecated
public static final String RETENTION_MANAGER_FREQUENCY_IN_SECONDS = "controller.retention.frequencyInSeconds";
+ public static final String RETENTION_MANAGER_FREQUENCY_PERIOD = "controller.retention.frequencyPeriod";
@Deprecated
// The ValidationManager has been split up into 3 separate tasks, each having their own frequency config settings
public static final String DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS =
"controller.validation.frequencyInSeconds";
+ @Deprecated
Review comment:
Another (separate) task can be to remove the `DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS` config and related code. I believe this has been deprecated for a few releases now, we can remove it.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -74,60 +76,115 @@
}
public static class ControllerPeriodicTasksConf {
+
// frequency configs
+ @Deprecated
public static final String RETENTION_MANAGER_FREQUENCY_IN_SECONDS = "controller.retention.frequencyInSeconds";
+ public static final String RETENTION_MANAGER_FREQUENCY_PERIOD = "controller.retention.frequencyPeriod";
@Deprecated
// The ValidationManager has been split up into 3 separate tasks, each having their own frequency config settings
public static final String DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS =
"controller.validation.frequencyInSeconds";
+ @Deprecated
public static final String OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS =
"controller.offline.segment.interval.checker.frequencyInSeconds";
+ public static final String OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD =
+ "controller.offline.segment.interval.checker.frequencyPeriod";
+ @Deprecated
public static final String REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS =
"controller.realtime.segment.validation.frequencyInSeconds";
+ public static final String REALTIME_SEGMENT_VALIDATION_FREQUENCY_PERIOD =
+ "controller.realtime.segment.validation.frequencyPeriod";
+ @Deprecated
public static final String REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_IN_SECONDS =
"controller.realtime.segment.validation.initialDelayInSeconds";
+ public static final String REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_PERIOD =
+ "controller.realtime.segment.validation.initialDelayPeriod";
+ @Deprecated
public static final String BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS =
"controller.broker.resource.validation.frequencyInSeconds";
+ public static final String BROKER_RESOURCE_VALIDATION_FREQUENCY_PERIOD =
+ "controller.broker.resource.validation.frequencyPeriod";
+ @Deprecated
public static final String BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_IN_SECONDS =
"controller.broker.resource.validation.initialDelayInSeconds";
+ public static final String BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_PERIOD =
+ "controller.broker.resource.validation.initialDelayPeriod";
+ @Deprecated
public static final String STATUS_CHECKER_FREQUENCY_IN_SECONDS = "controller.statuschecker.frequencyInSeconds";
+ public static final String STATUS_CHECKER_FREQUENCY_PERIOD = "controller.statuschecker.frequencyPeriod";
+ @Deprecated
public static final String STATUS_CHECKER_WAIT_FOR_PUSH_TIME_IN_SECONDS =
"controller.statuschecker.waitForPushTimeInSeconds";
+ public static final String STATUS_CHECKER_WAIT_FOR_PUSH_TIME_PERIOD =
Review comment:
This config is (and please add this doc here) to make sure that a recently pushed segment is not accidentally flagged as bad just because it does not reflect in external view as yet. So, this time should be in seconds all the time. Please revert this change.
--
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] [incubator-pinot] suddendust commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r674270861
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -74,60 +76,115 @@
}
public static class ControllerPeriodicTasksConf {
+
// frequency configs
+ @Deprecated
public static final String RETENTION_MANAGER_FREQUENCY_IN_SECONDS = "controller.retention.frequencyInSeconds";
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] [incubator-pinot] suddendust commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r672484276
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1; // Disabled
private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS = 60 * 60; // 1 Hour.
+ private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS =
+ 60 * 60; // 1 Hour.
private static final int DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS = 60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS = "server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD = "server.request.timeoutPeriod";
Review comment:
Yes makes sense. My thought process was that the configuration should move towards taking everything as human-readable strings and converting it to whatever units it wants to (to keep it generic, basically if user wanted to keep the timeout as 2m, he should be able to. The config takes care to changing of to seconds). Will address this.
--
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] [incubator-pinot] mcvsubbu commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r675008239
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -568,17 +666,21 @@ public void setDeletedSegmentsRetentionInDays(int retentionInDays) {
}
public int getTaskManagerFrequencyInSeconds() {
- return getProperty(ControllerPeriodicTasksConf.TASK_MANAGER_FREQUENCY_IN_SECONDS,
- ControllerPeriodicTasksConf.DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS);
+ return Optional.ofNullable(getProperty(ControllerPeriodicTasksConf.TASK_MANAGER_FREQUENCY_PERIOD))
Review comment:
I am sure I am missing something. I don't see the UT for this case?
##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java
##########
@@ -0,0 +1,193 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Random;
+import java.util.concurrent.TimeUnit;
+import org.apache.commons.lang3.RandomStringUtils;
+import org.apache.pinot.spi.utils.TimeUtils;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.controller.ControllerConf.ControllerPeriodicTasksConf.*;
+
+
+public class ControllerConfTest {
+
+ private static final List<String> DEPRECATED_CONFIGS = Arrays
+ .asList(DEPRECATED_RETENTION_MANAGER_FREQUENCY_IN_SECONDS,
+ DEPRECATED_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS,
+ DEPRECATED_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS,
+ DEPRECATED_REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS,
+ DEPRECATED_BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS, DEPRECATED_STATUS_CHECKER_FREQUENCY_IN_SECONDS,
+ DEPRECATED_TASK_MANAGER_FREQUENCY_IN_SECONDS, DEPRECATED_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS,
+ DEPRECATED_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_SECONDS,
+ DEPRECATED_TASK_METRICS_EMITTER_FREQUENCY_IN_SECONDS, DEPRECATED_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS,
+ DEPRECATED_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS,
+ DEPRECATED_REALTIME_SEGMENT_RELOCATION_INITIAL_DELAY_IN_SECONDS,
+ DEPRECATED_STATUS_CHECKER_WAIT_FOR_PUSH_TIME_IN_SECONDS);
+
+ private static final List<String> NEW_CONFIGS = Arrays
+ .asList(RETENTION_MANAGER_FREQUENCY_PERIOD, OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD,
+ REALTIME_SEGMENT_VALIDATION_FREQUENCY_PERIOD, BROKER_RESOURCE_VALIDATION_FREQUENCY_PERIOD,
+ STATUS_CHECKER_FREQUENCY_PERIOD, TASK_MANAGER_FREQUENCY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_PERIOD, TASK_METRICS_EMITTER_FREQUENCY_PERIOD,
+ SEGMENT_RELOCATOR_FREQUENCY_PERIOD, SEGMENT_LEVEL_VALIDATION_INTERVAL_PERIOD,
+ STATUS_CHECKER_WAIT_FOR_PUSH_TIME_PERIOD);
+
+ private static final Random RAND = new Random();
+
+ /**
+ * When config contains: 1. Both deprecated config and the corresponding new config. 2. All new
+ * configurations are valid. 3. Some deprecated configurations are invalid. then new configs
+ * override deprecated configs (invalid deprecated configs do not throw exceptions when
+ * corresponding valid new configs are supplied as well)
+ */
+ @Test
+ public void validNewConfigOverridesCorrespondingValidOrInvalidOldConfigOnRead() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ int durationInSeconds = getRandomDurationInSeconds();
+ DEPRECATED_CONFIGS.forEach(config -> controllerConfig.put(config, durationInSeconds));
+ //put some invalid deprecated configs
+ controllerConfig.put(DEPRECATED_RETENTION_MANAGER_FREQUENCY_IN_SECONDS, getRandomString());
+ controllerConfig.put(DEPRECATED_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS, getRandomString());
+ //override all deprecated configs with valid new configs
+ String period = getRandomPeriodInMinutes();
+ NEW_CONFIGS.forEach(config -> controllerConfig.put(config, period));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, TimeUnit.SECONDS.convert(TimeUtils.convertPeriodToMillis(period), TimeUnit.MILLISECONDS));
+ }
+
+ /**
+ * When config contains: 1. Both deprecated config and the corresponding new config. 2. All
+ * deprecated configurations are valid. 3. Some new configurations are invalid. then exceptions
+ * are thrown when invalid new configurations are read (there is no fall-back to the corresponding
+ * valid deprecated configuration). For all valid new configurations, they override the
+ * corresponding deprecated configuration.
+ */
+ @Test
+ public void invalidNewConfigShouldThrowExceptionOnReadWithoutFallbackToCorrespondingValidDeprecatedConfig() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ int durationInSeconds = getRandomDurationInSeconds();
+ //all deprecated configs should be valid
+ DEPRECATED_CONFIGS.forEach(config -> controllerConfig.put(config, durationInSeconds));
+ String randomPeriodInMinutes = getRandomPeriodInMinutes();
Review comment:
In this case maybe it is ok, but in general, it is a good idea to set the seed for the random number up front, and print out the value of the seed (in system.out). This will make sure that if there is failure due to some random value, we can reproduce the test case by manually setting the seed to that value. Otherwise, we will face intermittent failures in tests without knowing why that happened.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -74,60 +76,115 @@
}
public static class ControllerPeriodicTasksConf {
+
// frequency configs
+ @Deprecated
public static final String RETENTION_MANAGER_FREQUENCY_IN_SECONDS = "controller.retention.frequencyInSeconds";
+ public static final String RETENTION_MANAGER_FREQUENCY_PERIOD = "controller.retention.frequencyPeriod";
@Deprecated
// The ValidationManager has been split up into 3 separate tasks, each having their own frequency config settings
public static final String DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS =
"controller.validation.frequencyInSeconds";
+ @Deprecated
Review comment:
Can you either remove `DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS` or add a comment before line 80 saying it has been deprecated since 0.5.0
--
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] [incubator-pinot] mcvsubbu merged pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
mcvsubbu merged pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173
--
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] [incubator-pinot] suddendust commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r672487276
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -341,8 +406,9 @@ public String getDataDir() {
}
public int getSegmentCommitTimeoutSeconds() {
Review comment:
"The behavior should be that the new one should be used if both are specified or only the new one be specified" - Isn't it what it is doing @jackjlli? Basically if the new config is absent, it falls-back to the older one. But method still does what it says - `getSegmentCommitTimeoutSeconds`.
--
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] [incubator-pinot] codecov-commenter edited a comment on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-883325800
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?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 [#7173](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (43bb91e) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/6d0ab91de409fec8ddbf816c2348c8b95a37e393?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6d0ab91) will **increase** coverage by `0.01%`.
> The diff coverage is `97.82%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7173 +/- ##
============================================
+ Coverage 73.49% 73.50% +0.01%
Complexity 92 92
============================================
Files 1500 1506 +6
Lines 73758 73875 +117
Branches 10637 10657 +20
============================================
+ Hits 54209 54305 +96
- Misses 16005 16030 +25
+ Partials 3544 3540 -4
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.79% <97.82%> (-0.10%)` | :arrow_down: |
| unittests | `65.26% <89.13%> (-0.01%)` | :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/incubator-pinot/pull/7173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `61.11% <97.82%> (+8.24%)` | :arrow_up: |
| [...re/segment/processing/filter/NoOpRecordFilter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvZmlsdGVyL05vT3BSZWNvcmRGaWx0ZXIuamF2YQ==) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [.../processing/transformer/NoOpRecordTransformer.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvdHJhbnNmb3JtZXIvTm9PcFJlY29yZFRyYW5zZm9ybWVyLmphdmE=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...rocessing/transformer/RecordTransformerConfig.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvdHJhbnNmb3JtZXIvUmVjb3JkVHJhbnNmb3JtZXJDb25maWcuamF2YQ==) | `88.88% <0.00%> (-11.12%)` | :arrow_down: |
| [...ntroller/helix/core/minion/TaskMetricsEmitter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9UYXNrTWV0cmljc0VtaXR0ZXIuamF2YQ==) | `77.27% <0.00%> (-9.10%)` | :arrow_down: |
| [.../segment/processing/filter/RecordFilterConfig.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvZmlsdGVyL1JlY29yZEZpbHRlckNvbmZpZy5qYXZh) | `93.33% <0.00%> (-6.67%)` | :arrow_down: |
| [.../impl/dictionary/FloatOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRmxvYXRPbkhlYXBNdXRhYmxlRGljdGlvbmFyeS5qYXZh) | `69.87% <0.00%> (-6.03%)` | :arrow_down: |
| [...mpl/dictionary/DoubleOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT2ZmSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `57.44% <0.00%> (-5.32%)` | :arrow_down: |
| [...lix/core/minion/PinotHelixTaskResourceManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9QaW5vdEhlbGl4VGFza1Jlc291cmNlTWFuYWdlci5qYXZh) | `72.00% <0.00%> (-4.67%)` | :arrow_down: |
| [...oller/api/resources/PinotTableRestletResource.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90VGFibGVSZXN0bGV0UmVzb3VyY2UuamF2YQ==) | `50.17% <0.00%> (-4.58%)` | :arrow_down: |
| ... and [39 more](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6d0ab91...43bb91e](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-pinot] codecov-commenter edited a comment on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-883325800
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?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 [#7173](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (eeb896e) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/6d0ab91de409fec8ddbf816c2348c8b95a37e393?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6d0ab91) will **increase** coverage by `0.06%`.
> The diff coverage is `74.57%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7173 +/- ##
============================================
+ Coverage 73.49% 73.55% +0.06%
Complexity 92 92
============================================
Files 1500 1506 +6
Lines 73758 73826 +68
Branches 10637 10650 +13
============================================
+ Hits 54209 54306 +97
+ Misses 16005 15973 -32
- Partials 3544 3547 +3
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.91% <72.88%> (+0.01%)` | :arrow_up: |
| unittests | `65.25% <67.79%> (-0.02%)` | :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/incubator-pinot/pull/7173?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/controller/ControllerConf.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9Db250cm9sbGVyQ29uZi5qYXZh) | `55.51% <74.57%> (+2.64%)` | :arrow_up: |
| [...va/org/apache/pinot/common/minion/Granularity.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWluaW9uL0dyYW51bGFyaXR5LmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...re/segment/processing/filter/NoOpRecordFilter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvZmlsdGVyL05vT3BSZWNvcmRGaWx0ZXIuamF2YQ==) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [.../processing/transformer/NoOpRecordTransformer.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvdHJhbnNmb3JtZXIvTm9PcFJlY29yZFRyYW5zZm9ybWVyLmphdmE=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...readers/constant/ConstantMVForwardIndexReader.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvY29uc3RhbnQvQ29uc3RhbnRNVkZvcndhcmRJbmRleFJlYWRlci5qYXZh) | `14.28% <0.00%> (-28.58%)` | :arrow_down: |
| [...rocessing/transformer/RecordTransformerConfig.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvdHJhbnNmb3JtZXIvUmVjb3JkVHJhbnNmb3JtZXJDb25maWcuamF2YQ==) | `88.88% <0.00%> (-11.12%)` | :arrow_down: |
| [...ore/query/scheduler/resources/ResourceManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zY2hlZHVsZXIvcmVzb3VyY2VzL1Jlc291cmNlTWFuYWdlci5qYXZh) | `85.71% <0.00%> (-10.72%)` | :arrow_down: |
| [.../segment/processing/filter/RecordFilterConfig.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3Byb2Nlc3NpbmcvZmlsdGVyL1JlY29yZEZpbHRlckNvbmZpZy5qYXZh) | `93.33% <0.00%> (-6.67%)` | :arrow_down: |
| [...e/impl/dictionary/LongOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvTG9uZ09uSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `63.85% <0.00%> (-6.03%)` | :arrow_down: |
| [.../pinot/core/query/scheduler/PriorityScheduler.java](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zY2hlZHVsZXIvUHJpb3JpdHlTY2hlZHVsZXIuamF2YQ==) | `78.08% <0.00%> (-5.48%)` | :arrow_down: |
| ... and [52 more](https://codecov.io/gh/apache/incubator-pinot/pull/7173/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6d0ab91...eeb896e](https://codecov.io/gh/apache/incubator-pinot/pull/7173?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-pinot] suddendust commented on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-883611897
> Alsobe sure to document these in https://docs.pinot.apache.org/configuration-reference/controller
>
> I guess the convention is to keep the old config and mark them as deprecated in the doc, and add the new config. Be sure to add an example in the "Description" column.
This is a WIP. I was actually confident that some of these new configurations would be rolled-back, so I waited till a review was 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] [incubator-pinot] jackjlli commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r672465207
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -341,8 +406,9 @@ public String getDataDir() {
}
public int getSegmentCommitTimeoutSeconds() {
Review comment:
I feel that these method names also need to be changed, or have a new method here and mark the old one as deprecated.
The behavior should be that the new one should be used if both are specified or only the new one be specified. If not, use the old one.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -428,15 +496,20 @@ public void setRetentionControllerFrequencyInSeconds(int retentionFrequencyInSec
}
/**
- * Returns the config value for controller.offline.segment.interval.checker.frequencyInSeconds if it exists.
- * If it doesn't exist, returns the segment level validation interval. This is done in order to retain the current behavior,
- * wherein the offline validation tasks were done at segment level validation interval frequency
- * The default value is the new DEFAULT_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS
+ * Returns the config value for controller.offline.segment.interval.checker.frequencyInSeconds if
Review comment:
We should check the new one first and then the old one, right?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -341,8 +406,9 @@ public String getDataDir() {
}
public int getSegmentCommitTimeoutSeconds() {
Review comment:
Right, I wrote this one because I saw some of the java docs for the methods have been adjusted but it talks about the other way around. So mention it here.
--
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] [incubator-pinot] suddendust edited a comment on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust edited a comment on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-882012741
No setters have been added for the newly added configs as configs are meant to be immutable and these methods are deprecated for existing configs.
Also, I cannot add labels for some reason. This PR needs the "release notes" label.
--
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] [incubator-pinot] suddendust commented on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-884700008
PR for docs update: https://github.com/pinot-contrib/pinot-docs/pull/47
--
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] [incubator-pinot] suddendust commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r672484276
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1; // Disabled
private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS = 60 * 60; // 1 Hour.
+ private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS =
+ 60 * 60; // 1 Hour.
private static final int DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS = 60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS = "server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD = "server.request.timeoutPeriod";
Review comment:
Yes makes sense. My thought process was that the configuration should move towards taking everything as human-readable strings and converting it to whatever units it wants to (to keep it generic, basically if user wanted to keep the timeout as 2m, he should be able to. The config takes care to changing it to seconds). Will address this.
--
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] [incubator-pinot] mcvsubbu commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r672444492
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1; // Disabled
private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS = 60 * 60; // 1 Hour.
+ private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS =
+ 60 * 60; // 1 Hour.
private static final int DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS = 60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS = "server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD = "server.request.timeoutPeriod";
+
+ @Deprecated
private static final String SEGMENT_COMMIT_TIMEOUT_SECONDS = "controller.realtime.segment.commit.timeoutSeconds";
+ private static final String SEGMENT_COMMIT_TIMEOUT_PERIOD = "controller.realtime.segment.commit.timeoutPeriod";
Review comment:
This one should not be changed
##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java
##########
@@ -0,0 +1,158 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.controller.ControllerConf.ControllerPeriodicTasksConf.*;
+
+
+public class ControllerConfTest {
+
+ private static final List<String> UNIT_CONFIGS =
+ List.of(RETENTION_MANAGER_FREQUENCY_IN_SECONDS, OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS,
+ OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS, REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS,
+ REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_IN_SECONDS, BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS,
+ BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_IN_SECONDS, STATUS_CHECKER_FREQUENCY_IN_SECONDS,
+ STATUS_CHECKER_WAIT_FOR_PUSH_TIME_IN_SECONDS, TASK_MANAGER_FREQUENCY_IN_SECONDS,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS, MINION_INSTANCES_CLEANUP_TASK_INITIAL_DELAY_SECONDS,
+ MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_SECONDS,
+ TASK_METRICS_EMITTER_FREQUENCY_IN_SECONDS, SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS,
+ SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS, STATUS_CHECKER_INITIAL_DELAY_IN_SECONDS,
+ OFFLINE_SEGMENT_INTERVAL_CHECKER_INITIAL_DELAY_IN_SECONDS,
+ DEPRECATED_REALTIME_SEGMENT_RELOCATION_INITIAL_DELAY_IN_SECONDS, SEGMENT_RELOCATOR_INITIAL_DELAY_IN_SECONDS);
+
+ private static final List<String> PERIOD_CONFIGS =
+ List.of(RETENTION_MANAGER_FREQUENCY_PERIOD, OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD,
+ REALTIME_SEGMENT_VALIDATION_FREQUENCY_PERIOD, REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_PERIOD,
+ BROKER_RESOURCE_VALIDATION_FREQUENCY_PERIOD, BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_PERIOD,
+ STATUS_CHECKER_FREQUENCY_PERIOD, STATUS_CHECKER_WAIT_FOR_PUSH_TIME_PERIOD, TASK_MANAGER_FREQUENCY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_PERIOD, MINION_INSTANCES_CLEANUP_TASK_INITIAL_DELAY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_PERIOD, TASK_METRICS_EMITTER_FREQUENCY_PERIOD,
+ SEGMENT_RELOCATOR_FREQUENCY_PERIOD, SEGMENT_LEVEL_VALIDATION_INTERVAL_PERIOD,
+ STATUS_CHECKER_INITIAL_DELAY_PERIOD, OFFLINE_SEGMENT_INTERVAL_CHECKER_INITIAL_DELAY_PERIOD,
+ SEGMENT_RELOCATOR_INITIAL_DELAY_PERIOD);
+
+ private static final int DURATION_IN_SECONDS = 20;
+ private static final String DURATION_IN_PERIOD_VALID = "2m";
+ private static final int PERIOD_DURATION_IN_SECONDS = 120;
+ private static final String DURATION_IN_PERIOD_INVALID = "2m2m";
+
+ /**
+ * When both type of configs are present, then the human-readable period config overrides the unit config
+ */
+ @Test
+ public void periodConfigOverridesUnitConfig() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ UNIT_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_SECONDS));
+ PERIOD_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_PERIOD_VALID));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, PERIOD_DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When only unit configs are supplied, then the correct converted value is returned.
+ */
+ @Test
+ public void suppliedUnitConfigsShouldReturnCorrectValues() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ UNIT_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_SECONDS));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When only period configs are supplied, then the correct converted value is returned.
+ */
+ @Test
+ public void suppliedPeriodConfigsShouldReturnCorrectValues() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ PERIOD_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_PERIOD_VALID));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, PERIOD_DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When valid unit config and invalid period config are specified, then an {@link IllegalArgumentException} is thrown (valid unit
+ * config does not override invalid period config)
+ */
+ @Test(expectedExceptions = IllegalArgumentException.class)
Review comment:
Expected exceptions are good, but I prefer fielding specific exceptions at the specific points where we expect them. That makes it easy to add more code to the test, or re-factor things, etc. and not worry about the test passing if we accidentally get IllegalArgumentException some place else.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1; // Disabled
private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS = 60 * 60; // 1 Hour.
+ private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS =
+ 60 * 60; // 1 Hour.
private static final int DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS = 60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS = "server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD = "server.request.timeoutPeriod";
Review comment:
This one should not be changed. We don't expect people to configure this value in minutes or hours.
--
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] [incubator-pinot] suddendust commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r672485362
##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java
##########
@@ -0,0 +1,158 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.controller.ControllerConf.ControllerPeriodicTasksConf.*;
+
+
+public class ControllerConfTest {
+
+ private static final List<String> UNIT_CONFIGS =
+ List.of(RETENTION_MANAGER_FREQUENCY_IN_SECONDS, OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS,
+ OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS, REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS,
+ REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_IN_SECONDS, BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS,
+ BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_IN_SECONDS, STATUS_CHECKER_FREQUENCY_IN_SECONDS,
+ STATUS_CHECKER_WAIT_FOR_PUSH_TIME_IN_SECONDS, TASK_MANAGER_FREQUENCY_IN_SECONDS,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS, MINION_INSTANCES_CLEANUP_TASK_INITIAL_DELAY_SECONDS,
+ MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_SECONDS,
+ TASK_METRICS_EMITTER_FREQUENCY_IN_SECONDS, SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS,
+ SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS, STATUS_CHECKER_INITIAL_DELAY_IN_SECONDS,
+ OFFLINE_SEGMENT_INTERVAL_CHECKER_INITIAL_DELAY_IN_SECONDS,
+ DEPRECATED_REALTIME_SEGMENT_RELOCATION_INITIAL_DELAY_IN_SECONDS, SEGMENT_RELOCATOR_INITIAL_DELAY_IN_SECONDS);
+
+ private static final List<String> PERIOD_CONFIGS =
+ List.of(RETENTION_MANAGER_FREQUENCY_PERIOD, OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD,
+ REALTIME_SEGMENT_VALIDATION_FREQUENCY_PERIOD, REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_PERIOD,
+ BROKER_RESOURCE_VALIDATION_FREQUENCY_PERIOD, BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_PERIOD,
+ STATUS_CHECKER_FREQUENCY_PERIOD, STATUS_CHECKER_WAIT_FOR_PUSH_TIME_PERIOD, TASK_MANAGER_FREQUENCY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_PERIOD, MINION_INSTANCES_CLEANUP_TASK_INITIAL_DELAY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_PERIOD, TASK_METRICS_EMITTER_FREQUENCY_PERIOD,
+ SEGMENT_RELOCATOR_FREQUENCY_PERIOD, SEGMENT_LEVEL_VALIDATION_INTERVAL_PERIOD,
+ STATUS_CHECKER_INITIAL_DELAY_PERIOD, OFFLINE_SEGMENT_INTERVAL_CHECKER_INITIAL_DELAY_PERIOD,
+ SEGMENT_RELOCATOR_INITIAL_DELAY_PERIOD);
+
+ private static final int DURATION_IN_SECONDS = 20;
+ private static final String DURATION_IN_PERIOD_VALID = "2m";
+ private static final int PERIOD_DURATION_IN_SECONDS = 120;
+ private static final String DURATION_IN_PERIOD_INVALID = "2m2m";
+
+ /**
+ * When both type of configs are present, then the human-readable period config overrides the unit config
+ */
+ @Test
+ public void periodConfigOverridesUnitConfig() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ UNIT_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_SECONDS));
+ PERIOD_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_PERIOD_VALID));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, PERIOD_DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When only unit configs are supplied, then the correct converted value is returned.
+ */
+ @Test
+ public void suppliedUnitConfigsShouldReturnCorrectValues() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ UNIT_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_SECONDS));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When only period configs are supplied, then the correct converted value is returned.
+ */
+ @Test
+ public void suppliedPeriodConfigsShouldReturnCorrectValues() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ PERIOD_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_PERIOD_VALID));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, PERIOD_DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When valid unit config and invalid period config are specified, then an {@link IllegalArgumentException} is thrown (valid unit
+ * config does not override invalid period config)
+ */
+ @Test(expectedExceptions = IllegalArgumentException.class)
Review comment:
That's a valid point @mcvsubbu. I just went with the simplest approach as the test was very trivial, and you pretty much can point where exactly the exception was thrown. Will change 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
[GitHub] [incubator-pinot] suddendust commented on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-882012741
I can't add labels for some reason. This PR needs the "release note" label.
--
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] [incubator-pinot] jackjlli commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r672465207
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -341,8 +406,9 @@ public String getDataDir() {
}
public int getSegmentCommitTimeoutSeconds() {
Review comment:
I feel that these method names also need to be changed, or have a new method here and mark the old one as deprecated.
The behavior should be that the new one should be used if both are specified or only the new one be specified. If not, use the old one.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -428,15 +496,20 @@ public void setRetentionControllerFrequencyInSeconds(int retentionFrequencyInSec
}
/**
- * Returns the config value for controller.offline.segment.interval.checker.frequencyInSeconds if it exists.
- * If it doesn't exist, returns the segment level validation interval. This is done in order to retain the current behavior,
- * wherein the offline validation tasks were done at segment level validation interval frequency
- * The default value is the new DEFAULT_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS
+ * Returns the config value for controller.offline.segment.interval.checker.frequencyInSeconds if
Review comment:
We should check the new one first and then the old one, right?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -341,8 +406,9 @@ public String getDataDir() {
}
public int getSegmentCommitTimeoutSeconds() {
Review comment:
Right, I wrote this one because I saw some of the java docs for the methods have been adjusted but it talks about the other way around. So mention it here.
--
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] [incubator-pinot] suddendust commented on a change in pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust commented on a change in pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#discussion_r672484276
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1; // Disabled
private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS = 60 * 60; // 1 Hour.
+ private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS =
+ 60 * 60; // 1 Hour.
private static final int DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS = 60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS = "server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD = "server.request.timeoutPeriod";
Review comment:
Yes makes sense. My thought process was that the configuration should move towards taking everything as human-readable strings and converting it to seconds (to keep it generic, basically if user wanted to keep the timeout as 2m, he should be able to).
##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/ControllerConfTest.java
##########
@@ -0,0 +1,158 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.controller.ControllerConf.ControllerPeriodicTasksConf.*;
+
+
+public class ControllerConfTest {
+
+ private static final List<String> UNIT_CONFIGS =
+ List.of(RETENTION_MANAGER_FREQUENCY_IN_SECONDS, OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS,
+ OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS, REALTIME_SEGMENT_VALIDATION_FREQUENCY_IN_SECONDS,
+ REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_IN_SECONDS, BROKER_RESOURCE_VALIDATION_FREQUENCY_IN_SECONDS,
+ BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_IN_SECONDS, STATUS_CHECKER_FREQUENCY_IN_SECONDS,
+ STATUS_CHECKER_WAIT_FOR_PUSH_TIME_IN_SECONDS, TASK_MANAGER_FREQUENCY_IN_SECONDS,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS, MINION_INSTANCES_CLEANUP_TASK_INITIAL_DELAY_SECONDS,
+ MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_SECONDS,
+ TASK_METRICS_EMITTER_FREQUENCY_IN_SECONDS, SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS,
+ SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS, STATUS_CHECKER_INITIAL_DELAY_IN_SECONDS,
+ OFFLINE_SEGMENT_INTERVAL_CHECKER_INITIAL_DELAY_IN_SECONDS,
+ DEPRECATED_REALTIME_SEGMENT_RELOCATION_INITIAL_DELAY_IN_SECONDS, SEGMENT_RELOCATOR_INITIAL_DELAY_IN_SECONDS);
+
+ private static final List<String> PERIOD_CONFIGS =
+ List.of(RETENTION_MANAGER_FREQUENCY_PERIOD, OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_PERIOD,
+ REALTIME_SEGMENT_VALIDATION_FREQUENCY_PERIOD, REALTIME_SEGMENT_VALIDATION_INITIAL_DELAY_PERIOD,
+ BROKER_RESOURCE_VALIDATION_FREQUENCY_PERIOD, BROKER_RESOURCE_VALIDATION_INITIAL_DELAY_PERIOD,
+ STATUS_CHECKER_FREQUENCY_PERIOD, STATUS_CHECKER_WAIT_FOR_PUSH_TIME_PERIOD, TASK_MANAGER_FREQUENCY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_PERIOD, MINION_INSTANCES_CLEANUP_TASK_INITIAL_DELAY_PERIOD,
+ MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_PERIOD, TASK_METRICS_EMITTER_FREQUENCY_PERIOD,
+ SEGMENT_RELOCATOR_FREQUENCY_PERIOD, SEGMENT_LEVEL_VALIDATION_INTERVAL_PERIOD,
+ STATUS_CHECKER_INITIAL_DELAY_PERIOD, OFFLINE_SEGMENT_INTERVAL_CHECKER_INITIAL_DELAY_PERIOD,
+ SEGMENT_RELOCATOR_INITIAL_DELAY_PERIOD);
+
+ private static final int DURATION_IN_SECONDS = 20;
+ private static final String DURATION_IN_PERIOD_VALID = "2m";
+ private static final int PERIOD_DURATION_IN_SECONDS = 120;
+ private static final String DURATION_IN_PERIOD_INVALID = "2m2m";
+
+ /**
+ * When both type of configs are present, then the human-readable period config overrides the unit config
+ */
+ @Test
+ public void periodConfigOverridesUnitConfig() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ UNIT_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_SECONDS));
+ PERIOD_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_PERIOD_VALID));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, PERIOD_DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When only unit configs are supplied, then the correct converted value is returned.
+ */
+ @Test
+ public void suppliedUnitConfigsShouldReturnCorrectValues() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ UNIT_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_SECONDS));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When only period configs are supplied, then the correct converted value is returned.
+ */
+ @Test
+ public void suppliedPeriodConfigsShouldReturnCorrectValues() {
+ //setup
+ Map<String, Object> controllerConfig = new HashMap<>();
+ PERIOD_CONFIGS.forEach(config -> controllerConfig.put(config, DURATION_IN_PERIOD_VALID));
+ ControllerConf conf = new ControllerConf(controllerConfig);
+ //execution and assertion
+ assertOnDurations(conf, PERIOD_DURATION_IN_SECONDS);
+ }
+
+ /**
+ * When valid unit config and invalid period config are specified, then an {@link IllegalArgumentException} is thrown (valid unit
+ * config does not override invalid period config)
+ */
+ @Test(expectedExceptions = IllegalArgumentException.class)
Review comment:
That's a valid point @mcvsubbu. I just went with the simplest approach as the test was very trivial, and you pretty much can point where exactly the exception was thrown. Will change it.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1; // Disabled
private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS = 60 * 60; // 1 Hour.
+ private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS =
+ 60 * 60; // 1 Hour.
private static final int DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS = 60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS = "server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD = "server.request.timeoutPeriod";
Review comment:
Yes makes sense. My thought process was that the configuration should move towards taking everything as human-readable strings and converting it to whatever units it wants to (to keep it generic, basically if user wanted to keep the timeout as 2m, he should be able to. The config takes care to changing it to seconds). Will address this.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -341,8 +406,9 @@ public String getDataDir() {
}
public int getSegmentCommitTimeoutSeconds() {
Review comment:
"The behavior should be that the new one should be used if both are specified or only the new one be specified" - Isn't it what it is doing @jackjlli? Basically if the new config is absent, it falls-back to the older one. But method still does what it says - `getSegmentCommitTimeoutSeconds`.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -428,15 +496,20 @@ public void setRetentionControllerFrequencyInSeconds(int retentionFrequencyInSec
}
/**
- * Returns the config value for controller.offline.segment.interval.checker.frequencyInSeconds if it exists.
- * If it doesn't exist, returns the segment level validation interval. This is done in order to retain the current behavior,
- * wherein the offline validation tasks were done at segment level validation interval frequency
- * The default value is the new DEFAULT_OFFLINE_SEGMENT_INTERVAL_CHECKER_FREQUENCY_IN_SECONDS
+ * Returns the config value for controller.offline.segment.interval.checker.frequencyInSeconds if
Review comment:
Missed this, will update the docs.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -148,15 +205,23 @@ private static long getRandomInitialDelayInSeconds() {
private static final int DEFAULT_STATUS_CONTROLLER_WAIT_FOR_PUSH_TIME_IN_SECONDS = 10 * 60; // 10 minutes
private static final int DEFAULT_TASK_MANAGER_FREQUENCY_IN_SECONDS = -1; // Disabled
private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_FREQUENCY_IN_SECONDS = 60 * 60; // 1 Hour.
- private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS = 60 * 60; // 1 Hour.
+ private static final int DEFAULT_MINION_INSTANCES_CLEANUP_TASK_MIN_OFFLINE_TIME_BEFORE_DELETION_IN_SECONDS =
+ 60 * 60; // 1 Hour.
private static final int DEFAULT_SEGMENT_LEVEL_VALIDATION_INTERVAL_IN_SECONDS = 24 * 60 * 60;
private static final int DEFAULT_SEGMENT_RELOCATOR_FREQUENCY_IN_SECONDS = 60 * 60;
}
+ @Deprecated
private static final String SERVER_ADMIN_REQUEST_TIMEOUT_SECONDS = "server.request.timeoutSeconds";
+ private static final String SERVER_ADMIN_REQUEST_TIMEOUT_PERIOD = "server.request.timeoutPeriod";
Review comment:
Yes makes sense. My thought process was that the configuration should move towards taking everything as human-readable strings and converting it to whatever units it wants to (to keep it generic, basically if user wanted to keep the timeout as 2m, he should be able to. The config takes care to changing of to seconds). Will address this.
--
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] [incubator-pinot] suddendust edited a comment on pull request #7173: [7156] Human Readable Controller Configs
Posted by GitBox <gi...@apache.org>.
suddendust edited a comment on pull request #7173:
URL: https://github.com/apache/incubator-pinot/pull/7173#issuecomment-885389139
> Other than one minor request of adding a comment, we are good to merge this.
Removed the deprecated config `DEPRECATED_VALIDATION_MANAGER_FREQUENCY_IN_SECONDS` instead.
--
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