Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Chore] Improve the robustness of CI #16592

Merged
merged 18 commits into from
Sep 6, 2024
Merged

Conversation

SbloodyS
Copy link
Member

@SbloodyS SbloodyS commented Sep 5, 2024

Purpose of the pull request

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

Pull Request Notice

Pull Request Notice

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

@SbloodyS SbloodyS self-assigned this Sep 5, 2024
@SbloodyS SbloodyS added this to the 3.3.0 milestone Sep 5, 2024
if [ ! -f /tmp/${{ matrix.version }}/apache-dolphinscheduler-${{ matrix.version }}-bin.tar.gz ]; then
echo "Binary package not found in cache, downloading..."
mkdir -p /tmp/${{ matrix.version }}
wget https://mirrors.tuna.tsinghua.edu.cn/apache/dolphinscheduler/${{ matrix.version }}/apache-dolphinscheduler-${{ matrix.version }}-bin.tar.gz -P /tmp/${{ matrix.version }}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since https://archive.apache.org download speed is too slow and often causes timeout failures. So using https://mirrors.tuna.tsinghua.edu.cn to do the cache instead.

Notice: This site does not contains the archived versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure use tsinghua mirrors is good idea or not

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if accept can we add some desc in L144?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure use tsinghua mirrors is good idea or not

After I tested a dozen mirrors, I found this source to be the fastest in github action.

if accept can we add some desc in L144?

It seems comments don't seem to be supported in run. I tried, but I got an error...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about using apache cdn, such as https://dlcdn.apache.org/dolphinscheduler/3.2.2/apache-dolphinscheduler-3.2.2-bin.tar.gz for version 3.2.2?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems comments don't seem to be supported in run. I tried, but I got an error...

I mean we and comment before the line for run property.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about using apache cdn, such as https://dlcdn.apache.org/dolphinscheduler/3.2.2/apache-dolphinscheduler-3.2.2-bin.tar.gz for version 3.2.2?

And how about apache official cdn?

Copy link
Member Author

@SbloodyS SbloodyS Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about using apache cdn, such as https://dlcdn.apache.org/dolphinscheduler/3.2.2/apache-dolphinscheduler-3.2.2-bin.tar.gz for version 3.2.2?

This is the same speed as archive.apache.org...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all right, so maybe we can keep tsinghua mirror here

@SbloodyS SbloodyS added the chore label Sep 5, 2024
@SbloodyS SbloodyS marked this pull request as ready for review September 5, 2024 14:03
Copy link

sonarcloud bot commented Sep 6, 2024

Copy link
Contributor

@rickchengx rickchengx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -151,7 +151,7 @@ jobs:
uses: actions/cache@v4
with:
path: ~/.m2/repository
key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}-e2e
key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}-backend
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e2e has different pom.xml with the backend module, as you can see it uses dolphinscheduler-e2e/pom.xml instead of /pom.xml

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter. The maven cache path is ~/.m2/repository. The old dependency cache is retained and new dependencies are added.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have half a memory we have some error with the same cache key #14808 but I am not sure, maybe we can keep this change to see whether regression in the further

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarification.

@@ -62,7 +62,7 @@ jobs:
uses: actions/cache@v4
with:
path: ~/.m2/repository
key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}-api-test
key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}-backend
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change the cache key?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The total cache storage limit is 10GB. We add binary package to it will cause overuse. So I reuse it. Please refer to the doc in
https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/caching-dependencies-to-speed-up-workflows#usage-limits-and-eviction-policy

@Gallardot Gallardot merged commit 1b629bb into apache:dev Sep 6, 2024
70 checks passed
@SbloodyS SbloodyS deleted the improve_ci branch September 6, 2024 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants