You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "martin-g (via GitHub)" <gi...@apache.org> on 2023/10/03 12:30:33 UTC
[PR] AVRO-3870: [Rust] Use matrix.target, matrix.avro and hash of Cargo.lock for cache keys [avro]
martin-g opened a new pull request, #2538:
URL: https://github.com/apache/avro/pull/2538
AVRO-3870
## What is the purpose of the change
* Improve the cache keys for the Rust CI jobs
## Verifying this change
The build and tests should still work
## Documentation
- Does this pull request introduce a new feature? no
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3870: [Rust] Use `git` protocol to fetch dependencies until MSRV is 1.70.0 [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g closed pull request #2538: AVRO-3870: [Rust] Use `git` protocol to fetch dependencies until MSRV is 1.70.0
URL: https://github.com/apache/avro/pull/2538
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3870: [Rust] Use matrix.target, matrix.avro and hash of Cargo.lock for cache keys [avro]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on code in PR #2538:
URL: https://github.com/apache/avro/pull/2538#discussion_r1344376019
##########
.github/workflows/test-lang-rust-ci.yml:
##########
@@ -59,19 +60,27 @@ jobs:
uses: actions/checkout@v4
- name: Cache Cargo
+ id: cache-cargo
uses: actions/cache@v3
with:
# these represent dependencies downloaded by cargo
# and thus do not depend on the OS, arch nor rust version.
path: ~/.cargo
- key: cargo-cache1-
+ key: ${{ runner.os }}-cargo-cache1-${{ matrix.target }}-${{ matrix.rust }}-${{ hashFiles('**/Cargo.lock') }}
+
Review Comment:
I didn't use `${{ matrix.target }}` and `${{ matrix.rust }}` intentionally in #2517 because many caches are generated and take up the cache capacity.
See https://github.com/apache/avro/actions/caches
Rust related cache takes up approximately 9GB while the capacity limit is 10GB.
Do you think it's cost effective for the degree of the improvement?
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3870: [Rust] Use `git` protocol to fetch dependencies until MSRV is 1.70.0 [avro]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on code in PR #2538:
URL: https://github.com/apache/avro/pull/2538#discussion_r1346164698
##########
.github/workflows/test-lang-rust-ci.yml:
##########
@@ -59,19 +60,22 @@ jobs:
uses: actions/checkout@v4
- name: Cache Cargo
+ id: cache-cargo
uses: actions/cache@v3
with:
# these represent dependencies downloaded by cargo
# and thus do not depend on the OS, arch nor rust version.
path: ~/.cargo
- key: cargo-cache1-
+ key: ${{ runner.os }}-cargo-cache2-
+
- name: Cache Rust dependencies
+ id: cache-target
uses: actions/cache@v3
with:
# these represent compiled steps of both dependencies and avro
# and thus are specific for a particular OS, arch and rust version.
- path: ~/target
- key: ${{ runner.os }}-target-cache1-${{ matrix.rust }}-
+ path: lang/rust/target
+ key: ${{ runner.os }}-target-cache2-
Review Comment:
Also, why `${{ hasFiles(...) }}` is not used?
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3870: [Rust] Use matrix.target, matrix.avro and hash of Cargo.lock for cache keys [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #2538:
URL: https://github.com/apache/avro/pull/2538#discussion_r1344447288
##########
.github/workflows/test-lang-rust-ci.yml:
##########
@@ -59,19 +60,27 @@ jobs:
uses: actions/checkout@v4
- name: Cache Cargo
+ id: cache-cargo
uses: actions/cache@v3
with:
# these represent dependencies downloaded by cargo
# and thus do not depend on the OS, arch nor rust version.
path: ~/.cargo
- key: cargo-cache1-
+ key: ${{ runner.os }}-cargo-cache1-${{ matrix.target }}-${{ matrix.rust }}-${{ hashFiles('**/Cargo.lock') }}
+
Review Comment:
Good point!
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3870: [Rust] Use matrix.target, matrix.avro and hash of Cargo.lock for cache keys [avro]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on code in PR #2538:
URL: https://github.com/apache/avro/pull/2538#discussion_r1344376019
##########
.github/workflows/test-lang-rust-ci.yml:
##########
@@ -59,19 +60,27 @@ jobs:
uses: actions/checkout@v4
- name: Cache Cargo
+ id: cache-cargo
uses: actions/cache@v3
with:
# these represent dependencies downloaded by cargo
# and thus do not depend on the OS, arch nor rust version.
path: ~/.cargo
- key: cargo-cache1-
+ key: ${{ runner.os }}-cargo-cache1-${{ matrix.target }}-${{ matrix.rust }}-${{ hashFiles('**/Cargo.lock') }}
+
Review Comment:
I didn't use `${{ matrix.target }}` and `${{ matrix.rust }}` intentionally because many caches are generated and take up the cache capacity.
See https://github.com/apache/avro/actions/caches
Rust related cache takes up approximately 9GB while the capacity limit is 10GB.
Do you think it's cost effective for the degree of the improvement?
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3870: [Rust] Use matrix.target, matrix.avro and hash of Cargo.lock for cache keys [avro]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on code in PR #2538:
URL: https://github.com/apache/avro/pull/2538#discussion_r1344376019
##########
.github/workflows/test-lang-rust-ci.yml:
##########
@@ -59,19 +60,27 @@ jobs:
uses: actions/checkout@v4
- name: Cache Cargo
+ id: cache-cargo
uses: actions/cache@v3
with:
# these represent dependencies downloaded by cargo
# and thus do not depend on the OS, arch nor rust version.
path: ~/.cargo
- key: cargo-cache1-
+ key: ${{ runner.os }}-cargo-cache1-${{ matrix.target }}-${{ matrix.rust }}-${{ hashFiles('**/Cargo.lock') }}
+
Review Comment:
I didn't use `${{ matrix.target }}` intentionally in #2517 because many caches are generated and take up the cache capacity.
See https://github.com/apache/avro/actions/caches
Rust related caches take up approximately 9GB while the capacity limit is 10GB.
Do you think it's cost effective for the degree of the improvement?
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3870: [Rust] Use matrix.target, matrix.avro and hash of Cargo.lock for cache keys [avro]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on code in PR #2538:
URL: https://github.com/apache/avro/pull/2538#discussion_r1344449873
##########
.github/workflows/test-lang-rust-ci.yml:
##########
@@ -59,19 +60,27 @@ jobs:
uses: actions/checkout@v4
- name: Cache Cargo
+ id: cache-cargo
uses: actions/cache@v3
with:
# these represent dependencies downloaded by cargo
# and thus do not depend on the OS, arch nor rust version.
path: ~/.cargo
- key: cargo-cache1-
+ key: ${{ runner.os }}-cargo-cache1-${{ matrix.target }}-${{ matrix.rust }}-${{ hashFiles('**/Cargo.lock') }}
+
Review Comment:
If you don't mind, I'll update that PR with a compromised solution.
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3870: [Rust] Use matrix.target, matrix.avro and hash of Cargo.lock for cache keys [avro]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on code in PR #2538:
URL: https://github.com/apache/avro/pull/2538#discussion_r1344376019
##########
.github/workflows/test-lang-rust-ci.yml:
##########
@@ -59,19 +60,27 @@ jobs:
uses: actions/checkout@v4
- name: Cache Cargo
+ id: cache-cargo
uses: actions/cache@v3
with:
# these represent dependencies downloaded by cargo
# and thus do not depend on the OS, arch nor rust version.
path: ~/.cargo
- key: cargo-cache1-
+ key: ${{ runner.os }}-cargo-cache1-${{ matrix.target }}-${{ matrix.rust }}-${{ hashFiles('**/Cargo.lock') }}
+
Review Comment:
I didn't use `${{ matrix.target }}` and `${{ matrix.rust }}` intentionally in #2517 because many caches are generated and take up the cache capacity.
See https://github.com/apache/avro/actions/caches
Rust related caches take up approximately 9GB while the capacity limit is 10GB.
Do you think it's cost effective for the degree of the improvement?
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3870: [Rust] Use `git` protocol to fetch dependencies until MSRV is 1.70.0 [avro]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on code in PR #2538:
URL: https://github.com/apache/avro/pull/2538#discussion_r1346163594
##########
.github/workflows/test-lang-rust-ci.yml:
##########
@@ -59,19 +60,22 @@ jobs:
uses: actions/checkout@v4
- name: Cache Cargo
+ id: cache-cargo
uses: actions/cache@v3
with:
# these represent dependencies downloaded by cargo
# and thus do not depend on the OS, arch nor rust version.
path: ~/.cargo
- key: cargo-cache1-
+ key: ${{ runner.os }}-cargo-cache2-
+
- name: Cache Rust dependencies
+ id: cache-target
uses: actions/cache@v3
with:
# these represent compiled steps of both dependencies and avro
# and thus are specific for a particular OS, arch and rust version.
- path: ~/target
- key: ${{ runner.os }}-target-cache1-${{ matrix.rust }}-
+ path: lang/rust/target
+ key: ${{ runner.os }}-target-cache2-
Review Comment:
By removing `${{ matrix.rust }}`, is this part meaningful?
I think built the deps cannot be shared among `stable`, `nightly`, `beta` and `1.65.0`.
So, I'm wondering this usage of cache has little effect, and it's better not to use cache.
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3870: [Rust] Use matrix.target, matrix.avro and hash of Cargo.lock for cache keys [avro]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on code in PR #2538:
URL: https://github.com/apache/avro/pull/2538#discussion_r1344385177
##########
.github/workflows/test-lang-rust-ci.yml:
##########
@@ -59,19 +60,27 @@ jobs:
uses: actions/checkout@v4
- name: Cache Cargo
+ id: cache-cargo
uses: actions/cache@v3
with:
# these represent dependencies downloaded by cargo
# and thus do not depend on the OS, arch nor rust version.
path: ~/.cargo
- key: cargo-cache1-
+ key: ${{ runner.os }}-cargo-cache1-${{ matrix.target }}-${{ matrix.rust }}-${{ hashFiles('**/Cargo.lock') }}
+
- name: Cache Rust dependencies
+ id: cache-target
uses: actions/cache@v3
with:
# these represent compiled steps of both dependencies and avro
# and thus are specific for a particular OS, arch and rust version.
- path: ~/target
- key: ${{ runner.os }}-target-cache1-${{ matrix.rust }}-
+ path: lang/rust/target
+ key: ${{ runner.os }}-target-cache1-${{ matrix.target }}-${{ matrix.rust }}-${{ hashFiles('**/Cargo.lock') }}
+
+ - name: Cache hit ?!
Review Comment:
Do we really need this job?
We can check cache hit another way.
https://github.com/apache/avro/actions/runs/6393032720/job/17352024653#step:3:22
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3870: [Rust] Use matrix.target, matrix.avro and hash of Cargo.lock for cache keys [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #2538:
URL: https://github.com/apache/avro/pull/2538#discussion_r1344447887
##########
.github/workflows/test-lang-rust-ci.yml:
##########
@@ -59,19 +60,27 @@ jobs:
uses: actions/checkout@v4
- name: Cache Cargo
+ id: cache-cargo
uses: actions/cache@v3
with:
# these represent dependencies downloaded by cargo
# and thus do not depend on the OS, arch nor rust version.
path: ~/.cargo
- key: cargo-cache1-
+ key: ${{ runner.os }}-cargo-cache1-${{ matrix.target }}-${{ matrix.rust }}-${{ hashFiles('**/Cargo.lock') }}
+
- name: Cache Rust dependencies
+ id: cache-target
uses: actions/cache@v3
with:
# these represent compiled steps of both dependencies and avro
# and thus are specific for a particular OS, arch and rust version.
- path: ~/target
- key: ${{ runner.os }}-target-cache1-${{ matrix.rust }}-
+ path: lang/rust/target
+ key: ${{ runner.os }}-target-cache1-${{ matrix.target }}-${{ matrix.rust }}-${{ hashFiles('**/Cargo.lock') }}
+
+ - name: Cache hit ?!
Review Comment:
No, this is just to see that it hits the cache. It will be removed before merging
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3870: [Rust] Use `git` protocol to fetch dependencies until MSRV is 1.70.0 [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #2538:
URL: https://github.com/apache/avro/pull/2538#issuecomment-1756153335
Closed in favour of #2517
--
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@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org