You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/06/30 10:36:19 UTC
[GitHub] [pinot] gortiz opened a new pull request, #8999: Retry validateJobs for a while to avoid flaky errors
gortiz opened a new pull request, #8999:
URL: https://github.com/apache/pinot/pull/8999
This PR tries to fix #8776.
What the test does is to modify the state in helix and then verify that the result is correct. IICU Helix, there is no read-after-write guarantee in Helix, so there is a race condition between the update and the read. In normal cases, Helix will change the state faster and therefore the test doesn't fail. But that is not guaranteed and in some executions it may happen that the change is not applied when the validate is done.
Sequence diagram when everything works as expected:
```mermaid
sequenceDiagram
Test->>+Controller: update table config (0 */20 * ? * * *)
Controller->>+Helix: update table config (0 */20 * ? * * *)
Helix->>Helix: Change ideal state to (0 */20 * ? * * *)
Helix->>-Controller: Ok
Controller->>-Test: Ok
Test->>+Controller: get job info
Controller->>+Helix: get job info
Helix->>-Controller: updated job info (0 */20 * ? * * *)
Controller->>-Test: updated job info (0 */20 * ? * * *)
```
Sequence diagram when the change is not seen:
```mermaid
sequenceDiagram
Test->>+Controller: update table config (0 */20 * ? * * *)
Controller->>+Helix: update table config (0 */20 * ? * * *)
Helix->>Controller: Ok
Controller->>-Test: Ok
Test->>+Controller: get job info
Controller->>+Helix: get job info
Helix->>-Controller: updated job info (0 */10 * ? * * *)
Helix->>-Helix: Change ideal state (0 */20 * ? * * *)
Controller->>-Test: updated job info (0 */10 * ? * * *)
```
The only solution know is to retry the validation with some timeout.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #8999: Retry validateJobs for a while to avoid flaky errors
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8999:
URL: https://github.com/apache/pinot/pull/8999#issuecomment-1171093885
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8999?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 [#8999](https://codecov.io/gh/apache/pinot/pull/8999?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (187c6a5) into [master](https://codecov.io/gh/apache/pinot/commit/d2031e9479f8b56326ee025e9ac5f549d9665327?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d2031e9) will **decrease** coverage by `53.50%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #8999 +/- ##
=============================================
- Coverage 68.50% 15.00% -53.51%
+ Complexity 4894 170 -4724
=============================================
Files 1820 1773 -47
Lines 95396 93334 -2062
Branches 14276 14044 -232
=============================================
- Hits 65353 14002 -51351
- Misses 25458 78321 +52863
+ Partials 4585 1011 -3574
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| unittests1 | `?` | |
| unittests2 | `15.00% <ø> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8999?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/8999/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/8999/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/StringUtil.java](https://codecov.io/gh/apache/pinot/pull/8999/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvU3RyaW5nVXRpbC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8999/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/8999/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/8999/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/8999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/8999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/user/RoleType.java](https://codecov.io/gh/apache/pinot/pull/8999/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3VzZXIvUm9sZVR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1347 more](https://codecov.io/gh/apache/pinot/pull/8999/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/pinot/pull/8999?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/pinot/pull/8999?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 [d2031e9...187c6a5](https://codecov.io/gh/apache/pinot/pull/8999?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] [pinot] gortiz commented on a diff in pull request #8999: Retry validateJobs for a while to avoid flaky errors
Posted by GitBox <gi...@apache.org>.
gortiz commented on code in PR #8999:
URL: https://github.com/apache/pinot/pull/8999#discussion_r913064723
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManagerStatelessTest.java:
##########
@@ -239,10 +242,20 @@ private void validateJob(String taskType, String cronExpression)
throw new RuntimeException(e);
}
}, TIMEOUT_IN_MS, "JobDetail exiting but missing JobTrigger");
- List<? extends Trigger> triggersOfJob = scheduler.getTriggersOfJob(jobKey);
- Trigger trigger = triggersOfJob.iterator().next();
- assertTrue(trigger instanceof CronTrigger);
- assertEquals(((CronTrigger) trigger).getCronExpression(), cronExpression);
+
+ // There is no guarantee that previous changes have been applied, therefore we need to
+ // retry the check for a bit
+ TestUtils.waitForResult(() -> {
Review Comment:
Changed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli merged pull request #8999: Retry validateJobs for a while to avoid flaky errors
Posted by GitBox <gi...@apache.org>.
jackjlli merged PR #8999:
URL: https://github.com/apache/pinot/pull/8999
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a diff in pull request #8999: Retry validateJobs for a while to avoid flaky errors
Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #8999:
URL: https://github.com/apache/pinot/pull/8999#discussion_r911318797
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManagerStatelessTest.java:
##########
@@ -239,10 +242,20 @@ private void validateJob(String taskType, String cronExpression)
throw new RuntimeException(e);
}
}, TIMEOUT_IN_MS, "JobDetail exiting but missing JobTrigger");
- List<? extends Trigger> triggersOfJob = scheduler.getTriggersOfJob(jobKey);
- Trigger trigger = triggersOfJob.iterator().next();
- assertTrue(trigger instanceof CronTrigger);
- assertEquals(((CronTrigger) trigger).getCronExpression(), cronExpression);
+
+ // There is no guarantee that previous changes have been applied, therefore we need to
+ // retry the check for a bit
+ TestUtils.waitForResult(() -> {
Review Comment:
You can use `waitForCondition` instead of `waitForResult`, as the logic here doesn't require to return anything.
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManagerStatelessTest.java:
##########
@@ -239,10 +242,20 @@ private void validateJob(String taskType, String cronExpression)
throw new RuntimeException(e);
}
}, TIMEOUT_IN_MS, "JobDetail exiting but missing JobTrigger");
- List<? extends Trigger> triggersOfJob = scheduler.getTriggersOfJob(jobKey);
- Trigger trigger = triggersOfJob.iterator().next();
- assertTrue(trigger instanceof CronTrigger);
- assertEquals(((CronTrigger) trigger).getCronExpression(), cronExpression);
+
+ // There is no guarantee that previous changes have been applied, therefore we need to
+ // retry the check for a bit
+ TestUtils.waitForResult(() -> {
+ try {
+ List<? extends Trigger> triggersOfJob = scheduler.getTriggersOfJob(jobKey);
+ Trigger trigger = triggersOfJob.iterator().next();
+ assertTrue(trigger instanceof CronTrigger);
Review Comment:
If `waitForCondition` is used, you may need to replace this assertion statement with returning true or false.
--
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