You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "devmadhuu (via GitHub)" <gi...@apache.org> on 2023/09/11 09:03:58 UTC
[GitHub] [ozone] devmadhuu opened a new pull request, #5267: HDDS-6618. [perf] Require successful basic checks for long-running tests
devmadhuu opened a new pull request, #5267:
URL: https://github.com/apache/ozone/pull/5267
## What changes were proposed in this pull request?
Since basic checks are fairly quick, so we can postpone starting acceptance, kubernetes and integration checks until these finish. So this PR addressees following 2 things:
- Extract basic(unit) task into a separate job.
- And doesn't start acceptance, kubernetes and integration checks until `basic-test(unit)` is completed successfully.
Later this `basic-test(unit) ` may be changed to run as part of integration test as basic unit tests also takes significant amount of time to finish.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6618
## How was this patch tested?
This patch was tested by running [CI jobs](https://github.com/devmadhuu/ozone/actions/runs/6143269780) with updated ci.yml file.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5267: HDDS-6618. [perf] Require successful basic checks for long-running tests
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5267:
URL: https://github.com/apache/ozone/pull/5267#discussion_r1329584614
##########
dev-support/ci/selective_basic_ci_checks.sh:
##########
Review Comment:
Thanks @adoroszlai for re-review. I have incorporated your suggestions. Pls check.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a diff in pull request #5267: HDDS-6618. [perf] Require successful basic checks for long-running tests
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5267:
URL: https://github.com/apache/ozone/pull/5267#discussion_r1321409068
##########
.github/workflows/ci.yml:
##########
@@ -191,11 +193,7 @@ jobs:
with:
distribution: 'temurin'
java-version: 8
- - name: Execute tests
- run: hadoop-ozone/dev-support/checks/${{ matrix.check }}.sh
- - name: Summary of failures
- run: cat target/${{ matrix.check }}/summary.txt
- if: ${{ !cancelled() }}
Review Comment:
I think you had already fixed this problem by the changes done in `selective_ci_checks.sh`. `unit` is no longer emitted as part of the list of basic checks.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5267: HDDS-6618. [perf] Require successful basic checks for long-running tests
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5267:
URL: https://github.com/apache/ozone/pull/5267#discussion_r1329584514
##########
dev-support/ci/selective_ci_checks.sh:
##########
@@ -82,7 +82,9 @@ function get_changed_files() {
}
function set_outputs_run_everything_and_exit() {
- BASIC_CHECKS="author bats checkstyle docs findbugs native rat unit"
+ BASIC_CHECKS=$(grep -r '^#checks:' hadoop-ozone/dev-support/checks \
+ | sort -u | cut -f1 -d':' | rev | cut -f1 -d'/' | rev \
+ | cut -f1 -d'.')
Review Comment:
Thanks @adoroszlai for re-review. I have incorporated your suggestions. Pls check.
##########
.github/workflows/ci.yml:
##########
@@ -62,6 +64,31 @@ jobs:
- name: Acceptance suites
id: acceptance-suites
run: dev-support/ci/acceptance_suites.sh
+ pre-checks:
+ needs:
+ - build-info
+ runs-on: ubuntu-20.04
+ timeout-minutes: 45
+ outputs:
+ basic-checks: ${{ steps.selective-basic-checks.outputs.basic-checks }}
+ basic-unit: ${{ steps.selective-basic-checks.outputs.basic-unit }}
+ steps:
+ - name: "Checkout ${{ github.ref }} ( ${{ github.sha }} )"
+ uses: actions/checkout@v3
+ with:
+ persist-credentials: false
+ - name: Fetch incoming commit ${{ github.sha }} with its parent
+ uses: actions/checkout@v3
+ with:
+ ref: ${{ github.sha }}
+ fetch-depth: 2
+ persist-credentials: false
+ if: github.event_name == 'pull_request'
+ - name: Selective Basic Checks
+ id: selective-basic-checks
+ env:
+ ALL_BASIC_CHECKS: "${{ needs.build-info.outputs.all-basic-checks }}"
+ run: dev-support/ci/selective_basic_ci_checks.sh
Review Comment:
Thanks @adoroszlai for re-review. I have incorporated your suggestions. Pls check.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a diff in pull request #5267: HDDS-6618. [perf] Require successful basic checks for long-running tests
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5267:
URL: https://github.com/apache/ozone/pull/5267#discussion_r1321252209
##########
.github/workflows/ci.yml:
##########
@@ -242,6 +273,7 @@ jobs:
needs:
- build-info
- build
+ - basic-test
Review Comment:
```suggestion
- basic
```
##########
.github/workflows/ci.yml:
##########
@@ -191,11 +193,7 @@ jobs:
with:
distribution: 'temurin'
java-version: 8
- - name: Execute tests
- run: hadoop-ozone/dev-support/checks/${{ matrix.check }}.sh
- - name: Summary of failures
- run: cat target/${{ matrix.check }}/summary.txt
- if: ${{ !cancelled() }}
Review Comment:
This part should not be removed, otherwise these basic checks become no-ops.
##########
.github/workflows/ci.yml:
##########
@@ -291,6 +323,7 @@ jobs:
needs:
- build-info
- build
+ - basic-test
Review Comment:
```suggestion
- basic
```
##########
.github/workflows/ci.yml:
##########
@@ -321,6 +354,8 @@ jobs:
integration:
needs:
- build-info
+ - build
+ - basic-test
Review Comment:
```suggestion
- basic
```
##########
.github/workflows/ci.yml:
##########
@@ -209,6 +207,39 @@ jobs:
rm -rf ~/.m2/repository/org/apache/ozone/hdds*
rm -rf ~/.m2/repository/org/apache/ozone/ozone*
if: always()
+ basic-test:
Review Comment:
```suggestion
unit:
```
We also need to change `coverage` to depend on `unit` instead of `basic` (which no longer generates test coverage data).
##########
.github/workflows/ci.yml:
##########
@@ -209,6 +207,39 @@ jobs:
rm -rf ~/.m2/repository/org/apache/ozone/hdds*
rm -rf ~/.m2/repository/org/apache/ozone/ozone*
if: always()
+ basic-test:
+ needs:
+ - build-info
+ - build
Review Comment:
```suggestion
- basic
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai merged pull request #5267: HDDS-6618. Require successful basic checks for long-running tests
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #5267:
URL: https://github.com/apache/ozone/pull/5267
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5267: HDDS-6618. [perf] Require successful basic checks for long-running tests
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5267:
URL: https://github.com/apache/ozone/pull/5267#discussion_r1321360019
##########
.github/workflows/ci.yml:
##########
@@ -191,11 +193,7 @@ jobs:
with:
distribution: 'temurin'
java-version: 8
- - name: Execute tests
- run: hadoop-ozone/dev-support/checks/${{ matrix.check }}.sh
- - name: Summary of failures
- run: cat target/${{ matrix.check }}/summary.txt
- if: ${{ !cancelled() }}
Review Comment:
@adoroszlai thanks for review, I have a question here.. If we'll not remove step -> `Execute tests`, then unit.sh (unit tests) will run as part of basic check and we want to separate out unit test run out of basic checks...? Did I understood anything wrong 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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5267: HDDS-6618. [perf] Require successful basic checks for long-running tests
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5267:
URL: https://github.com/apache/ozone/pull/5267#discussion_r1321385160
##########
.github/workflows/ci.yml:
##########
@@ -242,6 +273,7 @@ jobs:
needs:
- build-info
- build
+ - basic-test
Review Comment:
Okay, got it.
##########
.github/workflows/ci.yml:
##########
@@ -291,6 +323,7 @@ jobs:
needs:
- build-info
- build
+ - basic-test
Review Comment:
Okay, 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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5267: HDDS-6618. [perf] Require successful basic checks for long-running tests
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5267:
URL: https://github.com/apache/ozone/pull/5267#discussion_r1321417487
##########
.github/workflows/ci.yml:
##########
@@ -191,11 +193,7 @@ jobs:
with:
distribution: 'temurin'
java-version: 8
- - name: Execute tests
- run: hadoop-ozone/dev-support/checks/${{ matrix.check }}.sh
- - name: Summary of failures
- run: cat target/${{ matrix.check }}/summary.txt
- if: ${{ !cancelled() }}
Review Comment:
> I think you had already fixed this problem by the changes done in `selective_ci_checks.sh`. `unit` is no longer emitted as part of the list of basic checks.
Yes, so we should not include below lines in basic checks...right ?
```
- name: Execute tests
run: hadoop-ozone/dev-support/checks/${{ matrix.check }}.sh
- name: Summary of failures
run: cat target/${{ matrix.check }}/summary.txt
if: ${{ !cancelled() }}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5267: HDDS-6618. [perf] Require successful basic checks for long-running tests
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5267:
URL: https://github.com/apache/ozone/pull/5267#discussion_r1321385498
##########
.github/workflows/ci.yml:
##########
@@ -321,6 +354,8 @@ jobs:
integration:
needs:
- build-info
+ - build
+ - basic-test
Review Comment:
Okay, got it.
##########
.github/workflows/ci.yml:
##########
@@ -209,6 +207,39 @@ jobs:
rm -rf ~/.m2/repository/org/apache/ozone/hdds*
rm -rf ~/.m2/repository/org/apache/ozone/ozone*
if: always()
+ basic-test:
+ needs:
+ - build-info
+ - build
Review Comment:
Okay, done.
##########
.github/workflows/ci.yml:
##########
@@ -209,6 +207,39 @@ jobs:
rm -rf ~/.m2/repository/org/apache/ozone/hdds*
rm -rf ~/.m2/repository/org/apache/ozone/ozone*
if: always()
+ basic-test:
Review Comment:
> We also need to change `coverage` to depend on `unit` instead of `basic` (which no longer generates test coverage data).
Ok. 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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5267: HDDS-6618. [perf] Require successful basic checks for long-running tests
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5267:
URL: https://github.com/apache/ozone/pull/5267#discussion_r1321462438
##########
.github/workflows/ci.yml:
##########
@@ -191,11 +193,7 @@ jobs:
with:
distribution: 'temurin'
java-version: 8
- - name: Execute tests
- run: hadoop-ozone/dev-support/checks/${{ matrix.check }}.sh
- - name: Summary of failures
- run: cat target/${{ matrix.check }}/summary.txt
- if: ${{ !cancelled() }}
Review Comment:
Yes you are right. I got it what you meant. Thanks for correcting me.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a diff in pull request #5267: HDDS-6618. [perf] Require successful basic checks for long-running tests
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5267:
URL: https://github.com/apache/ozone/pull/5267#discussion_r1321458889
##########
.github/workflows/ci.yml:
##########
@@ -191,11 +193,7 @@ jobs:
with:
distribution: 'temurin'
java-version: 8
- - name: Execute tests
- run: hadoop-ozone/dev-support/checks/${{ matrix.check }}.sh
- - name: Summary of failures
- run: cat target/${{ matrix.check }}/summary.txt
- if: ${{ !cancelled() }}
Review Comment:
No, we should include these. They are required for all checks (`checkstyle`, `bats`, etc.), not only `unit`. So the current state of the patch is fine.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a diff in pull request #5267: HDDS-6618. [perf] Require successful basic checks for long-running tests
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5267:
URL: https://github.com/apache/ozone/pull/5267#discussion_r1328131318
##########
dev-support/ci/selective_basic_ci_checks.sh:
##########
Review Comment:
The other script is named `selective_ci_checks.sh` because it decides which checks to run. This one does not filter the checks, rather categorizes them. So I think a better name for this script and the corresponding CI step would be something like `categorize_basic_checks.sh` and `categorize-basic-checks`, respectively.
##########
.github/workflows/ci.yml:
##########
@@ -62,6 +64,31 @@ jobs:
- name: Acceptance suites
id: acceptance-suites
run: dev-support/ci/acceptance_suites.sh
+ pre-checks:
+ needs:
+ - build-info
+ runs-on: ubuntu-20.04
+ timeout-minutes: 45
+ outputs:
+ basic-checks: ${{ steps.selective-basic-checks.outputs.basic-checks }}
+ basic-unit: ${{ steps.selective-basic-checks.outputs.basic-unit }}
+ steps:
+ - name: "Checkout ${{ github.ref }} ( ${{ github.sha }} )"
+ uses: actions/checkout@v3
+ with:
+ persist-credentials: false
+ - name: Fetch incoming commit ${{ github.sha }} with its parent
+ uses: actions/checkout@v3
+ with:
+ ref: ${{ github.sha }}
+ fetch-depth: 2
+ persist-credentials: false
+ if: github.event_name == 'pull_request'
+ - name: Selective Basic Checks
+ id: selective-basic-checks
+ env:
+ ALL_BASIC_CHECKS: "${{ needs.build-info.outputs.all-basic-checks }}"
+ run: dev-support/ci/selective_basic_ci_checks.sh
Review Comment:
This step (and its two outputs) can be added as the last step in `build-info`, no need for a separate job. The step's input needs to be tweaked a bit to `${{ steps.selective-checks.outputs.basic-checks }}`.
##########
dev-support/ci/selective_ci_checks.sh:
##########
@@ -82,7 +82,9 @@ function get_changed_files() {
}
function set_outputs_run_everything_and_exit() {
- BASIC_CHECKS="author bats checkstyle docs findbugs native rat unit"
+ BASIC_CHECKS=$(grep -r '^#checks:' hadoop-ozone/dev-support/checks \
+ | sort -u | cut -f1 -d':' | rev | cut -f1 -d'/' | rev \
+ | cut -f1 -d'.')
Review Comment:
Can be simplified a bit:
* `grep -l` only prints paths with match(es), so we can omit the first `cut`
* `basename` extracts the filename out of a path
* `xargs` helps apply a command to each item from stdin
```suggestion
BASIC_CHECKS=$(grep -lr '^#checks:' hadoop-ozone/dev-support/checks \
| sort -u | xargs -n1 basename \
| cut -f1 -d'.')
```
The same applies to similar commands in `selective_basic_ci_checks.sh`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org