Akshat-Jain commented on code in PR #17876:
URL: https://github.com/apache/druid/pull/17876#discussion_r2028965244
##########
.github/workflows/ci.yml:
##########
@@ -17,28 +17,23 @@ name: "CI"
on:
workflow_call:
- inputs:
- jdk:
- required: false
- type: string
- default: '17'
jobs:
run-unit-tests:
- name: "unit tests"
+ name: "unit tests(main)"
strategy:
fail-fast: false
matrix:
- Dtest: [ "A*,F*,S*", "B*,D*,L*,T*", "C*,O*", "E*,N*,Q*", "G*,R*,U*",
"H*,I*,J*", "K*,P*,V*,W*,X*,Y*,Z*", "M*"]
+ pattern: [ "L*,Q*,W*", "C*,E*", "B*,O*,S*,X*,Y*,Z*", "A*,G*,R*",
"M*,P*,V*", "D*,J*,K*", "F*,H*,U*", "I*,N*,T*"]
Review Comment:
Nit: Can sort the groups alphabetically, since the order here is how they
show up on the UI as well, so just to make it slightly easier for viewers.
```suggestion
pattern: [ "A*,G*,R*", "B*,O*,S*,X*,Y*,Z*", "C*,E*", "D*,J*,K*",
"F*,H*,U*", "I*,N*,T*", "L*,Q*,W*", "M*,P*,V*"]
```
##########
.github/workflows/ci.yml:
##########
@@ -61,11 +56,26 @@ jobs:
truncate_stack_traces: false
reporting-jacoco-coverage-failures:
- name: "report-jacoco-coverage-failures"
+ name: "coverage-jacoco"
needs: run-unit-tests
uses: ./.github/workflows/worker.yml
if: ${{ !contains( github.event.pull_request.labels.*.name, 'jacoco:skip')
}}
with:
script: .github/scripts/create-jacoco-coverage-report.sh
artifacts_to_download: "unit-test-reports-*"
- key: "jacoco-coverage-report"
+ key: "coverage-jacoco"
+
+ run-unit-tests-all-jdk:
+ needs: test-report
+ name: "unit tests(all jdk)"
+ if: ${{ !cancelled() && ( contains(
github.event.pull_request.labels.*.name, 'tests:all-jdk') || github.event_name
== 'push' ) }}
+ strategy:
+ fail-fast: false
+ matrix:
+ pattern: [ "L*,Q*,W*", "C*,E*", "B*,O*,S*,X*,Y*,Z*", "A*,G*,R*",
"M*,P*,V*", "D*,J*,K*", "F*,H*,U*", "I*,N*,T*"]
Review Comment:
Similar suggestion here about alphabetical ordering of the groups.
##########
dev/test_balancing_calc:
##########
@@ -0,0 +1,35 @@
+#!/bin/bash
+
+# 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.
+#--------------------------------------------------------------------
+
+# Helps to calculate new balancing
Review Comment:
Nit: Since this is outside of the unit test layer, it would be nice to
elaborate a bit on what we mean by balancing here.
##########
.github/workflows/ci.yml:
##########
@@ -61,11 +56,26 @@ jobs:
truncate_stack_traces: false
reporting-jacoco-coverage-failures:
- name: "report-jacoco-coverage-failures"
+ name: "coverage-jacoco"
needs: run-unit-tests
uses: ./.github/workflows/worker.yml
if: ${{ !contains( github.event.pull_request.labels.*.name, 'jacoco:skip')
}}
with:
script: .github/scripts/create-jacoco-coverage-report.sh
artifacts_to_download: "unit-test-reports-*"
- key: "jacoco-coverage-report"
+ key: "coverage-jacoco"
+
+ run-unit-tests-all-jdk:
Review Comment:
1. Can you share a sample GitHub Run URL where this is run? Maybe could add
it to the PR description itself.
2. If I have a PR, and label it after 4 hours, would it trigger the `all
jdk` test suite? Or is the expectation that the label needs to be applied when
creating the PR?
##########
.github/workflows/ci.yml:
##########
@@ -61,11 +56,26 @@ jobs:
truncate_stack_traces: false
reporting-jacoco-coverage-failures:
- name: "report-jacoco-coverage-failures"
+ name: "coverage-jacoco"
needs: run-unit-tests
uses: ./.github/workflows/worker.yml
if: ${{ !contains( github.event.pull_request.labels.*.name, 'jacoco:skip')
}}
with:
script: .github/scripts/create-jacoco-coverage-report.sh
artifacts_to_download: "unit-test-reports-*"
- key: "jacoco-coverage-report"
+ key: "coverage-jacoco"
+
+ run-unit-tests-all-jdk:
+ needs: test-report
+ name: "unit tests(all jdk)"
+ if: ${{ !cancelled() && ( contains(
github.event.pull_request.labels.*.name, 'tests:all-jdk') || github.event_name
== 'push' ) }}
+ strategy:
+ fail-fast: false
+ matrix:
+ pattern: [ "L*,Q*,W*", "C*,E*", "B*,O*,S*,X*,Y*,Z*", "A*,G*,R*",
"M*,P*,V*", "D*,J*,K*", "F*,H*,U*", "I*,N*,T*"]
+ jdk: [ "11", "21.0.4" ]
+ uses: ./.github/workflows/worker.yml
+ with:
+ script: .github/scripts/run-unit-tests.sh -Dtest='${{ matrix.pattern }}'
-fae
Review Comment:
`-fae` seems to have been added by mistake?
```suggestion
script: .github/scripts/run-unit-tests.sh -Dtest='${{ matrix.pattern
}}'
```
##########
dev/test_balancing_calc:
##########
@@ -0,0 +1,35 @@
+#!/bin/bash
+
+# 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.
+#--------------------------------------------------------------------
+
+# Helps to calculate new balancing
+# on the output a simple knapsack should be done...right now that's manual
+
+set -e
+
+find . -name 'TEST*xml' |
+# <testsuite ... name="org.apache.druid.query.policy.NoRestrictionPolicyTest"
time="0.071" ...>
+xargs sed -nr
'/^<testsuite/s/.*name=\"([^\"]+)\".*time=\"([^\"]+)\".*/\1\t\2/p' |
+# org.apache.druid.server.RequestLogLineTest 0.052
+sed -r 's/[^\t]+\.(.)[^\t.]+\t/\1\t/' > test_times
+
+for c in {A..Z} ;do
+ echo -n "$c ";
+ (echo 0;grep "^$c" test_times| cut -f 2)|paste -s -d + -|bc
+done | sort -k 2 -nr
+
Review Comment:
Can you add a sample output as a comment here? I feel it would help the
viewers to better understand the purpose of this 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
